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

Deamon · 2017-01-21T00:23:01Z

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

xabbuh · 2017-01-21T08:18:26Z

UPGRADE-3.3.md

@@ -34,6 +34,9 @@ FrameworkBundle
 ---------------

 * 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.


This is missing the Compiler part of the namespace.

xabbuh · 2017-01-21T08:18:36Z

UPGRADE-4.0.md

@@ -156,6 +156,9 @@ FrameworkBundle

 * 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


xabbuh · 2017-01-21T08:19:59Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

@@ -74,7 +74,10 @@ public function build(ContainerBuilder $container)
        parent::build($container);

        $container->addCompilerPass(new RoutingResolverPass());
-        $container->addCompilerPass(new ProfilerPass());
+        if (class_exists(AddConsoleCommandPass::class)) {


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

you're right, this will be deleted.

xabbuh · 2017-01-21T08:21:43Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

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


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.

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 ?

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 ?

@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?

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

xabbuh · 2017-01-21T09:09:08Z

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

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\WebProfilerBundle\DependencyInjection;


Should be moved to the Compiler subnamespace.

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

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

In AddConsoleCommandPass you were moving the file from Symfony\Bundle\FrameworkBundle to the component.
Here I am moving the file from Symfony\Bundle\FrameworkBundle to Symfony\Bundle\WebProfilerBundle.
After a short looking I figured that every "Bundle" in that namespace have a repository Compiler in there DIfolder so I think in that case I should move it in a subfolder.

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

chalasr · 2017-01-21T12:37:09Z

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

Deamon · 2017-01-21T16:05:08Z

@chalasr I updated the description to reflect that.

Deamon · 2017-01-21T17:42:23Z

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

chalasr

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.

Deamon · 2017-01-22T18:44:04Z

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

chalasr · 2017-01-23T14:41:38Z

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -57,7 +57,8 @@
    "conflict": {
        "phpdocumentor/reflection-docblock": "<3.0",
        "phpdocumentor/type-resolver": "<0.2.0",
-        "symfony/console": "<3.3"
+        "symfony/console": "<3.3",
+        "symfony/web-profiler-bundle": "<3.3"


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

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

…iler bundle

stof · 2017-01-23T16:48:49Z

-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

chalasr · 2017-01-24T09:01:16Z

@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.

fabpot · 2017-02-16T13:16:37Z

Closing as this is indeed wrong.

carsonbot added Status: Needs Review FrameworkBundle WebProfilerBundle Feature Deprecation labels Jan 21, 2017

Deamon force-pushed the move-profiler-pass branch from 2c03f03 to 08c0e9e Compare January 21, 2017 00:37

xabbuh requested changes Jan 21, 2017

View reviewed changes

Deamon force-pushed the move-profiler-pass branch from 08c0e9e to 3afe8a2 Compare January 21, 2017 17:33

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

chalasr mentioned this pull request Jan 22, 2017

[FrameworkBundle] Move compiler passes to their component #21284

Closed

11 tasks

Deamon force-pushed the move-profiler-pass branch 3 times, most recently from 9967d97 to 904b3e8 Compare January 22, 2017 18:30

Deamon force-pushed the move-profiler-pass branch from 904b3e8 to a970440 Compare January 23, 2017 13:48

chalasr requested changes Jan 23, 2017

View reviewed changes

Deamon force-pushed the move-profiler-pass branch 2 times, most recently from 3c7e18f to ebe2b93 Compare January 23, 2017 15:43

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

f98739b

…iler bundle

Deamon force-pushed the move-profiler-pass branch from ebe2b93 to f98739b Compare January 23, 2017 15:58

nicolas-grekas added this to the 3.3 milestone Jan 24, 2017

fabpot closed this Feb 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Deamon commented Jan 21, 2017 •

edited

xabbuh Jan 21, 2017

xabbuh Jan 21, 2017

xabbuh Jan 21, 2017

Deamon Jan 21, 2017

xabbuh Jan 21, 2017

Deamon Jan 21, 2017

Deamon Jan 21, 2017

chalasr Jan 21, 2017 •

edited

xabbuh Jan 22, 2017

xabbuh Jan 21, 2017

Deamon Jan 21, 2017

chalasr Jan 21, 2017

Deamon Jan 21, 2017

chalasr Jan 22, 2017

chalasr commented Jan 21, 2017

Deamon commented Jan 21, 2017

Deamon commented Jan 21, 2017 •

edited

chalasr left a comment

Deamon commented Jan 22, 2017

chalasr Jan 23, 2017

Deamon Jan 23, 2017

stof commented Jan 23, 2017

chalasr commented Jan 24, 2017

fabpot commented Feb 16, 2017

		@@ -156,6 +156,9 @@ FrameworkBundle

		* 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

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

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

Conversation

Deamon commented Jan 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Jan 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Jan 21, 2017

Deamon commented Jan 21, 2017

Deamon commented Jan 21, 2017 • edited

chalasr left a comment

Choose a reason for hiding this comment

Deamon commented Jan 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 23, 2017

chalasr commented Jan 24, 2017

fabpot commented Feb 16, 2017

Deamon commented Jan 21, 2017 •

edited

chalasr Jan 21, 2017 •

edited

Deamon commented Jan 21, 2017 •

edited