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

Omitting withMiddleware() and withExceptions() crashes the app without any error message #50516

Closed
benjamincrozat opened this issue Mar 13, 2024 · 7 comments
Assignees

Comments

@benjamincrozat
Copy link

Laravel Version

11.0

PHP Version

8.3

Database Driver & Version

SQLite

Description

Hello,

While migrating to the new skeleton, I encountered a problem where removing withMiddleware() and withExceptions() in bootstrap/app.php crashed the app without any error message.

I think it'd be great to let people do it, or at least show an error message.

That's it and much love to the team!

Benjamin

Steps To Reproduce

  1. laravel new example-app
  2. Remove withMiddleware() and withExceptions() from bootstrap/app.php.
  3. Open the app in your browser to see the blank page without any error message.
@nunomaduro
Copy link
Member

This is not a bug - those are instructions that should remain untouched like many other things in the skeleton. We will keep an eye on this, if we see other people trying to remove that line.

@driesvints
Copy link
Member

Hi @benjamincrozat. This is definitely something that can be improved but actually not different from the behaviour in Laravel v10 where if you removed a line in bootstrap/app the same thing would happen. Thanks for bringing this up though. If anyone is up for it we'd appreciate a PR in a non breaking way to make the error more obvious.

@benjamincrozat
Copy link
Author

@nunomaduro @driesvints Thanks guys! Obviously, this is a super edge-case. 🙂 Just wanted to document that at least. We never know and that may show up in someone's Google search.

@PerryvanderMeer
Copy link
Contributor

Actually I tried to remove those once, so would like to see those go 🔥

@coderabbi
Copy link
Contributor

coderabbi commented Mar 14, 2024

Yeah, I'd also be keen on seeing it optional.

Granted, most application will make use of these method calls, but would you scratch your head over userland code that made seeming noop calls? Of course you would. But in this case they aren't noop? Well, why not, they are empty, right? Well... magic.

Overall, I just really prefer that the user own as much of the skeleton as possible.

@epixian
Copy link

epixian commented Mar 14, 2024

Obviously, this is a super edge-case.

Seems more like the regular case. If this function call is required for the app to work, seems like it should be the framework's responsibility to initialize it properly if omitted. Taylor's rationale (which I agree with) was that the vast majority of users did not remove any of the standard 9 middleware from their app. Like other opt-in changes to L11 such as broadcasting/api, this should be an optional call, where the defaults are provided in the framework so that if this call is missing the app would still boot properly.

The builder-like methods in the Middleware class allow for advanced customization but it feels more like this is after-the-fact editing of a built thing rather than building it yourself if you don't like the defaults. Perhaps instead registering the base middleware could be opt-in via publishing a middleware.php to the bootstrap dir similar to how the providers are listed their own file. If the file does not exist, it would just load the standard 9 middleware from the framework. Then we don't have to append/replace/except/priority etc. Keys not present in the returned array would use the default framework values.

// bootstrap/middleware.php (after publishing)
<?php

return [
    // customize the base middleware
    'global' => [
        \Illuminate\Http\Middleware\TrustHosts::class : null,
        \Illuminate\Http\Middleware\TrustProxies::class,
        \Illuminate\Http\Middleware\HandleCors::class,
        \Illuminate\Foundation\Http\Middleware\PreventRequestsDuringMaintenance::class,
        \Illuminate\Http\Middleware\ValidatePostSize::class,
        \Illuminate\Foundation\Http\Middleware\TrimStrings::class,
        \Illuminate\Foundation\Http\Middleware\ConvertEmptyStringsToNull::class,
    ],

    'groups' => [
        // replace 'web' defaults...
        'web' => [
            \Illuminate\Cookie\Middleware\EncryptCookies::class,
            \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class,
            \Illuminate\Session\Middleware\StartSession::class,
            \Illuminate\View\Middleware\ShareErrorsFromSession::class,
            \Illuminate\Foundation\Http\Middleware\ValidateCsrfToken::class,
            \Illuminate\Routing\Middleware\SubstituteBindings::class,
            // \Illuminate\Session\Middleware\AuthenticateSession::class,
        ],

        // ...but keep 'api' defaults (by omitting the key)
    ],
];

Given this file existing (or not!), we wouldn't have to have a ->withMiddleware() call when configuring the application in bootstrap/app.php. Of course we could still have such a call, but it would act on what has already been built via framework init and the middleware.php file, and you could still use the builder-style append/prepend etc methods for further customization.

@navidmafi
Copy link

Hi

Could we add a comment or some documentation to warn against completely removing these chained methods? The only way I could find out about this was using git blame and even then it took a bit of time and was confusing.

thanks!

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

No branches or pull requests

7 participants