Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow swapping the modal implementation #66

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ragulka
Copy link

@ragulka ragulka commented Oct 10, 2023

This PR leverages the Laravel Service Container to create new instances of the Modal class. This allows developers to customize the Modal class by binding their own implementation to the container.

A concrete use case is to customize the Modal::handleRoute method to add additional middleware to the base route. For example, I need to forget a specific route parameter after a middleware runs, but by default Momentum Modal does not apply middleware to the base route at all (except for resolving the route parameters).

This change will not affect existing usage at all, but will make it easier (possible) to extend the existing functionality.

@what-the-diff
Copy link

what-the-diff bot commented Oct 10, 2023

PR Summary

  • Updated Documentation
    Provided instructions on how to use a custom Modal class in the README.md file. This helps users customize their implementation with ease.

  • Change of Modal Class Instantiation
    Modified the instantiation process of the Modal class by using the app()->makeWith method in ModalServiceProvider.php. This change increases the flexibility and scalability of the code.

  • Expanded Test Coverage
    Added a test case in ModalTest.php to verify that the modal implementation is easily swappable via the service container. This enhancement assures that our implementation continues to work even when the modal feature is replaced with another similar service.

  • Addition of a New Stub Class
    Included the ExampleModal stub class in the Stubs directory. This serves as a demonstration on how other developers can create similar classes within the application.

@darkons
Copy link

darkons commented Oct 17, 2023

I have a similar problem. In my case I am using the stancl/tenancy package and I need to apply the middleware for tenant identification on the base route.

Also, I think it could be very useful to add a config file to the package where you can add middlewares that you want to be always running:

// config/momentum-modal.php
'middlewares' => [
    Stancl\Tenancy\Middleware\InitializeTenancyByDomain::class,
    AnotherRandomMiddleware::class,
]
protected function handleRoute(Request $request, Route $route): mixed
    {
        /** @var \Illuminate\Routing\Router */
        $router = app('router');

        foreach (config('momentum-modal.middlewares') as $middleware) {
            $instance = App::make($middleware);

            if (method_exists($instance, 'handle')) {
                $instance->handle($request, fn () => $route->run());
            }
        }

        $middleware = new SubstituteBindings($router);

        return $middleware->handle(
            $request,
            fn () => $route->run()
        );
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants