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

realityking · 2013-12-22T11:19:54Z

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.

sstok · 2013-12-22T19:04:47Z

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.

stof · 2013-12-23T11:09:34Z

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

realityking · 2013-12-26T21:00:19Z

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.

stof · 2013-12-27T08:25:48Z

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

realityking · 2013-12-27T13:58:12Z

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

fabpot · 2013-12-28T11:42:08Z

src/Symfony/Component/DependencyInjection/Definition.php

@@ -56,6 +57,34 @@ public function __construct($class = null, array $arguments = array())
    }

    /**
+     * Sets a factory
+     *
+     * @param callable $callable A PHP callable


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

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

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

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

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 a Reference or Definition 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.

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

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?

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

realityking · 2013-12-28T13:25:31Z

@fabpot I added the service: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.

realityking · 2014-01-03T12:59:43Z

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

realityking · 2014-01-13T17:57:12Z

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

realityking · 2014-04-13T13:32:55Z

@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 · 2014-04-13T14:14:08Z

UPGRADE-3.0.md

@@ -18,6 +18,11 @@ UPGRADE FROM 2.x to 3.0
   `DebugClassLoader`. The difference is that the constructor now takes a
   loader to wrap.

+### DependencyInjection
+
+ * The methods `setFactoryClass()`, `setFactoryMethod()` and `setFactoryService()`have been removed in favor of `setFactory`.


there should be a space before "have been removed"

and setFactory() (to be consistent?)

Thanks, fixed.

…bles.

csarrazi · 2014-08-12T11:13:52Z

src/Symfony/Component/DependencyInjection/Definition.php

@@ -65,6 +94,7 @@ public function __construct($class = null, array $arguments = array())
     * @return Definition The current instance
     *
     * @api
+     * @deprecated Deprecated since version 2.5, to be removed in 3.0.


since version 2.6

fabpot · 2014-09-24T06:37:54Z

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?

…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 CS bd8531d added a new Syntax to define factories as callables.

realityking · 2014-09-24T12:51:09Z

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

fabpot reviewed Dec 28, 2013
View reviewed changes

wouterj reviewed Apr 13, 2014
View reviewed changes

realityking added 2 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

fabpot mentioned this pull request Sep 24, 2014

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

Merged

fabpot closed this Sep 24, 2014

This was referenced Apr 15, 2015

Fix deprecated Factory definition doctrine/DoctrineMongoDBBundle#291

Merged

Fix deprecated factory definition sonata-project/SonataDoctrineORMAdminBundle#446

Merged

realityking deleted the factory branch April 30, 2016 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

realityking commented Dec 22, 2013

sstok commented Dec 22, 2013

stof commented Dec 23, 2013

realityking commented Dec 26, 2013

stof commented Dec 27, 2013

realityking commented Dec 27, 2013

fabpot Dec 28, 2013

realityking Dec 28, 2013

fabpot Dec 28, 2013

realityking Dec 28, 2013

fabpot Dec 28, 2013

realityking Dec 28, 2013

fabpot Dec 28, 2013

realityking commented Dec 28, 2013

realityking commented Jan 3, 2014

realityking commented Jan 13, 2014

realityking commented Apr 13, 2014

wouterj Apr 13, 2014

wouterj Apr 13, 2014

realityking Apr 13, 2014

csarrazi Aug 12, 2014

fabpot commented Sep 24, 2014

realityking commented Sep 24, 2014

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

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

Conversation

realityking commented Dec 22, 2013

sstok commented Dec 22, 2013

stof commented Dec 23, 2013

realityking commented Dec 26, 2013

stof commented Dec 27, 2013

realityking commented Dec 27, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

realityking commented Dec 28, 2013

realityking commented Jan 3, 2014

realityking commented Jan 13, 2014

realityking commented Apr 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 24, 2014

realityking commented Sep 24, 2014