NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[FrameworkBundle][WebProfilerBundle] Move ProfilerPass to the WebProfiler bundle#21368

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Deamon wants to merge1 commit intosymfony:masterfromDeamon:move-profiler-pass

Closed

[FrameworkBundle][WebProfilerBundle] Move ProfilerPass to the WebProfiler bundle#21368

Deamon wants to merge1 commit intosymfony:masterfromDeamon:move-profiler-pass

Conversation

Copy link

Contributor

Deamon commentedJan 21, 2017•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes/no
Fixed tickets	-
License	MIT
Doc PR	n/a

This MR is part of the#21284 todo list

carsonbot added Status: Needs Review FrameworkBundle WebProfilerBundle Feature Deprecation labels

Jan 21, 2017

Deamon force-pushed themove-profiler-pass branch from2c03f03 to08c0e9eCompare

January 21, 2017 00:37

xabbuh requested changes

Jan 21, 2017

View reviewed changes

UPGRADE-3.3.md Outdated

		* The`Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConsoleCommandPass` has been deprecated. Use`Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass` instead.

		* The`ProfilerPass` class has been deprecated and will be removed in 4.0, use the
		`Symfony\Bundle\WebProfilerBundle\DependencyInjection\ProfilerPass` class instead.

Copy link

Member

Choose a reason for hiding this comment

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

This is missing theCompiler part of the namespace.

UPGRADE-4.0.md Outdated


		* The`Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddConsoleCommandPass` has been removed. Use`Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass` instead.

		* The`ProfilerPass` class has been removed, use the

Copy link

Member

Choose a reason for hiding this comment

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

