[DI] Add prototype services for PSR4-based discovery and registration #21289

Merged
merged 1 commit into from Feb 13, 2017

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Jan 14, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR to be done

Talking with @dunglas, we wondered if this could be a good idea, as a more general approach to folder-based service registration as done in DunglasActionBundle.

So here is the implementation.

This allows one to define a set of services as such:

services:
    App\:
        resource: ../src/{Controller,Command} # relative to the current file as usual
        autowire: true # or any other attributes really

This looks for php files in the "src" folder, derivates PSR-4 class names from them, and uses class_exists for final discovery. The resulting services are named after the classes found this way.

The "resource" attribute can be a glob to select only a subset of the classes/files.

This approach has several advantages over DunglasActionBundle:

  • it is resilient to missing parent classes (see test case)
  • it loads classes using the normal code path (the autoloader), thus play well with them (e.g. if one registered a special autoloader).
  • it is more predictable, because it uses discovered paths as the only source of ids/classes to register - vs relying on get_declared_classes, which would make things context sensitive.

Fits well with current initiatives to me.

+ $directory = $this->locator->locate($directory, $this->currentDir, true);
+ $directory = realpath($directory) ?: $directory;
+ if (!$classes = $this->findClasses($psr4Glob, $directory)) {
+ throw new InvalidArgumentException(sprintf('No classes matching "%s" were found in directory "%s".', $psr4Glob, $directory));

This comment has been minimized.

@dunglas

dunglas Jan 14, 2017

Member

Shouldn't we log something instead of throwing? It will allow to register empty directories (useful for starter packs for instance).

@dunglas

dunglas Jan 14, 2017

Member

Shouldn't we log something instead of throwing? It will allow to register empty directories (useful for starter packs for instance).

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

log added

+
+ $psr4Prefix = $match[0];
+ $classRegexp = self::patternToRegexp($psr4Glob);
+ $excludeTest = false === strpos($psr4Glob, 'Test', strlen($psr4Prefix));

This comment has been minimized.

@dunglas

dunglas Jan 14, 2017

Member

Do we really want to hardcode such behaviors? If the user doesn't want to include the Test/ he can already register all subdirectories manually.

@dunglas

dunglas Jan 14, 2017

Member

Do we really want to hardcode such behaviors? If the user doesn't want to include the Test/ he can already register all subdirectories manually.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

I think we do, because it gives a good default: test folder/classes usually contain strange things that can break PHP (look at our own test suite - this is were I needed this default first).

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

I think we do, because it gives a good default: test folder/classes usually contain strange things that can break PHP (look at our own test suite - this is were I needed this default first).

+ }
+ }
+
+ private static function patternToRegexp($pattern)

This comment has been minimized.

@dunglas

dunglas Jan 14, 2017

Member

If the method is private and called only from non-static methods, there is no benefit to make it static.

@dunglas

dunglas Jan 14, 2017

Member

If the method is private and called only from non-static methods, there is no benefit to make it static.

This comment has been minimized.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Jan 14, 2017

Member

👍

Member

dunglas commented Jan 14, 2017

👍

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Jan 14, 2017

Contributor

Does this also support Symfony specific conventions like parameters and the @Bundle notation?

Contributor

sstok commented Jan 14, 2017

Does this also support Symfony specific conventions like parameters and the @Bundle notation?

+ parent::setCurrentDir($dir);
+ }
+
+ public function registerClasses(Definition $prototype, $psr4Glob, $directory)

This comment has been minimized.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Should be protected, right?
I also suggest to make it final or @internal as I don't think it is useful to extend it.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Should be protected, right?
I also suggest to make it final or @internal as I don't think it is useful to extend it.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

Should be usable by people using the PhpFileLoader.

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

Should be usable by people using the PhpFileLoader.

This comment has been minimized.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Ok, good point.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Ok, good point.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

About final, I don't adhere to the trend of adding it everywhere, so no :)

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

About final, I don't adhere to the trend of adding it everywhere, so no :)

This comment has been minimized.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Imo final methods are great because of the incoming 7.0 type hints: it will allow us to upgrade a bigger part of the codebase.
However, if this RFC is voted, it will be less useful.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

Imo final methods are great because of the incoming 7.0 type hints: it will allow us to upgrade a bigger part of the codebase.
However, if this RFC is voted, it will be less useful.

This comment has been minimized.

@sstok

sstok Jan 14, 2017

Contributor

I seriously hope that Parameter Type Widening is not performed when strict-types are used (in the child class file) 😑 this would be a big mistake otherwise.

@sstok

sstok Jan 14, 2017

Contributor

I seriously hope that Parameter Type Widening is not performed when strict-types are used (in the child class file) 😑 this would be a big mistake otherwise.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

@sstok should be yes.

Member

nicolas-grekas commented Jan 14, 2017

@sstok should be yes.

@@ -23,6 +27,7 @@
abstract class FileLoader extends BaseFileLoader
{
protected $container;
+ protected $currentDir;

This comment has been minimized.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

private?

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

private?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

private done

@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

private done

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Jan 14, 2017

Member

I voted 😕, so let's explain some of my thoughts about this feature:

