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

Standard @Inject annotation #27

Open
mnapoli opened this issue Nov 14, 2014 · 21 comments
Open

Standard @Inject annotation #27

mnapoli opened this issue Nov 14, 2014 · 21 comments
Labels

Comments

@mnapoli
Copy link
Member

mnapoli commented Nov 14, 2014

Flamewar beginning in 3, 2, 1…

Not all containers support annotations. And that's fine :) But several do, and the bad part is that end users are coupling themselves to a specific container.

This RFC is about standardizing an @Inject annotation (or similar annotation) so that end users can switch between containers supporting annotations.

This RFC is not about encouraging containers to support annotations.

References

Comparison

Package Annotation engine Name Apply to Mandatory parameters Default parameter
Flow ? @Inject Property No ?
JMSDiBundle Doctrine @Inject
@InjectParams
Property
Method
No
No
Service ID
PHP-DI Doctrine @Inject Property, Constructor, Method No Service ID
Ray Doctrine @Inject Constructor, Method No None
Zend Framework Doctrine @Inject Method, ? No ?

I have only focused on @Inject type of annotations. Some packages offer @Service (or similar) annotations to declare and configure services, but this is not consistent enough across packages.

I have also not listed all the parameters (for now). The important thing is that none of them have mandatory parameters, which is good because it means there is a common compatible usage.

We can however see that they are not all usable everywhere, i.e. some packages support annotating properties, some support only annotating methods…

How?

It seems all (unless maybe Flow) containers use Doctrine's annotation system. So we could provide an annotation class like Interop\Container\Annotation\Inject.

I don't think there's a way to have an interface as annotation with Doctrine, so that's why we'll need to push a class and not an interface.

Example:

namespace Interop\Container\Annotation;
/**
 * @Annotation
 */
class Inject
{
}

Usage

Users can use the annotation with the FQN or using import aliases:

use Interop\Container\Annotation as DI;

class Foo
{
    /**
     * @Interop\Container\Annotation\Inject
     */
    public function __construct(/* ... */) { /* ... */ }

    /**
     * @DI\Inject
     */
    public function setBar(/* ... */) { /* ... */ }
}

Questions

  • Which "target" should be supported?
    • properties?
    • methods?
    • both?
  • Should we support an optional name parameter? => Need to evaluate support in every container.
  • Should we support any parameter for the annotation to let the package consuming it retrieve them?
@mnapoli mnapoli added the RFC label Nov 14, 2014
@mnapoli
Copy link
Member Author

mnapoli commented Nov 14, 2014

Pinging some contributors of these projects: @Ocramius, @koriym, @weierophinney, @schmittjoh

@mnapoli
Copy link
Member Author

mnapoli commented Nov 14, 2014

Also @loicfrering might be interested in the discussion, I didn't list LosoLib since it targeted ZF1 IIRC, so I'm not sure the project is maintained anymore.

@Ocramius
Copy link
Member

I completely disagree with annotations used for DI Container configuration, with a particular addition of "raging hate" when used on methods and properties.
I (personally) don't think that standardizing them could be of any use.

@philsturgeon
Copy link

As long as the annotations are labeled as super optional, and “if you’re going to use annotations, then please do them like this, then it should be hard to have a major problem with them being included. 

Regardless of personal opinions on annotations (I don’t like em), they are in popular use and should be mentioned in the standard.

-- 
Phil

On November 14, 2014 at 11:23:53 AM, Marco Pivetta (notifications@github.com) wrote:

I completely disagree with annotations used for DI Container configuration, with a particular addition of "raging hate" when used on methods and properties.
I (personally) don't think that standardizing them could be of any use.


Reply to this email directly or view it on GitHub.

@Ocramius
Copy link
Member

They may be used by different injectors, but they still couple to injector-specific configuration.

