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

Dont register routes and middleware if we are not in debug mode #906

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Dont register routes and middleware if we are not in debug mode #906

merged 1 commit into from
Feb 18, 2019

Conversation

vkarampinis
Copy link
Contributor

ServiceProvider as is now, register routes and middleware even when we are not on debug mode APP_DEBUG=false

This patch allows ServiceProvider to boot only when app.debug is TRUE

@barryvdh barryvdh merged commit f78618a into barryvdh:master Feb 18, 2019
@JaZo
Copy link

JaZo commented Mar 18, 2019

We are using the debugbar in a staging environment without APP_DEBUG=true (we don't want to see a detailed exception page etc.) and only enable it deferred when a user is logged in. Although the docs state APP_DEBUG=true is required, it has been working just fine for years. Unfortunately this change broke it and the only fix is to overwrite the boot method of the ServiceProvider.

I've been thinking about making a PR with something like LaravelDebugbar::isEnabled to check config('debugbar.enabled') and fallback to config('app.debug'), but that doesn't work if we enable the debugbar deferred 😞 @barryvdh, do you have an idea if and how we can support our use case?

@vkarampinis
Copy link
Contributor Author

Good catch.
replacing this line will do the trick.
https://github.com/barryvdh/laravel-debugbar/blob/master/src/ServiceProvider.php#L66

- if (!$this->app['config']->get('app.debug')) {
+ if (!app('debugbar')->isEnabled()) {

@barryvdh your call. I can send a PR for this.

@JaZo
Copy link

JaZo commented Mar 19, 2019

@vkarampinis, unfortunately that would break enabling it at runtime.

@aalyusuf
Copy link

This also renders config's "enabled" obsolete as it cannot actually be overriden.

@barryvdh
Copy link
Owner

Should we just revert this?

@JaZo
Copy link

JaZo commented Mar 25, 2019

@barryvdh, I think it's better to do so. Registering those routes and the middleware doesn't look like a big performance issue so I don't see a reason why we shouldn't register those.

@barryvdh
Copy link
Owner

Reverted

@mnabialek
Copy link
Contributor

@barryvdh Please make new release with this fix. I'm using 3.2.3 and it's not reverted in this release

@barryvdh
Copy link
Owner

done

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.

None yet

5 participants