ensure we return null rather than void in SyliusPluginTrait::getContainerExtension

[lsmith77]

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets
License	MIT

I am not entirely sure what is going on here.
It seems for one that when I implemented a proper Di extension in FOSSyliusImportExportPlugin I suddenly ended up with

  [Payum\Core\Exception\InvalidArgumentException]
  The storage factory with such name filesystem already registered

Which in turn seems to be caused by suddenly \Payum\Bundle\PayumBundle\PayumBundle::build being called twice.

Messing around I eventually ended up at:

PHP Fatal error:  Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Type error: Return value of FriendsOfSylius\SyliusImportExportPlugin\FOSSyliusImportExportPlugin::getContainerExtension() must implement interface Symfony\Component\DependencyInjection\Extension\ExtensionInterface or be null, none returned in /Users/lsmith/htdocs/SyliusImportExportPlugin/vendor/sylius/sylius/src/Sylius/Bundle/CoreBundle/Application/SyliusPluginTrait.php:71

Which led me to this PR and which magically fixed all the issues I had once I removed all my debug code.

[lsmith77]

Ok .. it seems like the issue is a bit deeper.

I run the following:

• (cd tests/Application && bin/console config:dump fos_sylius_import_export -e=test)
  • results in The storage factory with such name filesystem already registered
• (cd tests/Application && bin/console config:dump framework -e=test)
  • results in the expected output
• (cd tests/Application && bin/console config:dump fos_sylius_import_export -e=test)
  • results in the expected output if I have this PR checked out

I assume this is because if the cache is filled (though I don't think my test app actually has the cache enabled .. so wtf?) then it doesn't end up calling the PayumBundle::build() method twice ..

[lsmith77]

@makasim ok its getting stranger .. with the following change the issue with Payum goes way:

diff --git a/PayumBundle.php b/PayumBundle.php
index e49ad44..144ffd3 100644
--- a/PayumBundle.php
+++ b/PayumBundle.php
@@ -17,6 +17,11 @@ use Symfony\Component\DependencyInjection\ContainerBuilder;
 
 class PayumBundle extends Bundle
 {
+    public function getContainerExtension()
+    {
+        return new PayumExtension();
+    }
+
     public function build(ContainerBuilder $container)
     {
         parent::build($container);

[lsmith77]

an alternative fix is

diff --git a/DependencyInjection/PayumExtension.php b/DependencyInjection/PayumExtension.php
index f997b6a..2cd4a5b 100644
--- a/DependencyInjection/PayumExtension.php
+++ b/DependencyInjection/PayumExtension.php
@@ -272,7 +272,7 @@ class PayumExtension extends Extension implements PrependExtensionInterface
         if (empty($factoryName)) {
             throw new InvalidArgumentException(sprintf('The storage factory %s has empty name', get_class($factory)));
         }
-        if (array_key_exists($factoryName, $this->storagesFactories)) {
+        if (array_key_exists($factoryName, $this->storagesFactories) && $this->storagesFactories[$factoryName] != $factory) {
             throw new InvalidArgumentException(sprintf('The storage factory with such name %s already registered', $factoryName));
         }
         

[makasim]

@makasim ok its getting stranger .. with the following change the issue with Payum goes way:

@lsmith77 That might be merged. I'd like to know what is going there.

[lsmith77]

@makasim it looks to me like the PayumBundle::build() is called method twice .. now why that is the case I don't know. In that case we end up with that exception.

[makasim]

@lsmith77 Yeah, obviously. I might have a similar case in enqueue. I'll try to debug it there. Hope it is the same issue.

[lsmith77]

finally had some more time to look into this .. the issue is that the config:dump command runs another build() on all bundles .. as such I think PayumBundle::build() needs to be refactored to deal with this.

Again however, the change in this PR should still be applied to Sylius.

[lchrusciel]

Maybe just return $this->containerExtension ?: null;

[lsmith77]

yeah .. I guess that makes things a bit more compact and readable

[lsmith77]

the errors seem unrelated to the change ...

[lsmith77]

rebased .. and green

[lchrusciel]

Thanks Lukas!