Skip to content

Help Wanted: fix failure on WebProfilerBundle 5.4 loaded with higest deps #52210

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
nicolas-grekas opened this issue Oct 20, 2023 · 7 comments · Fixed by #52229
Closed

Help Wanted: fix failure on WebProfilerBundle 5.4 loaded with higest deps #52210

nicolas-grekas opened this issue Oct 20, 2023 · 7 comments · Fixed by #52229

Comments

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 20, 2023

On 5.4, we have a failure to fix:
https://github.com/symfony/symfony/actions/runs/6590102626/job/17912208613#step:7:393

I can reproduce it by going into the src/Symfony/Bundle/WebProfilerBundle folder on branch 5.4, then running composer up and
SYMFONY_DEPRECATIONS_HELPER=weak ../../../../phpunit --filter testPanelActionWithLatestToken$

Help welcome to figure out the issue and fix it!

@smnandre
Copy link
Member

smnandre commented Oct 21, 2023

1) Symfony\Bundle\WebProfilerBundle\Tests\Controller\ProfilerControllerTest::testPanelActionWithLatestToken
UnexpectedValueException: The profiler template "@WebProfiler/Collector/command.html.twig" for data collector "command" does not exist.

Two choices:

  • either the command profiler template is missing in this folder
  • either the definition of the collector must be deleted
  ".data_collector.command" => array:2 [
    0 => "command"
    1 => "@WebProfiler/Collector/command.html.twig"
  ]

@nicolas-grekas
Copy link
Member Author

The change should be done in 6.4 ideally /cc @HeahDude

@HeahDude
Copy link
Contributor

It seems WebProfilerBundle 5.4 cannot be compatible with FrameworkBundle 6.4, the latter being responsible for configuring collectors.

I see tow options:

  • add a compiler pass in WebProfilerBundle 5.4 to remove new data collectors introduced by 6.x (I guess we will have the same error with the Workflow data collector if we fix this only for the command data collector).
  • add a conflict rule in FrameworkBundle 6.4 to not allow WebProfilerBundle < 6.4.

I would go for the second solution.

@nicolas-grekas
Copy link
Member Author

Patching 5.4 to account for 6.4 would be wrong, so solution 2 would be the way. Ideally, I'd prefer FWB to not know about WPB. Is there no solution that doesn't involve adding any conflict line?

@HeahDude
Copy link
Contributor

See #52218.

@HeahDude
Copy link
Contributor

The conflict line was already there because I guess the problem was always there, adding collectors in FWB while adding templates in WebProfilerBundle.

nicolas-grekas added a commit that referenced this issue Oct 21, 2023
…6.4 (HeahDude)

This PR was merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle] Add conflict with `WebProfilerBundle` < 6.4

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | #52210
| License       | MIT

Commits
-------

6f8c912 [FrameworkBundle] Add conflict with `WebProfilerBundle` < 6.4
@HeahDude
Copy link
Contributor

So we can close here I guess.

@chalasr chalasr closed this as completed Oct 21, 2023
chalasr added a commit that referenced this issue Oct 22, 2023
…tered (smnandre)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[FrameworkBundle] Fix CommandDataCollector is always registered

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #52210
| License       | MIT

Another attempt at fixing #52210, allowing to revert #52218 and leave the conflict constraints untouched

CommandDataCollector was always registered, and not always removed from the container when not needed/invalid.

Commits
-------

6e2c2ec [FrameworkBundle] Fix CommandDataCollector is always registered
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 a pull request may close this issue.

4 participants