Do not hardcode bundles in AppKernel about core-bundle HOT 20 CLOSED

Comment by @tristanlins
Saturday Nov 01, 2014 at 13:27 GMT

if the FrameworkBundle is added to the autoload.json

You did something wrong, if you add "system related mandatory" bundles to your autoload.json.
But this can be solved simply, just check if the autoloaded bundle class is already registered to the kernel.

https://github.com/contao/module-core/blob/feature/symfony2/src/HttpKernel/ContaoKernel.php#L112

    protected function generateBundlesMap(array $hardcodedBundles)
    {
        $hardcodedBundleClasses = array_map(
            function(BundleInterface $bundle) { return get_class($bundle); },
            $hardcodedBundles
        );

        $autoloader = new BundleAutoloader($this->getRootDir(), $this->getEnvironment());
        $bundlesMap = $autoloader->load();

        $bundlesMap = array_filter(
            $bundlesMap,
            function (Config $bundleMap) use ($hardcodedBundleClasses) {
                return !in_array($bundleMap->getClass(), $hardcodedBundleClasses);
            }
        );

        return $bundlesMap;
    }

I have made the proposal to support a src/autoload.json for local / project specific autoload configuration on our meeting. The conclusion was that everyone can manipulate the AppKernel and write local bundles directly into the kernel. If we propose to not put bundles directly into the AppKernel we need to provide a local autoload.json.

BTW: what happen when two bundles define the same autoload bundle in their autoload.json???

from core-bundle.

Comment by @leofeyer
Saturday Nov 01, 2014 at 20:20 GMT

I am with @tristanlins on this.

BTW: what happen when two bundles define the same autoload bundle in their autoload.json???

We should definitely test this and apply array_unique if necessary.

from core-bundle.

Comment by @aschempp
Sunday Nov 02, 2014 at 07:36 GMT

Multiple requirements with the same name should not be a problem, because we use array keys for the bundle names.

To me, the FrameworkBundle is a dependency of the ContaoCoreBundle. I agree we don't need a src/autoload.json, because a developer can add his bundle in AppKernel if there is a custom one. But then he also knows about the dependencies and wether they are already loaded. But an extension developer does not know if you removed FrameworkBundle from the AppKernel.

I can't see any advantage of including it in the kernel, I can only see potential issues.

from core-bundle.

Comment by @leofeyer
Wednesday Nov 05, 2014 at 09:26 GMT

To me, the FrameworkBundle is a dependency of the ContaoCoreBundle.

No, I don't agree. The FrameworkBundle is required to run the application (just like the SensioFrameworkBundle etc.). And any application related bundle should be hardcoded in the AppKernel.

This also applies to any bundle that we are shipping pre-configured in the config.yml file, e.g. Doctrine and SwiftMailer.

from core-bundle.

Comment by @aschempp
Wednesday Nov 05, 2014 at 09:31 GMT

If you follow that logic, it would mean the CoreBundle is also a requirement to run the application... but this can't be included, because the autoloader would choke (with the same issue as I described).

Also, if we add ORM later, would that mean we need to add it to the AppKernel because it will now be an application requirement?

from core-bundle.

Comment by @leofeyer
Wednesday Nov 05, 2014 at 09:33 GMT

Probably. We need to discuss this.

from core-bundle.

Comment by @tristanlins
Wednesday Nov 05, 2014 at 09:49 GMT

FrameworkBundle is a little special, because it provide the distribution setup. Well it's not required to run a distribution, but it provide the basic "framework" to run the distribution.

from core-bundle.

Comment by @leofeyer
Wednesday Nov 05, 2014 at 10:04 GMT

I also tend to think that if the core bundle adds e.g. Doctrine, we should not provide its configuration in the config.yml file. We would otherwise configure bundles from which we don't know if they are enabled at all (e.g. removing the core bundle would also remove Doctrine if it were a core bundle dependency).

from core-bundle.

Comment by @aschempp
Wednesday Nov 05, 2014 at 10:05 GMT

