Custom repository service compiler pass #658

magnusnordlander · May 6, 2017

Added a compiler pass to register custom repositories as services, given they only belong only to one entity manager.

In accordance with the upcoming Symfony 3.3 standards, and in order to make it easy to use with autowiring, the service name is the class name of the repository.

Todo

Write some functional tests for the different scenarios in which repositories are registered or not.

GuilhemN · May 6, 2017

DependencyInjection/Compiler/RepositoryAliasPass.php

+                    $definition->setArguments($entities[0]);
+                    $definition->setShared(false);
+
+                    $container->setDefinition($repositoryClass, $definition);


Why not using register?

No reason in particular, just habit. I'm not especially attached to using setDefinition, though, I'll change it.

Koc · May 6, 2017

DependencyInjection/Compiler/RepositoryAliasPass.php

+            }
+
+            /** @var MappingDriver $metadataDriver */
+            $metadataDriver = $container->get($metadataDriverService);


Does getting services from non-compiled container not deprecated?

It's not marked as deprecated in dev-master at least. I'll look in to it, otherwise we can ping someone who knows for sure.

Doesn't seem like it, from what I can find.

Looks like it was reverted symfony/symfony#18728

weaverryan

Should this be configurable? I'd like to not introduce more configuration... and if we use setPublic(false), this feature mostly "stays out of the way": it doesn't really do anything unless you start using it.

There is one autowiring consequence: if you already have some of your repositories registered as services, and you are using autowiring, then these new services will be used for autowiring instead of your existing service (because when the service id matches the type exactly, that wins).

weaverryan · May 7, 2017

DependencyInjection/Compiler/RepositoryAliasPass.php