  • To start of: Symfony 3.3 is working at the very edge between the "magic convention-based Laravel" and "explicit and clear Symfony". All added features are both exciting and very scary because of their magic. Yet, I consider all currently merged features to still be explicit enough to overrule the "magic".

  • This PR however is removing quite a lot of explicitness. Previously, whenever someone asks on IRC how to find out which services they can use or how servcies are defined, I suggest them to take a look at the Resources/config directory of a bundle. All service definitions could be found somewhere in there. Just a Ctrl + F for the service ID or classname would give you all information you need.
    With this PR however, all this information will be removed from the service configuration. Symfony will derive this information itself based on some magic globs.

  • From my experience, class discovery in projects is one of the most magic things in live. It's mostly based on conventions and often isn't easy to understand. Also, these conventions are prone to change, which can lead to problems with Symfony versions (see what happend with PHPspec when PSR-4 became a thing).

  • I think the current configuration isn't clear. Defining the psr-4 glob as service ID confused me a lot when looking at this PR. I would rather expect the PSR-4 glob to be define by psr4 and have a special resource or the like option to define the directory. Like:

    services:
        app.controllers:
            psr4: AppBundle\Controller\
            resource: '@AppBundle/Controller/`

    That said, I think it would make more sense to support this in a new loader:

    imports:
        - { resource: ../src/Controller, psr4_prefix: AppBundle\Controller\ }

    This seems more clear to me than including this in the service definition configuration.

  • I don't see much use-cases for this kind of 'include every class as service' thing. Yet, it does make some sense for e.g. automatic twig extension registration.

  • I'm worrying that this feature will lead to structuring your code based on container import conventions. For instance, I expect lots of people to start introducing Service namespaces in their bundles to register all classes in this namespace as services. That would be a very bad practice imo.

  • I'm not sure about excluding Test, I don't know why we should introduce such convention. Nobody should ever define a complete bundle using this method and it makes sense to sometimes include some test helper classes (which are often found in the Test sub-namespace).

Member

wouterj commented Jan 14, 2017

I voted 😕, so let's explain some of my thoughts about this feature:

  • To start of: Symfony 3.3 is working at the very edge between the "magic convention-based Laravel" and "explicit and clear Symfony". All added features are both exciting and very scary because of their magic. Yet, I consider all currently merged features to still be explicit enough to overrule the "magic".

  • This PR however is removing quite a lot of explicitness. Previously, whenever someone asks on IRC how to find out which services they can use or how servcies are defined, I suggest them to take a look at the Resources/config directory of a bundle. All service definitions could be found somewhere in there. Just a Ctrl + F for the service ID or classname would give you all information you need.
    With this PR however, all this information will be removed from the service configuration. Symfony will derive this information itself based on some magic globs.

  • From my experience, class discovery in projects is one of the most magic things in live. It's mostly based on conventions and often isn't easy to understand. Also, these conventions are prone to change, which can lead to problems with Symfony versions (see what happend with PHPspec when PSR-4 became a thing).

  • I think the current configuration isn't clear. Defining the psr-4 glob as service ID confused me a lot when looking at this PR. I would rather expect the PSR-4 glob to be define by psr4 and have a special resource or the like option to define the directory. Like:

    services:
        app.controllers:
            psr4: AppBundle\Controller\
            resource: '@AppBundle/Controller/`

    That said, I think it would make more sense to support this in a new loader:

    imports:
        - { resource: ../src/Controller, psr4_prefix: AppBundle\Controller\ }

    This seems more clear to me than including this in the service definition configuration.

  • I don't see much use-cases for this kind of 'include every class as service' thing. Yet, it does make some sense for e.g. automatic twig extension registration.

  • I'm worrying that this feature will lead to structuring your code based on container import conventions. For instance, I expect lots of people to start introducing Service namespaces in their bundles to register all classes in this namespace as services. That would be a very bad practice imo.

  • I'm not sure about excluding Test, I don't know why we should introduce such convention. Nobody should ever define a complete bundle using this method and it makes sense to sometimes include some test helper classes (which are often found in the Test sub-namespace).

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 14, 2017

Member

Symfony 3.3 is working at the very edge between the "magic convention-based Laravel"

and we have to account for the success of Laravel because of its DXness.
But I don't agree with the word "magic".
Magic means "I didn't ask for and it works".
Black (bad) magic means "some global context leaks in to provide this thing".

Here, it's plain explicit - and plain local.
You ask for something by writting this snippet. You get it.

Symfony will derive this information itself based on some magic globs.

You asked for it. No convention here.

services:
app.controllers:
psr4: AppBundle\Controller
resource: '@AppBundle/Controller/`

that does not work, because what matters is to be able to express a pattern that can be expanded to generate service ids.

imports:
- { resource: ../src/Controller, psr4_prefix: AppBundle\Controller\ }

same here.

I don't see much use-cases for this kind

Ask DunglasActionBundle supporters (221 stars, not bad).

I'm not sure about excluding Test, I don't know why we should introduce such convention. Nobody should ever define a complete bundle using this method

I think it's a good default - but if I can't convince the majority, I'll remove it (but I still think that a good default and that not excluding it will prove soon to lead to more problems than WTFs :) )

and it makes sense to sometimes include some test helper classes (which are often found in the Test sub-namespace).

Sure it does! It's really easy to opt-in "Test" classes.

Last word on your other comments about your feelings:

I do understand them.
I felt the same when autowiring was introduced.
Now?
I love it.
:)

