Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom repository service compiler pass #658

Closed
wants to merge 8 commits into from

Conversation

Projects
None yet
7 participants
@magnusnordlander
Copy link
Contributor

commented 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.
$definition->setArguments($entities[0]);
$definition->setShared(false);
$container->setDefinition($repositoryClass, $definition);

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN May 6, 2017

Why not using register?

This comment has been minimized.

Copy link
@magnusnordlander

magnusnordlander May 6, 2017

Author Contributor

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

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

This comment has been minimized.

Copy link
@Koc

Koc May 6, 2017

Contributor

Does getting services from non-compiled container not deprecated?

This comment has been minimized.

Copy link
@magnusnordlander

magnusnordlander May 6, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@magnusnordlander

magnusnordlander May 6, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@Koc

Koc May 7, 2017

Contributor

Looks like it was reverted symfony/symfony#18728

@weaverryan
Copy link
Contributor

left a comment

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).

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 7, 2017

Contributor

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

This comment has been minimized.

Copy link
@magnusnordlander

magnusnordlander May 7, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 7, 2017

Contributor

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 :)

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan May 7, 2017

Contributor

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.

This comment has been minimized.

Copy link
@magnusnordlander

magnusnordlander May 7, 2017

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented 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

This comment has been minimized.

Copy link
Contributor Author

commented 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

This comment has been minimized.

Copy link
Contributor Author

commented 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?

magnusnordlander added some commits 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
Symfony 3.3 standards, the service name is the class name of the repository.
Updated to make services private, add logging, and to abort registrat…
…ion if there are already services for a given repository (and we’re not on Sf >=4.0).

@magnusnordlander magnusnordlander force-pushed the fervo:repository-services branch from fab0609 to a7a5067 Jun 1, 2017

magnusnordlander added some commits Jun 1, 2017

@magnusnordlander

This comment has been minimized.

Copy link
Contributor Author

commented 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.

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 2, 2017

Contributor

this could be private

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 2, 2017

Contributor

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).

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 2, 2017

Contributor

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

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 2, 2017

Contributor

Add some phpdoc here

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jun 2, 2017

Contributor

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.

This comment has been minimized.

Copy link
@magnusnordlander

magnusnordlander Jun 2, 2017

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2017

Thanks @weaverryan! Fixed.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

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

@magnusnordlander

This comment has been minimized.

Copy link
Contributor Author

commented 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

This comment has been minimized.

Copy link
Member

commented 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

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

Closing. Replaced by #719 and #725

@kimhemsoe kimhemsoe closed this Oct 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.