The standardization should (imo) be limited to injector public API, not to configuration formats (which would require much much more work, as I've already discussed with @shochdoerfer) and mutable aspects of the injector (setup time).

If we really want to go down the way of standardizing a config format based on annotations, then just import JSR-330, which is much more complete, widely used and the hard research work was already done to write it:

  • @Inject
  • @Named
  • @Provider
  • @Qualifier
  • @Scope
  • @Singleton

I'd still rather prefer having an XSD that can be applied to XML, YAML, JSON and PHP configuration, as it doesn't have the drawbacks of annotations coupling.

@schmittjoh
Copy link

Annotations are most useful (and I love them) for application code.

Application code is coupled to a specific DI container most of the time
already. So, I'm not sure if it's worth to standardize this part, on the
other hand, it does not really hurt either.

The Java annotations might be used for inspiration. However I had a look at
them when implementing the Symfony bundle a while back, and there are some
assumptions where Java differs from PHP (autoloading, lifecycle, scoping)
as such most of these annotations are not as useful in PHP.

On Fri, Nov 14, 2014 at 12:37 PM, Marco Pivetta notifications@github.com
wrote:

They may be used by different injectors, but they still couple to
injector-specific configuration.

The standardization should (imo) be limited to injector public API, not to
configuration formats (which would require much much more work, as I've
already discussed with @shochdoerfer https://github.com/shochdoerfer)
and mutable aspects of the injector (setup time).

If we really want to go down the way of standardizing a config format
based on annotations, then just import JSR-330
https://jcp.org/aboutJava/communityprocess/final/jsr330/index.html,
which is much more complete, widely used and the hard research work was
already done to implement:

I'd still rather prefer having an XSD that can be applied to XML, YAML,
JSON and PHP configuration, as it doesn't have the drawbacks of annotations
coupling.


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

@Ocramius
Copy link
Member

Application code is coupled to a specific DI container most of the time already.

@schmittjoh the point is that their coupling with container-specific aspects (and the assumption that the container will behave in a particular code) makes your code actually coupled with one particular container implementation.

So, unless there is some kind of spec on that side (behavior of a container), I don't see a use-case for a standardized annotation format, as userland code will break when swapping out containers.

there are some assumptions where Java differs from PHP (autoloading, lifecycle, scoping) as such most of these annotations are not as useful in PHP.

I agree on autoloading, less on lifecycle and scoping: some "shared" services may be reset (removed) when the container is re-used in later points of an app's run-time, which is pretty much what lifecycle handling is about. Scoping is something relevant if we consider #20 (should a service be exposed to other containers?)

@schmittjoh
Copy link

Like I said I'm not feeling strongly either way, so happy with whatever
direction this goes.

One remark regarding scoping/lifecycle, as we do seem to refer to different
things. PHP is usually run as single-request application where the
execution environment is not shared between requests. In Java on the other
hand, you fire up a single execution environment and then have some sort of
thread pool where objects are shared between threads. This results in
different lifecycle and scoping issues, and there is only a small overlap.
So, that should be considered when starting to port over annotations.

On Fri, Nov 14, 2014 at 12:57 PM, Marco Pivetta notifications@github.com
wrote:

Application code is coupled to a specific DI container most of the time
already.

@schmittjoh https://github.com/schmittjoh the point is that their
coupling with container-specific aspects (and the assumption that the
container will behave in a particular code) makes your code actually
coupled with one particular container implementation.

So, unless there is some kind of spec on that side (behavior of a
container), I don't see a use-case for a standardized annotation format, as
userland code will break when swapping out containers.

there are some assumptions where Java differs from PHP (autoloading,
lifecycle, scoping) as such most of these annotations are not as useful in
PHP.

I agree on autoloading, less on lifecycle and scoping: some "shared"
services may be reset (removed) when the container is re-used in later
points of an app's run-time, which is pretty much what lifecycle handling
is about. Scoping is something relevant if we consider #20
#20 (should
a service be exposed to other containers?)


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

@Ocramius
Copy link
Member

PHP is usually run as single-request application where the execution environment is not shared between requests

Aware of that difference, but I don't want to exclude lifecycle/scoping, even in PHP: Iong running processes is still something that is not very popular in this language, but even just worker threads are becoming more popular, as well as react-alike-based http servers.

Right now, the approach is to scrap an entire container per response, which is a real waste.

@mnapoli
Copy link
Member Author

mnapoli commented Nov 15, 2014

Let's not discuss wether we like them or not, that will get us nowhere.

Maybe that's my fault because I didn't stated this explicitly: I don't see Container Interop as one big standard (e.g. 1 PSR) but instead as several standards. It's definitely OK for containers to implement some and not all. If that were pushed to the FIG, I would definitely suggest that as a separate PSR.

So the goal is to help users decouple from container implementations when they use annotations.

Use JSR330

The thing is that no PHP container is currently compatible with these, except @Inject. I'd rather go simple and useful for now, it's easy to add new annotations/features, it's hard to remove.

Just like for ContainerInterface, the lowest common denominator would actually be useful.

@koriym
Copy link

koriym commented Nov 15, 2014

@mnapoli Thanks for the listed and the ping.

Let's not discuss wether we like them or not, that will get us nowhere.

Agreed.

Use JSR330

Ray.Di 1.x supports @Inject , @Named and @Scope. 2.x support @Qualifier. ( @PostConstruct) But I agree @Inject only first.

@mackstar
Copy link

Because all of these projects have naturally come up with the same annotation keyword @Inject it is testament to it pointing to a pattern of perhaps what could be considered best-practice.

Promoting and encouraging of this as a standard for annotation based DI containers would be a good step forward.

@philsturgeon
Copy link

Actually thinking about it, it would be brilliant to keep annotations out of this original PSR and put them into another.

Having optional things in one standard, or two standards - one of which can be ignored - is basically the same, but seriously reduces the friction of getting a basic container PSR out there. 
-- 
Phil

On November 15, 2014 at 2:43:49 PM, Richard McIntyre (notifications@github.com) wrote:

Because all of these projects have naturally come up with the same annotation keyword @Inject it is testament to it pointing to a pattern of perhaps what could be considered best-practice.

Promoting and encouraging of this as a standard for annotation based DI containers would be a good step forward.


Reply to this email directly or view it on GitHub.

@Fleshgrinder
Copy link

Maybe as part of the PHPDoc PSR?

Also consider naming it @inject (note the lower-case i), see also phpDocumentor/fig-standards#96.

@cxj
Copy link
Contributor

cxj commented Feb 17, 2016

Annotations are an anti-pattern, especially for DI. Read Tom Butler's blog post at the link for a highly dissected discussion as to why. https://r.je/php-annotations-are-an-abomination.html

@koriym
Copy link

koriym commented Feb 17, 2016

See "good summary of the current annotation status in PHP" by Rafael Dohms.
http://www.slideshare.net/rdohms/annotating-with-annotations-forumphp-2012

@shochdoerfer
Copy link
Contributor

I would not go so far to say that annotations are bad per se. I used to be totally against annotations in the past but my view has changed over the last couple years. In the end it resulted in removing the xml configuration format from our DI container and turn it into an annotation based DI container. I totally agree that spreading annotations throughout the whole code base is not the best idea. Using annotations for a single configuration class like we do with bitexpert/disco is perfectly valid in my opinion ;)