+                    continue;
+                }
+
+                if (count($entities) === 1) {


So this is saying:

If a user is (for some reason) using the same repository class for multiple entities, do not register the service.

I agree with this obviously. But it could confuse users in some edge cases! We could at least log a compiler message, e.g. https://github.com/symfony/symfony/blob/e5bb8fad3f2078ba4011690b9ecc57c1ada43a6f/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php#L83

Absolutely. I just didn't know that there was a logger facility available in the ContainerBuilder :)

It's because it's NEW!!! :D The profiler has a new tab for "compilation" stuff... which I don't think many users will use, but it DOES contain answers if you have some weird situation. And for bigger nerds, the new tab is super fun :)

weaverryan · May 7, 2017

DependencyInjection/Compiler/RepositoryAliasPass.php

+                    $container->register($repositoryClass, $repositoryClass)
+                        ->setFactory([new Reference('doctrine'), 'getRepository'])
+                        ->setArguments($entities[0])
+                        ->setShared(false)


What about ->setPublic(false)?

This is a bit controversial, but it's consistent with decisions we've made so far in 3.3 (like adding public: false under _defaults by default). It also makes this feature more "lean" - if the user doesn't end up using any of these repositories as a service, they'll be removed from the container.

I did consider this, and obviously you're more in tune with the new upcoming philosophy, but what made me decide to make the services public is the fact that they're not internal to the bundle, they're intended to be exposed as part of the bundle's "service interface".

Does this make sense?

magnusnordlander · May 7, 2017

There is one autowiring consequence: if you already have some of your repositories registered as services, and you are using autowiring, then these new services will be used for autowiring instead of your existing service (because when the service id matches the type exactly, that wins).

Yes... Actually it's even more complicated, because the services registered here uses the manager registry as a factory.

If you:

Have repositories registered as a service
Are already using autowiring
Did not use the class name as the service name
Have somehow made ManagerRegistry::getRepository not work (which isn't uncommon)

Then yes, you're gonna have a problem that you need to resolve by either:

Renaming or aliasing your current repository service to the class name
Implementing a custom repository factory making ManagerRegistry::getRepository work again

magnusnordlander · May 7, 2017

As per OOB conversations with @weaverryan I have now updated this PR with logging, making the services private, and stopping the registration of a repository if there's already a service for it. This is to maintain compatibility in some edge cases with people using autowiring with non-class-name service ids. Since this behaviour is deprecated in 3.3 and dropped in 4.0, the check is only made if the Symfony version is < 4.0.

magnusnordlander · Jun 1, 2017

I've been looking in to writing some functional tests for this, but it's a bit more convoluted to functional test compiler passes than I had initially guessed, and I've been too busy the past couple of weeks to devote any serious time to it.

If anyone has a spare bit of time to help by contributing some function tests for this, I'd be very grateful. Does perhaps @Nyholm have some time to spare?

given they only belong only to one entity manager. In accordance with Symfony 3.3 standards, the service name is the class name of the repository.

…ion if there are already services for a given repository (and we’re not on Sf >=4.0).

magnusnordlander · Jun 1, 2017

Okay. So, funny story: after having talked about this I got to thinking, and I came up with another way to do the tests that I wanted to. It's still pretty convoluted, but nowhere near as bad as the other approach I had in mind. Found a pretty serious bug too while testing.

The tests aren't quite as elegant as one could perhaps wish for, but they seem to get the job done. I'm pretty happy with this PR now.

weaverryan · Jun 2, 2017

Tests/DependencyInjection/AutoregisterRepositoriesTest.php

+        $pass->process($containerBuilder->reveal());
+    }
+
+    protected function prepPropheciesForOneEmAndClassA($containerBuilder, $metadataDriver, $parameterBag)


this could be private

weaverryan · Jun 2, 2017

DependencyInjection/Compiler/RepositoryAliasPass.php

+            $repoConflicts = $this->findConflictingServices($container, $repositoryClass);
+
+            if (count($repoConflicts)) {
+                $rootConflicts[$repositoryClass] = $repoConflicts;


It looks like the $repoConflicts isn't actually used. This could just be set to a boolean (and findConflictingServices could also be renamed and return a boolean).

weaverryan · Jun 2, 2017

Tests/DependencyInjection/AutoregisterRepositoriesTest.php

+use Symfony\Component\HttpKernel\Kernel;
+
+
+class AutoregisterRepositoriesTest extends TestCase


extra line here - and add the license stuff on top (of both files)

weaverryan · Jun 2, 2017

DependencyInjection/Compiler/RepositoryAliasPass.php

+use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\HttpKernel\Kernel;
+
+class RepositoryAliasPass implements CompilerPassInterface


Add some phpdoc here

weaverryan · Jun 2, 2017

DependencyInjection/Compiler/RepositoryAliasPass.php

+        foreach ($entityManagers as $name => $serviceName) {
+            $metadataDriverService = sprintf('doctrine.orm.%s_metadata_driver', $name);
+
+            if (!$container->has($metadataDriverService)) {


This is a private service. However, I've just discussed with Nicolas - this is still legal to do in the ContainerBuilder.

However, is it possible (and does it make sense) to use doctrine.orm.default_entity_manager.metadata_factory? I'm worried about the extra overhead of parsing all the metadata on each rebuild. However, that service might have too many side effects - e.g. caching (and we could be rebuilding the cache from a non-production machine where the cache isn't available). I don't honestly know if it's significant or not - we should probably check that.

It is a private service, but, nota bene, it's a private service from this bundle.

Using doctrine.orm.default_entity_manager.metadata_factory does not seem possible to me. That service depends on the entire entity manager being fully constructed. At the very least, it's not possible with the current extension code structure (because the entity manager is constructed using parent services).

magnusnordlander · Jun 2, 2017

Thanks @weaverryan! Fixed.

xabbuh · Jun 2, 2017

You may want to read #660 where @stof gave some strong reasons against this feature.

magnusnordlander · Jun 3, 2017

Wow. I didn't think about that. That is a huge bummer.

I'm not sure that this should be a blocker though, given that the exact same issue is already present in the bundle with the entity manager. If the entity manager is closed, any service already having been constructed with the entity manager becomes unusable. If we're concerned about closed entity managers, we should be deprecating those services.

I also suppose the issue could be circumvented though by using a proxy that always grabs a new repository from the registry, but that would have performance implications for each call... I'm gonna do some experimentation with that.

stof · Jul 10, 2017

@magnusnordlander no. As of Symfony 3.2, the EntityManager itself can be injected without breaking the resetting feature (due to using a proxy around the actual entity manager)

kimhemsoe · Oct 27, 2017

Closing. Replaced by #719 and #725

GuilhemN reviewed May 6, 2017

View reviewed changes

Koc reviewed May 6, 2017

View reviewed changes

Koc mentioned this pull request May 7, 2017

[DI] Revert "deprecate get() for uncompiled container builders" symfony/symfony#20533

Merged

weaverryan reviewed May 7, 2017

View reviewed changes

magnusnordlander added 5 commits June 2, 2017 00:14

Added a compiler pass to register custom repositories as services,

59de284

given they only belong only to one entity manager. In accordance with Symfony 3.3 standards, the service name is the class name of the repository.

Don’t overwrite existing services

1b8d2a3

Style change

9556520

Updated to make services private, add logging, and to abort registrat…

3a92081

…ion if there are already services for a given repository (and we’re not on Sf >=4.0).

Added some tests, fixed a bug when using multiple EMs.

a7a5067

magnusnordlander force-pushed the repository-services branch from fab0609 to a7a5067 Compare June 1, 2017 22:15

magnusnordlander added 2 commits June 2, 2017 00:18

Fixed regressions and improved tests

d590bd2

Modified skipped test

f253c1c

weaverryan reviewed Jun 2, 2017

View reviewed changes

Review fixes

6d31693

weaverryan mentioned this pull request Oct 20, 2017

Adding a new option to pull your repositories from the service container #719

Closed

kimhemsoe closed this Oct 27, 2017

		use Symfony\Component\HttpKernel\Kernel;


		class AutoregisterRepositoriesTest extends TestCase

Search code, repositories, users, issues, pull requests...

Uh oh!

Custom repository service compiler pass #658

Custom repository service compiler pass #658

Uh oh!

Conversation

magnusnordlander commented May 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Koc May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magnusnordlander commented May 7, 2017

Uh oh!

magnusnordlander commented May 7, 2017

Uh oh!

magnusnordlander commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magnusnordlander commented Jun 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magnusnordlander commented Jun 2, 2017

Uh oh!

xabbuh commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magnusnordlander commented Jun 3, 2017

Uh oh!

stof commented Jul 10, 2017

Uh oh!

kimhemsoe commented Oct 27, 2017

Uh oh!

Uh oh!

magnusnordlander commented May 6, 2017 •

edited

Loading

Koc May 7, 2017 •

edited

Loading

weaverryan left a comment •

edited

Loading

magnusnordlander commented Jun 1, 2017 •

edited

Loading

xabbuh commented Jun 2, 2017 •

edited

Loading