Register commands using PSR-4 based Service Discovery #684

GuilhemN · 2017-08-06T14:20:28Z

Commands auto-registration will be deprecated soon (see symfony/symfony#23805) so we need to stop using it.

Koc · 2017-08-06T15:26:36Z

DependencyInjection/DoctrineExtension.php

+
+            $prototype = new Definition();
+            $prototype
+                ->setPublic(False)


Indeed, fixed

Ocramius · 2017-08-06T15:40:38Z

DependencyInjection/DoctrineExtension.php

@@ -72,6 +72,20 @@ public function load(array $configs, ContainerBuilder $container)

        $this->adapter->loadServicesConfiguration($container);

+        // Use the default logic when the ORM is available.
+        // This avoids listing all ORM commands by hand.
+        if (class_exists('Doctrine\ORM\Version') && method_exists(XmlFileLoader::class, 'registerClasses')) {


Version::class

Ocramius · 2017-08-06T15:41:14Z

DependencyInjection/DoctrineExtension.php

+                ->addTag('console.command')
+            ;
+
+            $loader->registerClasses($prototype, 'Doctrine\Bundle\DoctrineBundle\Command\\', '../../Command/*');


Please add __DIR__

Ocramius · 2017-08-06T15:42:03Z

DoctrineBundle.php

@@ -143,7 +144,10 @@ public function registerCommands(Application $application)
        // Use the default logic when the ORM is available.
        // This avoids listing all ORM commands by hand.
        if (class_exists('Doctrine\\ORM\\Version')) {
-            parent::registerCommands($application);
+            if (!method_exists(FileLoader::class, 'registerClasses')) {
+                // BC sf < 3.3


Could bump the dependency instead

Is it ok? It will forbid using sf 2.x.

Yes, that is OK. This is not a bugfix, so it will land in the next minor anyway

Ocramius · 2017-08-06T16:29:59Z

@GuilhemN the commit messages: make them descriptive, please

GuilhemN · 2017-08-06T16:51:53Z

@Ocramius I rebased, these commits were just here so you could know the changes I've done, they didn't bring much value otherwise.

ogizanagi · 2017-08-06T22:52:59Z

DependencyInjection/DoctrineExtension.php

@@ -72,6 +72,20 @@ public function load(array $configs, ContainerBuilder $container)

        $this->adapter->loadServicesConfiguration($container);

+        // Use a prototype when the ORM is available.
+        // This avoids listing all ORM commands by hand.
+        if (class_exists(Version::class) && method_exists(XmlFileLoader::class, 'registerClasses')) {


&& method_exists(XmlFileLoader::class, 'registerClasses') part of the condition could be removed now that the DI dependency has been bumped to 3.3.

indeed, updated

Ocramius

Besides nitpicks, LGTM 👍

@kimhemsoe can you check on which milestone this has to land?

Ocramius · 2017-08-07T15:36:52Z

DoctrineBundle.php

@@ -140,11 +140,8 @@ public function shutdown()
     */
    public function registerCommands(Application $application)
    {
-        // Use the default logic when the ORM is available.
-        // This avoids listing all ORM commands by hand.
        if (class_exists('Doctrine\\ORM\\Version')) {


Version::class

kimhemsoe · 2017-08-07T18:27:36Z

Are we OK with dropping support for sf2.7?
Seems to me that it would be possible to support 3.4 without dropping support for 2.7 without much trouble.

kimhemsoe · 2017-08-07T18:57:03Z

Sorry, we pull in symfony/dependency-injection 3.3, which require finder, config and yaml to be 3.3 if installed, if i am correct?

Is that any nasty surprises here? Like is that any issues with console which yaml require to be 3.3?

GuilhemN · 2017-08-08T09:55:41Z

Are we OK with dropping support for sf2.7?
Seems to me that it would be possible to support 3.4 without dropping support for 2.7 without much trouble.

@Ocramius told me it was ok (#684 (comment)).

Sorry, we pull in symfony/dependency-injection 3.3, which require finder, config and yaml to be 3.3 if installed, if i am correct?

Is that any nasty surprises here? Like is that any issues with console which yaml require to be 3.3?

Indeed I had to bump symfony/framework-bundle and symfony/yaml.

Ocramius · 2017-08-08T13:37:09Z

@kimhemsoe yeah, dropping symfony 2.x and <3.3 is perfectly OK for new improvement releases.

kimhemsoe · 2017-08-08T13:58:30Z

OK i will try to get the last bug fix in for a 1.7 release then. And then we set this for 1.8

GuilhemN · 2017-08-09T17:55:50Z

symfony/symfony#23805 has been merged :)

chalasr · 2017-08-11T09:15:12Z

DI <3.3 layers should be removed once this merged.

Ocramius · 2017-09-19T11:46:08Z

@kimhemsoe please rebase instead

kimhemsoe · 2017-09-19T12:02:03Z

Will do, also need to fix that travis failure we have.

Hmm.. did not see that the edit ended up with a merge.

Majkl578 · 2017-09-19T17:38:27Z

DependencyInjection/DoctrineExtension.php

@@ -73,6 +73,20 @@ public function load(array $configs, ContainerBuilder $container)

        $this->adapter->loadServicesConfiguration($container);

+        // Use a prototype when the ORM is available.
+        // This avoids listing all ORM commands by hand.
+        if (class_exists(Version::class)) {


This type of check should be avoided since Version class is being removed in 3.0, see doctrine/orm#6653.
Probably good enough for now (already used in registerCommands()), but should be handled differently in future.

nicolas-grekas · 2017-10-13T17:27:53Z

can be closed

GuilhemN changed the title ~~Register commands use PSR-4 based Service Discovery~~ Register commands using PSR-4 based Service Discovery Aug 6, 2017

Koc reviewed Aug 6, 2017

View reviewed changes

Ocramius suggested changes Aug 6, 2017

View reviewed changes

ogizanagi approved these changes Aug 6, 2017

View reviewed changes

Ocramius approved these changes Aug 7, 2017

View reviewed changes

Ocramius added the Feature label Aug 7, 2017

Ocramius assigned kimhemsoe Aug 7, 2017

Register commands using PSR-4 based Service Discovery

6c56f29

Bump symfony/framework-bundle and symfony/yaml

06271f8

kimhemsoe added this to the 1.8.0 milestone Aug 8, 2017

GuilhemN mentioned this pull request Aug 11, 2017

[HttpKernel] Deprecate Bundle::registerCommands() symfony/symfony#23861

Closed

chalasr approved these changes Aug 11, 2017

View reviewed changes

kimhemsoe mentioned this pull request Aug 30, 2017

Auto-registration of commands deprecated since Symfony 3.4 #693

Closed

Merge branch 'master' into commands

e4eb1bc

Majkl578 reviewed Sep 19, 2017

View reviewed changes

fabpot mentioned this pull request Sep 29, 2017

This bundle should follow Symfony min requirements #704

Closed

fabpot closed this Oct 13, 2017

GuilhemN deleted the commands branch May 31, 2020 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register commands using PSR-4 based Service Discovery #684

Register commands using PSR-4 based Service Discovery #684

GuilhemN commented Aug 6, 2017

Koc Aug 6, 2017

GuilhemN Aug 6, 2017

Ocramius Aug 6, 2017

GuilhemN Aug 6, 2017

Ocramius Aug 6, 2017

GuilhemN Aug 6, 2017

Ocramius Aug 6, 2017

GuilhemN Aug 6, 2017

Ocramius Aug 6, 2017

GuilhemN Aug 6, 2017

Ocramius commented Aug 6, 2017

GuilhemN commented Aug 6, 2017

ogizanagi Aug 6, 2017

GuilhemN Aug 7, 2017

Ocramius left a comment

Ocramius Aug 7, 2017

GuilhemN Aug 7, 2017

kimhemsoe commented Aug 7, 2017

kimhemsoe commented Aug 7, 2017

GuilhemN commented Aug 8, 2017

Ocramius commented Aug 8, 2017

kimhemsoe commented Aug 8, 2017

GuilhemN commented Aug 9, 2017

chalasr commented Aug 11, 2017

Ocramius commented Sep 19, 2017

kimhemsoe commented Sep 19, 2017

Majkl578 Sep 19, 2017

nicolas-grekas commented Oct 13, 2017

Register commands using PSR-4 based Service Discovery #684

Register commands using PSR-4 based Service Discovery #684

Conversation

GuilhemN commented Aug 6, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Aug 6, 2017

GuilhemN commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimhemsoe commented Aug 7, 2017

kimhemsoe commented Aug 7, 2017

GuilhemN commented Aug 8, 2017

Ocramius commented Aug 8, 2017

kimhemsoe commented Aug 8, 2017

GuilhemN commented Aug 9, 2017

chalasr commented Aug 11, 2017

Ocramius commented Sep 19, 2017

kimhemsoe commented Sep 19, 2017

Choose a reason for hiding this comment

nicolas-grekas commented Oct 13, 2017