Member

nicolas-grekas commented Jan 14, 2017

Symfony 3.3 is working at the very edge between the "magic convention-based Laravel"

and we have to account for the success of Laravel because of its DXness.
But I don't agree with the word "magic".
Magic means "I didn't ask for and it works".
Black (bad) magic means "some global context leaks in to provide this thing".

Here, it's plain explicit - and plain local.
You ask for something by writting this snippet. You get it.

Symfony will derive this information itself based on some magic globs.

You asked for it. No convention here.

services:
app.controllers:
psr4: AppBundle\Controller
resource: '@AppBundle/Controller/`

that does not work, because what matters is to be able to express a pattern that can be expanded to generate service ids.

imports:
- { resource: ../src/Controller, psr4_prefix: AppBundle\Controller\ }

same here.

I don't see much use-cases for this kind

Ask DunglasActionBundle supporters (221 stars, not bad).

I'm not sure about excluding Test, I don't know why we should introduce such convention. Nobody should ever define a complete bundle using this method

I think it's a good default - but if I can't convince the majority, I'll remove it (but I still think that a good default and that not excluding it will prove soon to lead to more problems than WTFs :) )

and it makes sense to sometimes include some test helper classes (which are often found in the Test sub-namespace).

Sure it does! It's really easy to opt-in "Test" classes.

Last word on your other comments about your feelings:

I do understand them.
I felt the same when autowiring was introduced.
Now?
I love it.
:)

@wouterj

This comment has been minimized.

Show comment
Hide comment
@wouterj

wouterj Jan 14, 2017

Member

@nicolas-grekas please note that I'm 😕 and not 👎. I'm just not sure whether I like this or not, but I just wanted to share my current thoughts.

Member

wouterj commented Jan 14, 2017

@nicolas-grekas please note that I'm 😕 and not 👎. I'm just not sure whether I like this or not, but I just wanted to share my current thoughts.

+ $classes[] = $class;
+ continue;
+ }
+ if (!function_exists('opcache_is_script_cached') || !opcache_is_script_cached($info->getRealPath() ?: $path)) {

This comment has been minimized.

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

wouldn't something rejected by this check, be also rejected by the class_exists below?

@GuilhemN

GuilhemN Jan 14, 2017

Contributor

wouldn't something rejected by this check, be also rejected by the class_exists below?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 16, 2017

Member

having a script in cache does not mean there is a class inside - so that's not the same

@nicolas-grekas

nicolas-grekas Jan 16, 2017

Member

having a script in cache does not mean there is a class inside - so that's not the same

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 15, 2017

Contributor

@wouterj a few thoughts:

This PR however is removing quite a lot of explicitness. Previously, whenever someone asks on IRC how to find out which services they can use or how servcies are defined, I suggest them to take a look at the Resources/config directory of a bundle

That's not true: a service can easily be added via a Compiler pass, and a few classes a service locators which are not declared as services, e.g. Controllers and (console) Commands.

The reason why DunglasApiBundle is popular, is that it allows you to remove those service locators (Controllers & Commands) and make use of dependency injection, without introducing the pain of having to declare them as services (although you do add the necessity to ensure your autowiring bindings are correct).

That said, I think it would make more sense to support this in a new loader:

imports:
    - { resource: ../src/Controller, psr4_prefix: AppBundle\Controller\ }

I must say that it makes more sense to me as well, with psr4_prefix which is optional. If it's already declared in the composer.json I don't see why (from a user perspective) you would need to declare it here as well. It seems more confusing and error prone than anything to me.

Maybe you would need to add a type:

imports:
    - { resource: ../src/Controller, type: autowired-services }
Contributor

theofidry commented Jan 15, 2017

@wouterj a few thoughts:

This PR however is removing quite a lot of explicitness. Previously, whenever someone asks on IRC how to find out which services they can use or how servcies are defined, I suggest them to take a look at the Resources/config directory of a bundle

That's not true: a service can easily be added via a Compiler pass, and a few classes a service locators which are not declared as services, e.g. Controllers and (console) Commands.

The reason why DunglasApiBundle is popular, is that it allows you to remove those service locators (Controllers & Commands) and make use of dependency injection, without introducing the pain of having to declare them as services (although you do add the necessity to ensure your autowiring bindings are correct).

That said, I think it would make more sense to support this in a new loader:

imports:
    - { resource: ../src/Controller, psr4_prefix: AppBundle\Controller\ }

I must say that it makes more sense to me as well, with psr4_prefix which is optional. If it's already declared in the composer.json I don't see why (from a user perspective) you would need to declare it here as well. It seems more confusing and error prone than anything to me.

Maybe you would need to add a type:

imports:
    - { resource: ../src/Controller, type: autowired-services }
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 15, 2017

Member

If it's already declared in the composer.json I don't see why (from a user perspective) you would need to declare it

that would be black magic as defined earlier: some global context leaks in!

type: autowired-services

that is strong coupling with autowiring. This feature is orthogonal: autowiring is not required by this PR at all. The fact that it plays well with autowiring is not an unwanted side effect of course, but things are better decoupled+composed than strongly tightened together.

Member

nicolas-grekas commented Jan 15, 2017

If it's already declared in the composer.json I don't see why (from a user perspective) you would need to declare it

that would be black magic as defined earlier: some global context leaks in!

type: autowired-services

that is strong coupling with autowiring. This feature is orthogonal: autowiring is not required by this PR at all. The fact that it plays well with autowiring is not an unwanted side effect of course, but things are better decoupled+composed than strongly tightened together.

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 15, 2017

Contributor

So the few points lost in the edit:

  • What happens when you come across a file which is not a class: a function, a trait, an abstract class, an interface...
  • The include works with globs is that right? Is it possible to have a kinda granular filtering/include? We had a discussion with @dunglas about it somewhere at some point. For example you may have a class in the directory which should not be declared as a service or something
  • What happens if a class found is already registered as a service? (I would expect the original definition not to be touched)
  • I fall in the case several times where I have to do slightly more than "just" registering the services found in a directory, adding a special tag which the name can be determined from the FQCN. Is there any way to re-use that [the one from your PR] logic to achieve that rather than rolling a kinda duplicate of DunglasApiBundle for this specific task?

A few other things lost but can't remember them...

Contributor

theofidry commented Jan 15, 2017

So the few points lost in the edit:

  • What happens when you come across a file which is not a class: a function, a trait, an abstract class, an interface...
  • The include works with globs is that right? Is it possible to have a kinda granular filtering/include? We had a discussion with @dunglas about it somewhere at some point. For example you may have a class in the directory which should not be declared as a service or something
  • What happens if a class found is already registered as a service? (I would expect the original definition not to be touched)
  • I fall in the case several times where I have to do slightly more than "just" registering the services found in a directory, adding a special tag which the name can be determined from the FQCN. Is there any way to re-use that [the one from your PR] logic to achieve that rather than rolling a kinda duplicate of DunglasApiBundle for this specific task?

A few other things lost but can't remember them...

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 15, 2017

Member

What happens when you come across a file which is not a class: a function, a trait, an abstract class, an interface...

the code uses class_exists, which is selective about classes, so interfaces and traits are excluded. For abstract classes, they would be registered, but of course you'll get an error if you use them. Still, this will prove usefull when getter injection will be merged (because it allows to make them usable when abstract is only about getters).

The include works with globs is that right?

yes, and for consistency with #21270, I'm going to use a real glob call.
but I don't think we should add some "exclude" mechanism: if you happen to register too many classes, that's not an issue. Just don't use them. Or don't use this mechanism. Or, last option, mark these as private and all the non used ones will be removed automatically.

What happens if a class found is already registered as a service?

same logic as today: the last registered ones wins. Which means you should put these auto-registering declarations before the typical ones and you'll get the behavior you describe.

I fall in the case several times where I have to do slightly more than "just" registering the services found in a directory, adding a special tag which the name can be determined from the FQCN.

That's the reason why this needs to be in the "services" section and not in "imports":
the "psr4" attribute is just one amongst all the regular ones, and _default also applies.

eg this works:

services:
    App\Command\:
        psr4: ../src/Command
        tags: [console.command]

this will tag all discovered classes with the console.command tag

DunglasActionBundle provides one additional feature that this PR does not try to implement:
conditional tagging (the bundle allows to declare: every instance of EventSubscriberInterface should have the kernel.event_subscriber tag).
We've some ideas with @dunglas about this, but this is orthogonal to this PR, so may come in a later PR.

Member

nicolas-grekas commented Jan 15, 2017

What happens when you come across a file which is not a class: a function, a trait, an abstract class, an interface...

the code uses class_exists, which is selective about classes, so interfaces and traits are excluded. For abstract classes, they would be registered, but of course you'll get an error if you use them. Still, this will prove usefull when getter injection will be merged (because it allows to make them usable when abstract is only about getters).

The include works with globs is that right?

yes, and for consistency with #21270, I'm going to use a real glob call.
but I don't think we should add some "exclude" mechanism: if you happen to register too many classes, that's not an issue. Just don't use them. Or don't use this mechanism. Or, last option, mark these as private and all the non used ones will be removed automatically.

What happens if a class found is already registered as a service?

same logic as today: the last registered ones wins. Which means you should put these auto-registering declarations before the typical ones and you'll get the behavior you describe.

I fall in the case several times where I have to do slightly more than "just" registering the services found in a directory, adding a special tag which the name can be determined from the FQCN.

That's the reason why this needs to be in the "services" section and not in "imports":
the "psr4" attribute is just one amongst all the regular ones, and _default also applies.

eg this works:

services:
    App\Command\:
        psr4: ../src/Command
        tags: [console.command]

this will tag all discovered classes with the console.command tag

DunglasActionBundle provides one additional feature that this PR does not try to implement:
conditional tagging (the bundle allows to declare: every instance of EventSubscriberInterface should have the kernel.event_subscriber tag).
We've some ideas with @dunglas about this, but this is orthogonal to this PR, so may come in a later PR.

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 15, 2017

Contributor

@nicolas-grekas thanks for the answers.

Overall, I'm in favour of this mechanism: there is parts on my application where loosening the control whilst still using dependency injection over service locators which I prefer. There is however a big caveat which I'm not happy at all with: side effects.

  1. It may not be clear for the typical user what happens when service IDs collides, i.e. which trumps on the other. (I think for this one a mention in the doc is enough)
  2. You may have classes you don't expect to be registered which are. You're answer to this for now is "it's ok, if you don't use it they are going to be removed or it doesn't matter much". Whilst I perfectly understand the approach, IMO this is a big deal and this is by far the thing I hate the most in Laravel. (Further explanation in this below).

To extend on the second point, in Laravel you can try to retrieve any class from the DIC. It may not succeed, but it will try at least, even if the class in question is not meant to be registered in the DIC. Now in Symfony we are adopting a similar thing with this mechanism, more scoped as we are specifying the folders we want to apply this mechanism to, but the problem remains the same: you are registering things that should not be registered.

To this problem, I think we should, at least, proving the tooling necessary for the people how mind it like me to "uncover" this side-effect. For example a command listing the services registered in this way.

In case this seems weird to you I'm giving you the use case I have in mind: I get my hand on a application I'm not familiar with, the glob is hard to understand or I suspect a class is registered when it should not or simply, I want to see if what I expect to be registered actually is, how can I see that easily?

That's the reason why this needs to be in the "services" section and not in "imports":
the "psr4" attribute is just one amongst all the regular ones, and _default also applies.

Fair enough in a very simple case, but say mind the tag name needs a bit of processing to be determined (and is currently done in a Compiler pass which does something a la DunglasApiBundle). To extend a bit on this use case: it seems much more interesting to me, more than this PR to be honest, interesting to provide a way to re-use that logic of "find recursively the classes here, return an array of FQC => serviceDefinition", to which you could hook something to do whatever kind of processing you would like to in a compiler pass. Granted this may not be the scope of this PR, but I think it affects the way this feature is implemented (if we want to provide this kind of hooks).

Contributor

theofidry commented Jan 15, 2017

@nicolas-grekas thanks for the answers.

Overall, I'm in favour of this mechanism: there is parts on my application where loosening the control whilst still using dependency injection over service locators which I prefer. There is however a big caveat which I'm not happy at all with: side effects.

  1. It may not be clear for the typical user what happens when service IDs collides, i.e. which trumps on the other. (I think for this one a mention in the doc is enough)
  2. You may have classes you don't expect to be registered which are. You're answer to this for now is "it's ok, if you don't use it they are going to be removed or it doesn't matter much". Whilst I perfectly understand the approach, IMO this is a big deal and this is by far the thing I hate the most in Laravel. (Further explanation in this below).

To extend on the second point, in Laravel you can try to retrieve any class from the DIC. It may not succeed, but it will try at least, even if the class in question is not meant to be registered in the DIC. Now in Symfony we are adopting a similar thing with this mechanism, more scoped as we are specifying the folders we want to apply this mechanism to, but the problem remains the same: you are registering things that should not be registered.

To this problem, I think we should, at least, proving the tooling necessary for the people how mind it like me to "uncover" this side-effect. For example a command listing the services registered in this way.

In case this seems weird to you I'm giving you the use case I have in mind: I get my hand on a application I'm not familiar with, the glob is hard to understand or I suspect a class is registered when it should not or simply, I want to see if what I expect to be registered actually is, how can I see that easily?

That's the reason why this needs to be in the "services" section and not in "imports":
the "psr4" attribute is just one amongst all the regular ones, and _default also applies.

Fair enough in a very simple case, but say mind the tag name needs a bit of processing to be determined (and is currently done in a Compiler pass which does something a la DunglasApiBundle). To extend a bit on this use case: it seems much more interesting to me, more than this PR to be honest, interesting to provide a way to re-use that logic of "find recursively the classes here, return an array of FQC => serviceDefinition", to which you could hook something to do whatever kind of processing you would like to in a compiler pass. Granted this may not be the scope of this PR, but I think it affects the way this feature is implemented (if we want to provide this kind of hooks).

@theofidry

This comment has been minimized.

Show comment
Hide comment
@theofidry

theofidry Jan 15, 2017

Contributor

@nicolas-grekas I'm asking those questions here as I'm missing a bit the technical insight to be able to judge if this requires to be done upfront or if this can be done gradually, in which case this PR is just a first step. If you think this can be done gradually, I'm in favour to open dedicated issue for this PR and just 👍 this one, your call.

Contributor

theofidry commented Jan 15, 2017

@nicolas-grekas I'm asking those questions here as I'm missing a bit the technical insight to be able to judge if this requires to be done upfront or if this can be done gradually, in which case this PR is just a first step. If you think this can be done gradually, I'm in favour to open dedicated issue for this PR and just 👍 this one, your call.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 16, 2017

Member

It may not be clear for the typical user what happens when service IDs collides

That could also apply to "imports". That's why ordering is the only rule to me - for consistency.

in Laravel you can try to retrieve any class from the DIC

that is #18268 - discuss that there - nothing to do with this PR :)

mind the tag name needs a bit of processing to be determined

exactly the topic I mentionned before: DunglasActionBundle provides conditional tagging - and this PR does not try to do it, because that's something orthogonal - ie that must be done indenpendently, because it is not required for making this PR self consistent.

return an array of FQC => serviceDefinition

same here, that's building on top of this PR, or reusing some of its logic to do something else. Not required to make this PR self consistent.

All in all @theofidry, I'm really happy to see that this PR is raising many ideas in your mind.
Just let's things be built one consistent step at a time. PR after PR, each one self-consistent and useful by itself.

Member

nicolas-grekas commented Jan 16, 2017

It may not be clear for the typical user what happens when service IDs collides

That could also apply to "imports". That's why ordering is the only rule to me - for consistency.

in Laravel you can try to retrieve any class from the DIC

that is #18268 - discuss that there - nothing to do with this PR :)

mind the tag name needs a bit of processing to be determined

exactly the topic I mentionned before: DunglasActionBundle provides conditional tagging - and this PR does not try to do it, because that's something orthogonal - ie that must be done indenpendently, because it is not required for making this PR self consistent.

return an array of FQC => serviceDefinition

same here, that's building on top of this PR, or reusing some of its logic to do something else. Not required to make this PR self consistent.

All in all @theofidry, I'm really happy to see that this PR is raising many ideas in your mind.
Just let's things be built one consistent step at a time. PR after PR, each one self-consistent and useful by itself.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 16, 2017

Member

PR updated, trying to listen to comments:

  • the glob pattern is moved from the "id" side to the "psr4" side, so that we can use the real glob() function
  • given that glob() is powerful enough to include/exclude dirs, I removed the Test special exclusion
  • the "id" side now must be a PSR4 namespace prefix, thus must end with a \ (same convention as composer)
Member

nicolas-grekas commented Jan 16, 2017

PR updated, trying to listen to comments:

  • the glob pattern is moved from the "id" side to the "psr4" side, so that we can use the real glob() function
  • given that glob() is powerful enough to include/exclude dirs, I removed the Test special exclusion
  • the "id" side now must be a PSR4 namespace prefix, thus must end with a \ (same convention as composer)
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Jan 30, 2017

Member

rebased

Member

nicolas-grekas commented Jan 30, 2017

rebased

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 2, 2017

Member

noted as experimental

Member

nicolas-grekas commented Feb 2, 2017

noted as experimental

@@ -69,8 +69,7 @@ public function isFresh($timestamp)
$exists = $exists || class_exists($this->resource, false) || interface_exists($this->resource, false) || trait_exists($this->resource, false);
} elseif (self::EXISTS_KO_WITH_THROWING_AUTOLOADER === $this->existsStatus) {
if (null === self::$throwingAutoloader) {
- $signalingException = new \ReflectionException();
- self::$throwingAutoloader = function () use ($signalingException) { throw $signalingException; };
+ self::$throwingAutoloader = function () { throw new \ReflectionException(); };

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

The signaling exception trick breaks HHVM. Let's drop it, it's a µoptim anyway.

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

The signaling exception trick breaks HHVM. Let's drop it, it's a µoptim anyway.

+ $classes = $this->findClasses($namespace, $directoryGlob, strlen($directoryPrefix));
+
+ foreach ($classes as $class) {
+ $this->container->setDefinition($class, clone $prototype);

This comment has been minimized.

@xabbuh

xabbuh Feb 3, 2017

Member

Shouldn't we update the definition's class attribute?

@xabbuh

xabbuh Feb 3, 2017

Member

Shouldn't we update the definition's class attribute?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

ResolveClassPass will do it perfectly well

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

ResolveClassPass will do it perfectly well

+ continue;
+ }
+ if ($node->hasAttribute('psr4')) {
+ throw new InvalidArgumentException(sprintf('The "psr4" attribute cannot be used with inline services in %s.', $file));

This comment has been minimized.

@xabbuh

xabbuh Feb 3, 2017

Member

Should we add a test for this case?

@xabbuh

xabbuh Feb 3, 2017

Member

Should we add a test for this case?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

test case added

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

test case added

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Feb 3, 2017

Member

I am not sure if it is really helpful to have this feature as it heavily depends on the environment/context, but since it is marked as experimental I am 👍 for merging it and see how it works out in practice.

Member

xabbuh commented Feb 3, 2017

I am not sure if it is really helpful to have this feature as it heavily depends on the environment/context, but since it is marked as experimental I am 👍 for merging it and see how it works out in practice.

+ $classes = $this->findClasses($namespace, $directoryGlob, strlen($directoryPrefix));
+
+ foreach ($classes as $class) {
+ $this->container->setDefinition($class, clone $prototype);

This comment has been minimized.

@stof

stof Feb 3, 2017

Member

Does cloning a Definition behaves properly for other Definition objects (or other kind of objects) nested inside it (in arguments for instance).
If nested objects keep being shared, it would create weird things when compiler passes alter them later (as they will find the same instance for both and so changes will be shared). We need to ensure that we are deep-cloning here as necessary. I'm not sure the value objects of the component deal with it properly today

@stof

stof Feb 3, 2017

Member

Does cloning a Definition behaves properly for other Definition objects (or other kind of objects) nested inside it (in arguments for instance).
If nested objects keep being shared, it would create weird things when compiler passes alter them later (as they will find the same instance for both and so changes will be shared). We need to ensure that we are deep-cloning here as necessary. I'm not sure the value objects of the component deal with it properly today

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

deep clone done

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

deep clone done

+ $classes = array();
+ $extRegexp = defined('HHVM_VERSION') ? '/\\.(?:php|hh)$/' : '/\\.php$/';
+
+ foreach (glob($directoryGlob, defined('GLOB_BRACE') ? GLOB_BRACE : 0) as $path) {

This comment has been minimized.

@stof

stof Feb 3, 2017

Member

The conditional here means that the available features depend on the availability of GLOB_BRACE, which may make the code less portable.
In which cases can it be undefined ?

@stof

stof Feb 3, 2017

Member

The conditional here means that the available features depend on the availability of GLOB_BRACE, which may make the code less portable.
In which cases can it be undefined ?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

From the PHP doc:

Note: The GLOB_BRACE flag is not available on some non GNU systems, like Solaris.

so it's system dependent - and there is no way around - the Finder and all other code do the same when using glob

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

From the PHP doc:

Note: The GLOB_BRACE flag is not available on some non GNU systems, like Solaris.

so it's system dependent - and there is no way around - the Finder and all other code do the same when using glob

+ if (!$r = $this->container->getReflectionClass($class, true)) {
+ continue;
+ }
+ if (!$r->isInterface() && !$r->isTrait()) {

This comment has been minimized.

@stof

stof Feb 3, 2017

Member

we should also skip abstract classes

@stof

stof Feb 3, 2017

Member

we should also skip abstract classes

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

nope: as explained in the description, getter injection works also with abstract classes, and that's desired (prevents writting a method with gargabe in it, and explicitly tells consumers that this needs to be overriden)

@nicolas-grekas

nicolas-grekas Feb 3, 2017

Member

nope: as explained in the description, getter injection works also with abstract classes, and that's desired (prevents writting a method with gargabe in it, and explicitly tells consumers that this needs to be overriden)

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 8, 2017

Member

PR will now handle double-star globs and GLOB_BRACE portability via to #21572.

Member

nicolas-grekas commented Feb 8, 2017

PR will now handle double-star globs and GLOB_BRACE portability via to #21572.

@@ -34,4 +39,101 @@ public function __construct(ContainerBuilder $container, FileLocatorInterface $l
parent::__construct($locator);
}
+
+ /**
+ * @experimental in version 3.3

This comment has been minimized.

@fabpot

fabpot Feb 9, 2017

Member

Can you add a method description?

@fabpot

fabpot Feb 9, 2017

Member

Can you add a method description?

+ }
+
+ $classes = $this->findClasses($namespace, $resourcesGlob);
+ $prototype = serialize($prototype);

This comment has been minimized.

@fabpot

fabpot Feb 9, 2017

Member

I think we need to add a comment like // Prepare for deep cloning.

@fabpot

fabpot Feb 9, 2017

Member

I think we need to add a comment like // Prepare for deep cloning.

+<?xml version="1.0" encoding="utf-8"?>
+<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+ <services>
+ <service id="Symfony\Component\DependencyInjection\Tests\Fixtures\Psr4\" psr4="../Psr4/*" />

This comment has been minimized.

@fabpot

fabpot Feb 9, 2017

Member

<prototype resource="../Psr4/*" namespace="Symfony\Component\DependencyInjection\Tests\Fixtures\Psr4\" />?

@fabpot

fabpot Feb 9, 2017

Member

<prototype resource="../Psr4/*" namespace="Symfony\Component\DependencyInjection\Tests\Fixtures\Psr4\" />?

@nicolas-grekas nicolas-grekas changed the title from [DI] Add "psr4" service attribute for PSR4-based discovery and registration to [DI] Add protype services for PSR4-based discovery and registration Feb 9, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 9, 2017

Member

@fabpot comments addressed

Member

nicolas-grekas commented Feb 9, 2017

@fabpot comments addressed

@nicolas-grekas nicolas-grekas changed the title from [DI] Add protype services for PSR4-based discovery and registration to [DI] Add prototype services for PSR4-based discovery and registration Feb 9, 2017

@@ -659,13 +692,15 @@ private function loadFromExtensions(array $content)
*/
private static function checkDefinition($id, array $definition, $file)
{
+ $keywords = isset($definition['resources']) ? static::$prototypeKeywords : static::$serviceKeywords;

This comment has been minimized.

@stof

stof Feb 9, 2017

Member

private properties must be accessed using self::, not static::. Otherwise, the code breaks when extending the class

@stof

stof Feb 9, 2017

Member

private properties must be accessed using self::, not static::. Otherwise, the code breaks when extending the class

+ return $classes;
+ }
+
+ private function glob($resources, $recursive, &$prefixLen = null)

This comment has been minimized.

@stof

stof Feb 9, 2017

Member

do we ever call it in non-recursive mode ? I see a single usage of this method

@stof

stof Feb 9, 2017

Member

do we ever call it in non-recursive mode ? I see a single usage of this method

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 9, 2017

Member

That's preparing for #21270

@nicolas-grekas

nicolas-grekas Feb 9, 2017

Member

That's preparing for #21270

@fabpot

fabpot approved these changes Feb 12, 2017

@@ -0,0 +1,3 @@
+services:
+ Symfony\Component\DependencyInjection\Tests\Fixtures\Prototype\:
+ resources: ../Prototype

This comment has been minimized.

@fabpot

fabpot Feb 12, 2017

Member

I would have named it resource. A plural seems to indicate that the value is an array, which is not supported here.

@fabpot

fabpot Feb 12, 2017

Member

I would have named it resource. A plural seems to indicate that the value is an array, which is not supported here.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 12, 2017

Member

Tests are broken.

Member

fabpot commented Feb 12, 2017

Tests are broken.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Feb 12, 2017

Member

back to green

Member

nicolas-grekas commented Feb 12, 2017

back to green

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Feb 13, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Feb 13, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 03470b7 into symfony:master Feb 13, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 13, 2017

feature #21289 [DI] Add prototype services for PSR4-based discovery a…
…nd registration (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Add prototype services for PSR4-based discovery and registration

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | to be done

Talking with @dunglas, we wondered if this could be a good idea, as a more general approach to folder-based service registration as done in [DunglasActionBundle](https://github.com/dunglas/DunglasActionBundle/blob/master/DependencyInjection/DunglasActionExtension.php).

So here is the implementation.

This allows one to define a set of services as such:
```yaml
services:
    App\:
        resources: ../src/{Controller,Command} # relative to the current file as usual
        autowire: true # or any other attributes really
```

This looks for php files in the "src" folder, derivates PSR-4 class names from them, and uses `class_exists` for final discovery. The resulting services are named after the classes found this way.

The "resource" attribute can be a glob to select only a subset of the classes/files.

This approach has several advantages over [DunglasActionBundle](https://github.com/dunglas/DunglasActionBundle/blob/master/DependencyInjection/DunglasActionExtension.php):
- it is resilient to missing parent classes (see test case)
- it loads classes using the normal code path (the autoloader), thus play well with them (e.g. if one registered a special autoloader).
- it is more predictable, because it uses discovered paths as the only source of ids/classes to register - vs relying on `get_declared_classes`, which would make things context sensitive.

Fits well with current initiatives to me.

Commits
-------

03470b7 [DI] Add "psr4" service attribute for PSR4-based discovery and registration

@nicolas-grekas nicolas-grekas moved this from In Review to Done in Lower entry bar Feb 13, 2017

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-batch branch Feb 14, 2017

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba Apr 28, 2017

Contributor

Awesome job!

Finally can drop this bundle doing pretty much the same thing: https://github.com/Symplify/AutoServiceRegistration

Contributor

TomasVotruba commented Apr 28, 2017

Awesome job!

Finally can drop this bundle doing pretty much the same thing: https://github.com/Symplify/AutoServiceRegistration

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

fabpot added a commit that referenced this pull request May 14, 2017

feature #22680 [DI] Fixing missing "exclude" functionality from PSR4 …
…loader (weaverryan)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Fixing missing "exclude" functionality from PSR4 loader

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | TODO

When the PSR4 loader was added in #21289, @nicolas-grekas said this:

> given that glob() is powerful enough to include/exclude dirs, I removed the Test special exclusion (source: #21289 (comment))

But, I don't believe that's true! [Glob is all about inclusion, not exclusion](https://en.wikipedia.org/wiki/Glob_(programming)#Syntax) - the maximum you can exclude is a single character. Thus, I've marked this as a bug.

My motivation came from upgrading KnpU to the new 3.3 DI stuff. We have many directories (40) in `src/`, and listing them all one-by-one in `resource:` is crazy - the `resource` line would be ~350 characters long and quite unreadable. What I really want to do is include *everything* and then exclude few directories. I tried to do this with `glob`, but it's not possible.

This PR allows for something like this:

```yml
services:
    _defaults:
        public: false

    # ...
    AppBundle\:
        resource: '../../src/AppBundle/*'
        exclude: '../../src/AppBundle/{AppBundle.php,Entity}'
```

This works *beautifully* in practice. And even if I forget to exclude a directory - since the services are private -  **they're ultimately removed from the container anyways**. In fact, the *only* reason I need to exclude *anything* is because of the new "service argument resolver", which causes entities to not be properly removed from the compiled container.

Thanks!

Commits
-------

7d07f19 Allowing prototype/PSR4 service loading ability to exclude, because glob doesn't support this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment