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

[FrameworkBundle] Fix CommandDataCollector is always registered #52229

Merged
merged 1 commit into from Oct 22, 2023

Conversation

smnandre
Copy link
Contributor

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.

@smnandre
Copy link
Contributor Author

cc @nicolas-grekas

@chalasr chalasr merged commit b9334ab into symfony:6.4 Oct 22, 2023
6 of 9 checks passed
@chalasr
Copy link
Member

chalasr commented Oct 22, 2023

Thanks @smnandre

@@ -922,7 +923,7 @@ private function registerProfilerConfiguration(array $config, ContainerBuilder $
$container->removeDefinition('console_profiler_listener');
}

if (!class_exists(CommandDataCollector::class)) {
if (!$container->getParameter('kernel.debug') || !class_exists(CliRequest::class) || !class_exists(CommandDataCollector::class)) {
Copy link
Member

Choose a reason for hiding this comment

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

why this check for kernel.debug ? If you enable the profiler in non-debug mode (for instance in tests), you will need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, i’ll digg tomorrow

Copy link
Member

Choose a reason for hiding this comment

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

This line will also be reverted in #52274.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's continue discussion (or stop it) on #52274 then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found it: it was because then (non-debug mode), there is something not working, but this is not directly related to the composer conflit, and i'll open a dedicated issue.

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.

Help Wanted: fix failure on WebProfilerBundle 5.4 loaded with higest deps
6 participants