@mackstar
Copy link

@cxj I am familiar with Tom's arguments against annotations. To call them an anti-pattern is quite extreme. Other languages have adopted annotations as part of their core implementations such as Java or attributes in C#. PHP is becoming more and more like these 2 languages and having the extra expressiveness that annotations provide I myself an others agree is a good thing.

Sure over-using any single pattern, or using a pattern inappropriately is not good. But lets not get hung up on several peoples conflicting opinions, in the Java world annotations have been widely accepted way to help many people get out of XML configuration hell. They have also been used with elegance in many very successful projects regardless of programming language.

As @mnapoli says

Let's not discuss wether we like them or not, that will get us nowhere.

Lets get back into the spirit of the conversation which is over standardisation. If you would like to suggest an argument for/against annotations then can we take those back to Reddit and keep this space for constructive discussion over how we can make @Inject annotation less implementation specific?

@Ocramius
Copy link
Member

IMO, annotations are an implementation detail, far from the consumer. I wouldn't try standardizing them, as every container will provide its own way to configure more fine-grained @Inject details anyway.

Also, as part of the project that actually tends to drive the annotations system, I kinda agree with the idea that they are an anti-pattern, as they couple implementation with injector (while DI is about decoupling dependencies).

Exactly like with a service locator there is though a place for them in non-reusable highly-app-specific code.

Standardizing them would lead to usage of annotations in open source projects (reusable code), which would not represent that use-case (specific apps).

@mnapoli
Copy link
Member Author

mnapoli commented Feb 18, 2016

If you don't mind me copy-pasting the first lines of the issue:

This RFC is about standardizing an @Inject annotation (or similar annotation) so that end users can switch between containers supporting annotations.

This RFC is not about encouraging containers to support annotations.

I don't think it makes sense to discuss the point of annotations here, this will lead nowhere (and this is not the topic).

@cxj
Copy link
Contributor

cxj commented Feb 19, 2016

I apologize for going off topic. It was wrong for me to do that.

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

No branches or pull requests

9 participants