Do you mean the config.yml of the bundle or the app folder?

from core-bundle.

Comment by @leofeyer
Wednesday Nov 05, 2014 at 10:06 GMT

Of the app folder (see #30).

from core-bundle.

Comment by @aschempp
Wednesday Nov 05, 2014 at 10:18 GMT

I think so too. Not sure if there are other options than a prepent bundle extension. It would also correctly mean extensions should add an autoload statement on the CoreBundle, which is correct in my opinion. They should also add the CoreBundle to their composer.json to make sure the Contao version fits.

from core-bundle.

Comment by @tristanlins
Wednesday Nov 05, 2014 at 11:01 GMT

It is possible to prepent the default connection. You can overwrite or extend it in the app/config/config.yml.
Here is an example for the doctrine connection:

@CoreBundle\Resources\config\prepend.yml

doctrine:
    dbal:
        default_connection: default
        connections:
            default:
                driver:   "%database_driver%"
                host:     "%database_host%"
                dbname:   "%database_name%"
                user:     "%database_user%"
                password: "%database_password%"

app/config/config.yml

doctrine:
    dbal:
        connections:
            database2:
                driver:   "%database2_driver%"
                host:     "%database2_host%"
                dbname:   "%database2_name%"
                user:     "%database2_user%"
                password: "%database2_password%"

After normalisation, the result would be:

doctrine:
    dbal:
        default_connection: default
        connections:
            default:
                driver:   "%database_driver%"
                host:     "%database_host%"
                dbname:   "%database_name%"
                user:     "%database_user%"
                password: "%database_password%"
            database2:
                driver:   "%database2_driver%"
                host:     "%database2_host%"
                dbname:   "%database2_name%"
                user:     "%database2_user%"
                password: "%database2_password%"

from core-bundle.

Comment by @tristanlins
Wednesday Nov 05, 2014 at 11:03 GMT

They should also add the CoreBundle to their composer.json to make sure the Contao version fits.

They must define all dependencies and CoreBundle is a dependency for every contao bundle!

from core-bundle.

Comment by @leofeyer
Wednesday Nov 05, 2014 at 11:08 GMT

Well, as I have said before, I would prefer to hardcode the application dependencies in the AppKernel and add their default configuration to the app/config/config.yml file.

What if e.g. five bundles require Doctrine but it is not available application wide? Are all five supposed to create a prepend configuration? Or will they rely on the core bundle? But what if the core bundle removes the Doctrine dependency? It would break the other four bundles.

@aschempp Just as you have said above (but exactly the other way round): I can't see any advantage of not including it in the kernel, I can only see potential issues ;)

from core-bundle.

Comment by @tristanlins
Wednesday Nov 05, 2014 at 11:21 GMT

Personally I prefer to add really basic bundles like database and swiftmailer to the global config. That make it easier to understand for newcomers. And bundle specific options are added to the bundle prepend configuration.

from core-bundle.

Comment by @aschempp
Wednesday Nov 05, 2014 at 11:52 GMT

What if e.g. five bundles require Doctrine but it is not available application wide? Are all five supposed to create a prepend configuration?

There's not really any other solution? If multiple bundles already use ORM now, what do they do?

from core-bundle.

Comment by @leofeyer
Wednesday Nov 05, 2014 at 12:39 GMT

The correct solution is to adjust the bundle autoloader so it does not throw an exception if one of the hardcoded bundles is being re-added by means of autoloading.

from core-bundle.

Comment by @leofeyer
Wednesday Nov 05, 2014 at 14:17 GMT

FIxed in d0900e7.

from core-bundle.

Comment by @tristanlins
Wednesday Nov 05, 2014 at 14:22 GMT

👍

from core-bundle.

Comment by @aschempp
Wednesday Nov 05, 2014 at 14:26 GMT

Looks good, apart from the fact that we are mixing bundle class/file name and $bundle->getName() everywhere… This won't work if someone will include a legacy Contao module (which would be stupid though).

from core-bundle.