same here

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php Outdated


		$container->addCompilerPass(newRoutingResolverPass());
		$container->addCompilerPass(newProfilerPass());
		if (class_exists(AddConsoleCommandPass::class)) {

Copy link

Member

Choose a reason for hiding this comment

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

This change does not look related to the goal of the PR.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

you're right, this will be deleted.

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

		if (class_exists(AddConsoleCommandPass::class)) {
		$container->addCompilerPass(newAddConsoleCommandPass());
		}
		$this->addCompilerPassIfExists($container, ProfilerPass::class);

Copy link

Member

Choose a reason for hiding this comment

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

You have to specify the FQCN of an alternative implementation (the already existing one) that will be used as a fallback to provide backwards compatibility.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

You're saying that I should write

if (class_exists(ProfilerPass::class)) {    $container->addCompilerPass(new ProfilerPass());}

and duplicate the code ?
or just say somewhere that I took this part of the code from#21283 which has already been reviewed ?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Or do I have to link to the old Implementation ?

or the last chance, make an if/else to import the new one or the old one ?

Copy link

Member

chalasrJan 21, 2017•
edited
Loading

Choose a reason for hiding this comment

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

@xabbuh In fact this doesn't expect a fallback. For the AddConsoleCommandPass, we just added a conflict to the frameworkbundle so that it is always available (it was your suggestion#19443 (comment)). I started to do the same for the FormPass (#21283) and SerializerPass(#21293), is it ok to do the same here or should we fallback to the deprecated one and mute deprecation instead?

Copy link

Member

xabbuhJan 22, 2017

Choose a reason for hiding this comment

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

You are right. Adding the conflict rule should be the solution here too.

src/Symfony/Bundle/WebProfilerBundle/DependencyInjection/ProfilerPass.php Outdated

		* file that was distributed with this source code.
		*/

		namespaceSymfony\Bundle\WebProfilerBundle\DependencyInjection;

Copy link

Member

xabbuhJan 21, 2017

Choose a reason for hiding this comment

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

Should be moved to theCompiler subnamespace.

Copy link

ContributorAuthor

DeamonJan 21, 2017

Choose a reason for hiding this comment

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

Should I change only the namespace or move the file in a subdirectory ?

Copy link

Member

chalasrJan 21, 2017

Choose a reason for hiding this comment

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

@xabbuh is it worth it? We did not do it for theAddConsoleCommandPass in#19443, going to do the same for FormPass and SerializerPass

Copy link

ContributorAuthor

DeamonJan 21, 2017

Choose a reason for hiding this comment

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

InAddConsoleCommandPass you were moving the file fromSymfony\Bundle\FrameworkBundle to the component.
Here I am moving the file fromSymfony\Bundle\FrameworkBundle toSymfony\Bundle\WebProfilerBundle.
After a short looking I figured that every "Bundle" in that namespace have a repositoryCompiler in thereDIfolder so I think in that case I should move it in a subfolder.

Copy link

Member

chalasrJan 22, 2017

Choose a reason for hiding this comment

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

Indeed, the Compiler subnamespace seems to concern bundles only, sorry.

Copy link

Member

chalasr commentedJan 21, 2017

#21284 should not be closed after this, it will only be partially fixed.

Copy link

ContributorAuthor

Deamon commentedJan 21, 2017

@chalasr I updated the description to reflect that.

Deamon force-pushed themove-profiler-pass branch from08c0e9e to3afe8a2Compare

January 21, 2017 17:33

Copy link

ContributorAuthor

Deamon commentedJan 21, 2017•
edited
Loading

@xabbuh,@chalasr, I did a mistake while commiting validated fixes,this discussion still open

Deamon changed the title~~WIP: [FrameworkBundle][WebProfilerBundle] Move ProfilerPass to the WebProfiler bundle~~[FrameworkBundle][WebProfilerBundle] Move ProfilerPass to the WebProfiler bundle

Jan 22, 2017

chalasr requested changes

Jan 22, 2017

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

The FrameworkBundle's composer.json needs a conflict rule for"symfony/web-profiler-bundle": "<3.3", and its changelog needs a mention for the deprecated pass.

chalasr mentioned this pull request

Jan 22, 2017

[FrameworkBundle] Move compiler passes to their component#21284

Closed

11 tasks

Deamon force-pushed themove-profiler-pass branch 3 times, most recently from9967d97 to904b3e8Compare

January 22, 2017 18:30

Copy link

ContributorAuthor

Deamon commentedJan 22, 2017

The conflict line has been added in the composer.json file and a note in the changelog of the FrameworkBundle.

Deamon force-pushed themove-profiler-pass branch from904b3e8 toa970440Compare

January 23, 2017 13:48

chalasr requested changes

Jan 23, 2017

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/composer.json

		"phpdocumentor/type-resolver":"<0.2.0",
		"symfony/console":"<3.3"
		"symfony/console":"<3.3",
		"symfony/web-profiler-bundle":"<3.3"

Copy link

Member

chalasrJan 23, 2017

Choose a reason for hiding this comment

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

"symfony/web-profiler-bundle": "~3.3" must be added as dev requirement in addition of the conflict, that should make the build green.

Copy link

ContributorAuthor

DeamonJan 23, 2017

Choose a reason for hiding this comment

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

That's what I figured inyour comment.
The fix is coming.

Deamon force-pushed themove-profiler-pass branch 2 times, most recently from3c7e18f toebe2b93Compare

January 23, 2017 15:43

[FrameworkBundle][WebProfilerBundle] Move ProfilerPass to the WebProf…

f98739b

…iler bundle

Deamon force-pushed themove-profiler-pass branch fromebe2b93 tof98739bCompare

January 23, 2017 15:58

Copy link

Member

stof commentedJan 23, 2017

-1 for this. the configuration to enable the profiler itself is also part of FrameworkBundle.

WebProfilerBundle is currently only responsible for building a UI for the profiler, not for integrating the profiler itself.
The profiler is part of HttpKernel, not of WebProfilerBundle

Copy link

Member

chalasr commentedJan 24, 2017

@stof is right as usual.

Removed this pass from the list, might be reconsidered if we come to extract the profiler from the HttpKernel to its own component, but its place is not the WebProfilerBundle for sure.
I'll double check the remaining tasks of#21284 to avoid such issues.