Skip to content

[WebProfilerBundle] Extend web profiler listener & config for replace on ajax requests #59123

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 15, 2025
Merged

[WebProfilerBundle] Extend web profiler listener & config for replace on ajax requests #59123

merged 1 commit into from
Jan 15, 2025

Conversation

chr-hertel
Copy link
Contributor

@chr-hertel chr-hertel commented Dec 7, 2024 •
edited
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

Basically a follow up of #26655 to make the feature of replacing the toolbar on ajax requests configurable instead of application side listeners for setting the Symfony-Debug-Toolbar-Replace header

when@dev:
    web_profiler:
        toolbar:
            ajax_replace: true # <---
        intercept_redirects: false

#SymfonyHackday

@carsonbot carsonbot added this to the 7.3 milestone Dec 7, 2024
@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch from a948029 to bb0c996 Compare December 7, 2024 11:25
@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch from bb0c996 to 2a695ed Compare December 7, 2024 11:30
javiereguiluz
javiereguiluz approved these changes Dec 7, 2024
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! Thanks Christopher.

The only thing that I'm not sure about is the name of the option: ajax_replace is very concise, but to me it's not self-explanatory

Here are some new name proposals for your consideration:

  • update_toolbar_from_ajax
  • refresh_toolbar_with_ajax
  • ajax_toolbar_refresh
  • sync_toolbar_with_ajax

@chr-hertel
Copy link
Contributor Author

what about toolbar_ajax_replace?

i would at least stick with replace since that is the term in the header as well

@javiereguiluz
Copy link
Member

Yes, I like toolbar_ajax_replace. Thanks!

@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch from 2a695ed to ee2248b Compare December 7, 2024 12:07
@chr-hertel
Copy link
Contributor Author

@javiereguiluz reworked it a bit further to have a structured config

@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch 2 times, most recently from 086eb64 to ee83789 Compare December 7, 2024 12:32
@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch from ee83789 to fe1f12a Compare December 8, 2024 23:21
@alexislefebvre
Copy link
Contributor

alexislefebvre commented Dec 10, 2024 •
edited
Loading

Can you please think again about naming the parameter refresh, update or reload instead of replace? As a developer and end user of the web profiler, “replace” content sounds strange - replacing with what? -, while refreshing or reloading looks more clear.

@chr-hertel
Copy link
Contributor Author

@alexislefebvre it's just like more confusing to me if header and option don't use the same terminology, is it not?

@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch 2 times, most recently from dfd14c2 to e311eb3 Compare December 10, 2024 22:39
@alexislefebvre
Copy link
Contributor

@alexislefebvre it's just like more confusing to me if header and option don't use the same terminology, is it not?

I'm sorry but I may miss some context here, what are header and option?

@chr-hertel
Copy link
Contributor Author

@alexislefebvre it's just like more confusing to me if header and option don't use the same terminology, is it not?

I'm sorry but I may miss some context here, what are header and option?

the underlying feature is already part of Symfony => when setting the response header Symfony-Debug-Toolbar-Replace the toolbar gets replaced on XHR

and since this config option is controlling whether the header is send or not, i would argue to have a similar wording - meaning to use the word "replace" for the option instead of renaming the header -> would be a breaking change without much value IMHO

@alexislefebvre
Copy link
Contributor

@alexislefebvre it's just like more confusing to me if header and option don't use the same terminology, is it not?

I'm sorry but I may miss some context here, what are header and option?

the underlying feature is already part of Symfony => when setting the response header Symfony-Debug-Toolbar-Replace the toolbar gets replaced on XHR

and since this config option is controlling whether the header is send or not, i would argue to have a similar wording - meaning to use the word "replace" for the option instead of renaming the header -> would be a breaking change without much value IMHO

Thanks for the explanation. As a dev and end-user of WPB I will probably not know about the Symfony-Debug-Toolbar-Replace so I would prefer that the YAML parameter has a clear meaning, even if the name differs from the implementation.

@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch 2 times, most recently from 00b167e to e093c12 Compare December 12, 2024 19:12
@OskarStark
Copy link
Contributor

We can add an info to the option to make the intend more clear, but I would also reuse the wording here.

@symfony/mergers please advice, thanks

@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch from e093c12 to 979f45f Compare December 20, 2024 13:30
@chr-hertel
Copy link
Contributor Author

done from my point of view - also tested the xml config with attribute and element.

@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch from 979f45f to 07b755d Compare December 23, 2024 00:11
@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch 2 times, most recently from aa6761b to 62a2507 Compare January 7, 2025 20:25
@chr-hertel chr-hertel force-pushed the feat-profiler-replace branch from 62a2507 to 3a723b5 Compare January 14, 2025 17:44
@fabpot
Copy link
Member

fabpot commented Jan 15, 2025

Thank you @chr-hertel.

@fabpot fabpot merged commit ed1166c into symfony:7.3 Jan 15, 2025
11 checks passed
@fabpot fabpot mentioned this pull request Jan 15, 2025
Closed
@chr-hertel chr-hertel deleted the feat-profiler-replace branch January 15, 2025 08:33
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@OskarStark OskarStark OskarStark approved these changes

@stof stof Awaiting requested review from stof

@maxbeckers maxbeckers Awaiting requested review from maxbeckers

Assignees
No one assigned
Labels
Feature Status: Reviewed WebProfilerBundle
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
9 participants
@chr-hertel @javiereguiluz @alexislefebvre @OskarStark @fabpot @nicolas-grekas @stof @maxbeckers @carsonbot