NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[WIP][DependencyInjection] Add a new Syntax to define factories as callables.#9839

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

realityking wants to merge2 commits intosymfony:masterfromrealityking:factory

Closed

[WIP][DependencyInjection] Add a new Syntax to define factories as callables.#9839

realityking wants to merge2 commits intosymfony:masterfromrealityking:factory

Conversation

Copy link

Contributor

realityking commentedDec 22, 2013

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

This pull requests adds a new syntax to define factories based on the syntax for configurators. This is more flexible than the old syntax (factoryMethod and either of factoryClass or factoryService), as it also allows for functions as factories.

Since the service is now a Reference to a Definition it also allows us to inline factories for a small performance improvement and better encapsulation.

Lastly this prevents a bug where a private factory is simple removed because it's not referenced in the graph.

I did not change any of the existing definitions (there's one use of a factory in FrameworkBundle) or automatically use the new internal representation when parsing YAML or XML definitions because this could introduce subtle B/C issues.

There are still a few things left to do (see list below), I wanted a general thumbs up on the approach and the deprecation before I continue.

Todo:

Write documentation
Update UPGRADE-3.0.md
Add a check to CheckDefinitionValidityPass to validate only one factory is defined.

Copy link

Contributor

sstok commentedDec 22, 2013

just a random thought This will make setting factories easier, and so would also allow Expressions as factory, as it does not make sense to use service of class for this.
And it just might solve the issue were one needs dynamic services (like#9658), but expressions are none static and a service 'should' not change except for prototype and request scope.

But this is just random thought, so nothing concrete.

Copy link

Member

stof commentedDec 23, 2013

the new API seems easier to use. However, the Definition now has 2 different factories stored in it, which can lead to weird state when manipulating it. the deprecated API should be reimplemented on top of the new API IMO, to make it only a BC layer

Copy link

ContributorAuthor

realityking commentedDec 26, 2013

I'm relatively sure this will lead to really weird edge cases. For example, if I set the factory to an object (not a service nor a class), than I will get a result for factoryMethod but neither factoryClass nor factoryService. Not sure everything outside out control can handle that.

Copy link

Member

stof commentedDec 27, 2013

@realityking This case should be forbidden, because it will not be possible to dump the container if you pass an object

Copy link

ContributorAuthor

realityking commentedDec 27, 2013

@stof The dumpers already check for that and throw an exception if they encounter an object. Shouldn't this remain consistent?

fabpot reviewed

Dec 28, 2013

View reviewed changes

src/Symfony/Component/DependencyInjection/Definition.php Outdated

Copy link

Member

fabpotDec 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would it make sense to support strings like 'class_name::method' or a service call like 'service_name.method_name' (both of which being supported elsewhere in the framework)?

Copy link

ContributorAuthor

realitykingDec 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Starting with PHP 5.2.3 (no typo),class_name::method should work just fine as they're valid PHP callable.

As for theservice_name.method_name syntax, I'd have to take a look how complex that'd be to do. Where else is this used?

Copy link

Member

fabpotDec 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@realityking Actually, I made a typo, i meantservice_name:method_name. Support should be relatively easy as this would be converted to something like$container->get('service_name')->method_name()

Copy link

ContributorAuthor

realitykingDec 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Doesn't that create a fair amount ambiquity? What if I have a service and and a class with the same name?

I'll have to change this comment though, since it's not really a PHP callable I expect. It also accepts aReference orDefinition and a method name as an array.

That said, if we allow a plain service name instead of a reference we'll break the inlining and get the weird bug with private services again. The only solution to that would be to do the conversion at parse time - but than I don't know all the services and get back to the problem above.

Copy link

Member

fabpotDec 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Where do see an ambiguity? It's:, not:: for service calls.

Copy link

ContributorAuthor

realitykingDec 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah sorry, didn't look careful enough.

With that information, would it be ok to do this at the parser level and only allow this syntax in YAML and XML but not when directly manipulating the Definition?

Copy link

Member

fabpotDec 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, support is only relevant for the YAML and XML formats.

Copy link

ContributorAuthor

realityking commentedDec 28, 2013

@fabpot I added theservice:method short syntax, but only for YAML. I did not add it for XML because after looking at the code, it just isn't a good fit. Currently the service, method and class are different attributes on the factory element. I wouldn't want to change that either, because that make the syntax different from configurators.

If you think this is still worthwhile with just YAML support, than I'll add the same for configurators in a different pull request to keep them in sync.

Copy link

ContributorAuthor

realityking commentedJan 3, 2014

@fabpot Any feedback on using the short hand syntax only for YAML? I'd like to write the documentation to get this done.

Copy link

ContributorAuthor

realityking commentedJan 13, 2014

@fabpot Ping. I'd like to write the docs to get this done.

Copy link

ContributorAuthor

realityking commentedApr 13, 2014

@fabpot I know the ship has sailed for 2.5, but could you give me some feedback about the short hand syntax? (only for YAML or not at all)

wouterj reviewed

Apr 13, 2014

View reviewed changes

UPGRADE-3.0.md Outdated

Copy link

Member

wouterjApr 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

there should be a space before "have been removed"

Copy link

Member

wouterjApr 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

andsetFactory() (to be consistent?)

Copy link

ContributorAuthor

realitykingApr 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks, fixed.

realityking added2 commits

April 13, 2014 17:21

[DependencyInjection] added a new Syntax to define factories as calla…

369c77b

…bles.

[DependencyInjection] short syntax in YAML for new factory syntax

f50c44e

csarrazi reviewed

Aug 12, 2014

View reviewed changes

src/Symfony/Component/DependencyInjection/Definition.php

Copy link

Contributor

csarraziAug 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

since version 2.6

fabpot mentioned this pull request

Sep 24, 2014

[DependencyInjection] Add a new Syntax to define factories as callables#12008

Merged

Copy link

Member

fabpot commentedSep 24, 2014

Don't know why this one slipped under the radar. I'm going to submit a new PR to fix some small things (mainly CS and updated to current master) -- see#12008.

Thanks@realityking for this great work and sorry again for the delay. Can you submit a PR for the documentation?

fabpot closed this

Sep 24, 2014

fabpot added a commit that referenced this pull request

Sep 24, 2014

feature#12008[DependencyInjection] Add a new Syntax to define facto…

1ebf82f

…ries as callables (realityking, fabpot)This PR was merged into the 2.6-dev branch.Discussion----------[DependencyInjection] Add a new Syntax to define factories as callables| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -From the original PR#9839:"This pull requests adds a new syntax to define factories based on the syntax for configurators. This is more flexible than the old syntax (factoryMethod and either of factoryClass or factoryService), as it also allows for functions as factories.Since the service is now a Reference to a Definition it also allows us to inline factories for a small performance improvement and better encapsulation.Lastly this prevents a bug where a private factory is simple removed because it's not referenced in the graph.I did not change any of the existing definitions (there's one use of a factory in FrameworkBundle) or automatically use the new internal representation when parsing YAML or XML definitions because this could introduce subtle B/C issues."Commits-------187aeee fixed CSbd8531d added a new Syntax to define factories as callables.

Copy link

ContributorAuthor

realityking commentedSep 24, 2014

Awesome, I already wrote this off. I'll see that I get to the docs on the weekend.

This was referencedApr 15, 2015

Fix deprecated Factory definitiondoctrine/DoctrineMongoDBBundle#291

Merged

Fix deprecated factory definitionsonata-project/SonataDoctrineORMAdminBundle#446

Merged

realityking deleted the factory branch

April 30, 2016 09:28

Labels

DependencyInjection

Movatterモバイル変換

Uh oh!

[WIP][DependencyInjection] Add a new Syntax to define factories as callables.#9839

[WIP][DependencyInjection] Add a new Syntax to define factories as callables.#9839

Uh oh!

Conversation

realityking commentedDec 22, 2013

Uh oh!

sstok commentedDec 22, 2013

Uh oh!

stof commentedDec 23, 2013

Uh oh!

realityking commentedDec 26, 2013

Uh oh!

stof commentedDec 27, 2013

Uh oh!

realityking commentedDec 27, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realityking commentedDec 28, 2013

Uh oh!

realityking commentedJan 3, 2014

Uh oh!

realityking commentedJan 13, 2014

Uh oh!

realityking commentedApr 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedSep 24, 2014

Uh oh!

realityking commentedSep 24, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants