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

Use wildcards in definitions #156

Closed
bgaillard opened this issue Feb 26, 2014 · 23 comments
Closed

Use wildcards in definitions #156

bgaillard opened this issue Feb 26, 2014 · 23 comments
Milestone

Comments

@bgaillard
Copy link
Contributor

Hi, we are currently integrating PHP-DI 4 in one of our project.

Because its a good practice to have interfaces for our business services we have to define a lot of PHP-DI interface => implementation mappings.

For exemple (this sample can also be configured using a PHP-DI configuration array) :

    $container -> set('Application\Service\IAuthorAccountService', \DI\object('Application\Service\Impl\AuthorAccountService'));
    $container -> set('Application\Service\IAuthorNbReadingService', \DI\object('Application\Service\Impl\AuthorNbReadingService'));
    $container -> set('Application\Service\IAuthorPublicationService', \DI\object('Application\Service\Impl\AuthorPublicationService'));
    $container -> set('Application\Service\ILastUpdatedService', \DI\object('Application\Service\Impl\LastUpdatedService'));
    $container -> set('Application\Service\IMailService', \DI\object('Application\Service\Impl\MailService'));
    $container -> set('Application\Service\IPublicationNbReadingService', \DI\object('Application\Service\Impl\PublicationNbReadingService'));
    $container -> set('Application\Service\ISubjectNbReadingService', \DI\object('Application\Service\Impl\SubjectNbReadingService'));

This is correctly working but at the end we will have a lot of mappings even if we have only one instance of our business services in the DI container.

I think a really good improvement for future releases of PHP-DI would be to have an auto-scanning feature to scan classes with the @Injectable annotation. This is exactly what the Spring Framework does.

For example in Spring we can do the following to automatically scan classes using the @Component annotation and create an instance of each component in the DI container at starting :

     <context:component-scan base-package="application.service.impl" />

For this reason interface mappgins is not necessary in Spring, each time a @Component annotation is found on a class Spring instanciate this class and injects the associated object inside its DI Container. Then when a @Resource annotation is found on a class attribute Spring look inside its DI container to find one object which implements the specified interface. If multiple objects implementing the interface are present inside the container Spring fails or forces you to define the name of the object to inject.

In PHP-DI it would be great to have something similar, for example :

$container = $containerBuilder -> buildDevContainer();
$container -> scanPackage('Application\Service\Impl');

This would prevent developers to define lots of interface => implementation mappings in PHP-DI.

Any thoughts about this feature ?

Thanks,

Baptiste

@mnapoli
Copy link
Member

mnapoli commented Feb 26, 2014

Hi, thanks for suggesting this.

Yes I see what you mean, I have the same "problem" (rather inconvenience). Scanning a directory could be a solution, however there are cons:

  • it can be slow in development. In production it will be cached, so there is no problem, but in development scanning directories and parsing all files found for annotations on each request might quickly become slow. Implementing a sort of a cache (reparse only files that have changed) just for this will definitely be overkill, because that means bypassing the normal cache system and setting up a cache in dev (which right now can be avoided).
  • it is limited to annotations

However it still is a good feature to implement. Maybe there could be other solutions, for example:

$container->set('Application\Service\I*Service', \DI\object('Application\Service\Impl\*Service'));

By the way, unrelated, but if you use PHP 5.5 you can simplify your definitions a bit:

$container -> set(IAuthorAccountService::class, \DI\object(AuthorAccountService::class));

@bgaillard
Copy link
Contributor Author

Hi Matthieu and thanks for your response, the solution you propose would be good to declare a directory / namespace scanning.

You're right about the limitations, in Java the scanning is performed only one time when the Web server or application server starts so their is no problem after when HTTP requests are performed.

Perhaps the "solution" would be to say that in PHP-DI the pattern Application\Service\Service\I*Service will only be able to scan one directory and not subdirectories.

An application which is designed carefully should never have lots of files in the same directory so this could be a good solution. To limit problems in applications which have a bad design (too much classes in the same directory) we could have a scanner which logs a warning message when the scanning takes too much time.

In my opinion having a cache in development like the one you described would be a lot of work but if it works it would be a killing feature. In our application we have clear responsabilities in the source code and it would be easy to configure 4 or 5 scanning configurations to declare nearly 80% of our components in the DI container.

I never had a look inside the source code of PHP-DI but after some reflection automatic scanning is perhaps a little trickier than I though first. Each time a class with a @Component annotation is encountered the container should remember all the interfaces implemented by this class. Then when an interface is encountered in an @var attribute declaration the container should be able to look at all those implemented interfaces to pick the instance of the object which has the right class implementing the right interface...

Perhaps this would be problematic for performances ?

By the way, unrelated, but if you use PHP 5.5 you can simplify your definitions a bit

Yes, sadly we cannot upgrade our code to PHP 5.5 because its to risky for us and we do not have the time to do it for now. But we'll probably use a new PHP version in one of our next projet 😃

@ghost
Copy link

ghost commented Jun 1, 2014

Please take into consideration the following pattern
$container->set('Application\Service_Service', \DI\object('Application\Service_ServiceImpl'));

Using "I" in front is an old type hinting approach. In modern IDEs, when you look at your code you don't want to see IService all over.

@mnapoli
Copy link
Member

mnapoli commented Jun 1, 2014

@daniphp What's suggested isn't limited by any naming convention. You just place the * wherever you want, you could as well do:

$container->set('Foo*', \DI\object('Bar*'));

Then FooHello would be mapped to BarHello.

@ghost
Copy link

ghost commented Jun 1, 2014

Ok, that sounds great.
Any real plans to implement this? I hate having to write 100 service mappings by hand.

@mnapoli
Copy link
Member

mnapoli commented Jun 2, 2014

@daniphp I'm working on the 4.2, which for now contains only #162. I could give it a shot, I think it shouldn't be too hard. I'll give it a try this week. If it's indeed feasible, can you help me up and do the documentation (https://github.com/mnapoli/PHP-DI/blob/master/doc/definition.md)?

@ghost
Copy link

ghost commented Jun 2, 2014

I think a simple example in PHP configuration could be enough, if you
manage to implement this I can help.

On Mon, Jun 2, 2014 at 10:40 AM, Matthieu Napoli notifications@github.com
wrote:

@daniphp https://github.com/daniphp I'm working on the 4.2, which for
now contains only #162 #162. I
could give it a shot, I think it shouldn't be too hard. I'll give it a try
this week. If it's indeed feasible, can you help me up and do the
documentation (
https://github.com/mnapoli/PHP-DI/blob/master/doc/definition.md)?


Reply to this email directly or view it on GitHub
#156 (comment).

@mnapoli
Copy link
Member

mnapoli commented Jun 2, 2014

Yep it should be enough! Great I'll let you know.

@mnapoli mnapoli added this to the 4.2 milestone Jun 3, 2014
@mnapoli mnapoli mentioned this issue Jun 3, 2014
Merged
11 tasks
@mnapoli mnapoli changed the title Automatic object definitions using interfaces Use wildcards in definitions Jun 3, 2014
@mnapoli
Copy link
Member

mnapoli commented Jun 3, 2014

Good news, I've managed to implement it (see #165). It allows to use any number of wildcards, so you can match:

My\*Interface -> My\*
will match:
My\FooInterface -> My\Foo

but also:

*\Service\*Interface -> *\ServiceImpl\*Impl
will match:
User\Service\AuthInterface -> User\ServiceImpl\AuthImpl

You can also use the wildcard for definitions that are not about classes, e.g.:

return [
    'foo*' => 'bar', // value definition
    'factory*' => DI\factory(function () { /* ... */ }), // factory definition
];

(but I don't think this will be very useful)

As you can see it's pretty flexible, I think it covers most use cases. @daniphp & @bgaillard please review and tell me if that's fine with you :)

Also @daniphp a PR for the doc is welcome (on the 4.2 branch) if you have time, thanks!

@mnapoli
Copy link
Member

mnapoli commented Jun 3, 2014

Also I forgot to add: I think it hurts a little the performances, because if a class name is not found in the definitions, then it will do a foreach on all the definitions and use a regex on each definition containing a * to see if it matches the pattern. See the code here

However given definitions are cached, it will not be a problem in production. So I think it's OK.

@ghost
Copy link

ghost commented Jun 3, 2014

What do you think if conflicting definitions would throw an exception.

For example:
'foo_' => 'bar'
'fo_' => 'baz'

For a class "foor", the clients might expect "baz" but would get "bar". This might be tricky to debug on large configurations.

@mnapoli
Copy link
Member

mnapoli commented Jun 3, 2014

Right. Right now the first one to match is considered (except if an exact match is found, in that case the exact match prevails).

I would say it's a feature that shouldn't be abused, else there will always be problems I'm afraid.

@mnapoli mnapoli closed this as completed Jun 3, 2014
@bgaillard
Copy link
Contributor Author

👍

Hi @mnapoli & @daniphp, happy to see you've implemented this useful feature. I do not have the time to test it for now but will do it with one of our project when the 4.2 release will be out.

I think conflict definitions detection could be a great feature, but if its too complicated or not a priority for now a warning in the docs could prevent problems.

"the first one to match is considered...".

In my opinion few lines about best practices using the wildcards could also be useful in the docs. For example this could be simple advices like (those are general software design best practices in fact) :

  • Always place your business service interfaces in a clearly identified directory, for example MyProject\Services
  • Always place your business service implementations in a clearly identified directory which is not the same directory where you have your interfaces, for example MyProject\Services\Impl
  • Give your service interfaces coherent naming conventions like XxxService or IXxxService
  • Give your service implementations coherent naming conventions like XxxServiceImpl

I see their are already good advices on the page http://php-di.org/doc/best-practices.html, perhaps this page could be completed.

Finally, as @daniphp indicates in a previous post :

Using "I" in front is an old type hinting approach. In modern IDEs, when you look at your code you don't want to see IService all over.

All developers are not using the same conventions, perhaps my conventions (principaly taken from Java & Spring) are not yours and needs more discussions.

@mnapoli
Copy link
Member

mnapoli commented Jun 4, 2014

Detecting conflict would be (I think) a bit complex and add some overhead. I'm not really against it, but I'll keep it simple for now. It's open for suggestions and pull requests.

I'll expand the documentation to add warnings.

Thanks

@mnapoli
Copy link
Member

mnapoli commented Jun 5, 2014

One last thing: the wildcard currently matches any character. That means that it will match sub-namespaces too, i.e. MyProject\Service\*Interface will match MyProject\Service\SubNamespace\AuthServiceInterface.

Do you think that's OK?

Of course if you map MyProject\Service\*Interface to MyProject\Service\*Impl then MyProject\Service\SubNamespace\AuthServiceInterface will correctly be mapped to MyProject\Service\SubNamespace\AuthServiceImpl, so in the end it should work as expected. But I'm afraid it can cause weird issues that I haven't thought of :/

Do you think it's better to restrict * to match anything except \ or not?

@bgaillard
Copy link
Contributor Author

Hi @mnapoli,

Do you think it's better to restrict * to match anything except \ or not?

In my opinion it would be very hard to predict what mappings would be performed by the framework if we allow the \ characters, especially if their is a very deep hierarchy of namespaces behind the interface or the implementation pattern.

Also, because I consider (I hope I'm right) that placing all the business service interfaces and implementations in clear dedicated and flat directories to be a best practice I would prefer to not match the \ character. Finally I think this is an opportunity to give clues to developers to organize their source code the right way and as you say they are chances this will prevent issues which are hard to identify.

Thanks Matthieu, we're sure the next release will help us produce better code and speed up our devs even more 😃 !

@mnapoli
Copy link
Member

mnapoli commented Jun 5, 2014

Thanks, I agree with you it's best to restrict it so that it doesn't match cross-namespaces, I'll update the code :)

mnapoli added a commit that referenced this issue Jun 6, 2014
@ewinslow
Copy link

@mnapoli Great work on this DI container. Very excited to start using it in our project.

In your example above, you show:

return [
    'foo*' => 'bar', // value definition
    'factory*' => DI\factory(function () { /* ... */ }), // factory definition
];

Did you provide a way for the factory to get the value matched by *? That would be immensely useful to us

@mnapoli
Copy link
Member

mnapoli commented Jul 28, 2014

@ewinslow Sorry for the delay. Thanks :)

To answer your question: no, currently there is no way, could you elaborate on why you would need this feature?

@ewinslow
Copy link

Sure, we have a template system where templates are named based on their
location on the filesystem, so I would need to know the name before knowing
where to find it.

The alternative would be to read the entire filesystem and register each
template with the injector up front.

On Mon, Jul 28, 2014, 12:46 PM Matthieu Napoli notifications@github.com
wrote:

@ewinslow https://github.com/ewinslow Sorry for the delay. Thanks :)

To answer your question: no, currently there is no way, could you
elaborate on why you would need this feature?


Reply to this email directly or view it on GitHub
#156 (comment).

@mnapoli
Copy link
Member

mnapoli commented Jul 31, 2014

@ewinslow sorry I completely missed something obvious!

$definition = $container->getDefinitionManager()->getDefinition('foo*bar');

echo $definition->getName();

That should give you the name of the definition that matched :)

@ewinslow
Copy link

Thanks @mnapoli! I will try that and let you know whether it works for me.

@marcusmendesmovin
Copy link

How I map a same interface for various classes?

return array(
    IAdvertise::class => DI\object(Advertise::class),
    IAdvertise::class => DI\object(Partner::class),
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants