symfony-cmf / resource-bundle

Resource Location for CMF documents based on Puli
https://cmf.symfony.com
5 stars 7 forks source link

Use repository factories #18

Closed dantleech closed 8 years ago

dantleech commented 8 years ago

This PR makes the bundle more extensible by introducing the concept of repository factories.

Instead of the service instantaition being hard-coded in the DI extension (and limited to the 3 drivers that we provide) service instantiation is taken care of by a dedicated Factory class for each driver.

These factories are tagged using the cmf_resource.repository_factory tag.

Note that this PR is backwards compatible.

wouterj commented 8 years ago

What about refactoring this to: (this is what I meant originally)

class FilesystemFactory implements RepositoryFactoryInterface
{
    /**
     * {@inheritdoc}
     */
    public function create(ContainerBuilder $container, array $options)
    {
        if (null === $options['base_dir']) {
            throw new \InvalidArgumentException(
                'The filesystem repository type requires the `base_dir` option to be set.'
            );
        }

        $container->setDefinition('cmf_resource.repository.filesystem', new Definition(
            FilesystemRepository::class,
            [$options['base_dir'], $options['symlink']]
        ));
    }

    /**
     * {@inheritdoc}
     */
    public function addConfiguration(NodeDefinition $node)
    {
        $node->children()
                ->scalarNode('base_dir')->defaultNull()->end()
                ->booleanNode('symlink')->defaultTrue()->end()
            ->end();
    }
}
dantleech commented 8 years ago

@WouterJ this is what Sylius does too (in a slightly different context).

So - we would parse the repository configuration build up the required DI definitions using configurator classes (as in your example) then pass the container and service IDs to the Registry?

The only issue I see here is that we cannot register these "DI" configurators with tags, they would need to be added manually and the source of knowledge about them would reside in this bundle, or?

wouterj commented 8 years ago

So - we would parse the repository configuration build up the required DI definitions using configurator classes (as in your example) then pass the container and service IDs to the Registry?

Yeah, or immediately pass Reference instances to the Registry (the drupal guys discovered that this was faster than getting each service ID when needed in the EventDispatcher).

The only issue I see here is that we cannot register these "DI" configurators with tags, they would need to be added manually and the source of knowledge about them would reside in this bundle, or?

The SecurityBundle solves this problem like this: https://github.com/symfony/security-bundle/blob/master/SecurityBundle.php#L42-L57

dantleech commented 8 years ago

The SecurityBundle solves this problem like this

But that couples the implementations to this bundle. My aim with this PR was to allow 3rd-party repositories to be used seamlessly - so that there is no distrinction between the repositories we provide and those which a developer might choose to write/use.

wouterj commented 8 years ago

Unless I'm wrong, any bundle can do ->get('security')->addRepositoryFactory(...) in the Bundle#build() method.

dantleech commented 8 years ago

ah I see.

$extension = $container->getExtension('security');
$extension->addSecurityListenerFactory(new FormLoginFactory());
$extension->addSecurityListenerFactory(new HttpBasicFactory());

Looks good then. Will try it.

dantleech commented 8 years ago

One issue is the configuration:

We do this:

cmf_resource:
    repositories:
        test_repository:
            type: doctrine_phpcr_odm
            basepath: /test

We don't do this:

cmf_resource:
    repositories:
        doctrine_phpcr_odm:
            name: test_repository
            basepath: /test

So we are going to have to parse the configuration before building the configuration (if thats possible) and dynamically build the configuration depending on the configuration ... which sounds implausable.

Otherwise we can switch the configuration format (to something less user friendly imo) or we defer option validation to the create method of the factory.

wouterj commented 8 years ago

Hmm, indeed. Let's not change the configuration and put option validation in create() instead (maybe use OptionsResolver component?).

dantleech commented 8 years ago

Have updated the Registry and fixed everything. If the tests are green this is in a finalizable state. /cc @WouterJ

dantleech commented 8 years ago

Updated to use symfony 2.6 minimum.

wouterj commented 8 years ago

Updated to use symfony 2.6 minimum.

For CMF 2.0, we decided to go for 2.8+ (or maybe 2.7+ if that's possible without introducing any BC layers). 2.6 reached end of maintaince a long time ago, it's not worth supporting it.

dantleech commented 8 years ago

Updated. Also updated to Symfony 2.7 as the minumum .. ?

dantleech commented 8 years ago

@WouterJ feel free to merge, otherwise I will do so later this afternoon.