Skip to content

[WebProfilerBundle] Add intercept redirects configuration from toolbar #47663

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

Closed

Conversation

Nicals
Copy link
Contributor

@Nicals Nicals commented Sep 22, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR

This PR adds a convenient way to override the WebProfilter intercept_redirects settings from the frontend toolbar.

When the need come to intercept redirect when developing applications, one need to edit the config/packages/web_profiler.yaml file (risking to commit it by mistake).
With this PR addition, a single click on a checkbox added to the toolbar config tab will change the intercept_redirects settings for the current browser session as seen in the following screenshot.

intercept-redirects

This feature was inspired by django's Web Debug Toolbar intercept redirect functionality which can also be triggered from the frontend part of the application.

@carsonbot carsonbot added this to the 6.2 milestone Sep 22, 2022
@Nicals Nicals changed the title add intercept redirects configuration from toolbar [WebProfilerBundle] Add intercept redirects configuration from toolbar Sep 22, 2022
@tigitz
Copy link
Contributor

tigitz commented Sep 26, 2022

To me, the convenience it brings is not worth the added code complexity and maintenance while you could already do intercept_redirects: '%env(resolve:WEBPROFILER_INTERCEPT_REDIRECTS)%' with WEBPROFILER_INTERCEPT_REDIRECTS=true in your .env.local and achieve relatively the same thing.

@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@fabpot
Copy link
Member

fabpot commented Dec 9, 2022

I agree with the previous comment. In any case, I'm not comfortable with adding configuration in the web profiler directly.
WDYT @symfony/mergers?

@Nicals
Copy link
Contributor Author

Nicals commented Dec 9, 2022

I agree with the added complexity. Without taking a look at the code base, my first idea was that I would be able to add this feature as part as an external bundle.

After seeing this long chain of ifs in WebDebugToolbarListener::onKernelResponse, i saw that it would be harder than I first thought.

Such a feature could be easily inserted in the listener logic if this long if chain was refactored in some kind of Chain of Responsability, the Listener being its Client.
I think this could lower the need for mocks in unit tests, the overall complexity of the actual listener and allow a more extensible debug listener.

Would this be an improvement that appeals to you ?

@javiereguiluz
Copy link
Member

I'm divided about this (as Fabien, I don't like to add config in the toolbar ... but as upvotes showed, this specific option shortcut would be convenient sometimes).

After thinking about this, I think my vote would be 👎 In addition to the added complexity, this could be used in the future to add more config options directly in the toolbar.

Moreover, this introduces a mismatch between your Symfony config files and the Symfony behavior (e.g. you have false in the intercept option in config/packages/web_profiler.yaml file ... and you click on this option but forget about it ... then redirects are intercepted even if it's false in your config, so you are very confused). Related to this, we'd need 3 states for this option: yes (intercept), no (don't intercept), unset (do what the config file says).

In any case, I appreciate the effort that you put on creating this pull request.

@Nyholm
Copy link
Member

Nyholm commented Dec 15, 2022

Thank you for this suggestion. I like the idea.
But I think we need to consider the feature "config in the toolbar" and how that is implemented properly before we can tackle this. (ie, to handle weird edge cases that Javier points out)

I suggest to close this PR now and then reopen the idea as soon as we know a good architecture/strategy for options.

@chalasr
Copy link
Member

chalasr commented Dec 29, 2022

Same here, I don't expect content that is not purely informational in this area. Closing, thanks for proposing.

@chalasr chalasr closed this Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants