[DependencyInjection] Introduce a new ParameterResolver #17192

GuilhemN · 2015-12-30T23:47:53Z

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

Actually, to resolve a value we can use ParameterBag::resolveValue() which is accessible in the Container.
This works fine but the problem is when we want to resolve a value at runtime and we have a custom ContainerInterface instance.

So I would like to have a trait or a class which would allow to resolve values without an access to a ParameterBag instance.

You can find an example of use case in FOSRestBundle where we need to resolve values at runtime.
We actually use a trait but I'd prefer to use something from the core.

As suggested by @xabbuh, this PR creates a new ParameterResolver used internally by the ParameterBag.

GuilhemN · 2015-12-30T23:51:21Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php

@@ -264,6 +219,9 @@ public function escapeValue($value)
        return $value;
    }

+    /**
+     * {@inheritdoc}
+     */
    public function unescapeValue($value)


I'm wondering if this function should also be moved in the ParameterResolver.

That's not really related to resolving values, is it?

Yes it is, after resolving a value we must want to unescape it.
An example if I use

$resolver = new ParameterResolver(function() { return 'foobar'; }); $resolver->resolveValue('foo %bar% %%');

I will obtain foo foobar %% which is still escaped.

this looks like an issue with resolveValue, isn't it? I'd expect resolveValue to resolve parameters and escaping in the same run (but without exposing any specific public method for that)

It seems that it was mostly introduced in #4707.
We can still add a new argument to resolveValue() which would specify if the value must be unescaped (and change its default value in 4.0) and deprecate unescapeValue and escapeValue.

@nicolas-grekas I added BC layers in the latest commits to unescape values by default but I'm not sure we should do that in this PR (this makes a lot of changes).
Should I open a new PR?

resolveValue can return different values depending if the ParameterBag is resolved or not...

See this example:

$parameterBag = new ParameterBag(array( 'foo' => '100 %%', )); $parameterBag->resolveValue('%foo% %%'); // will return '100 %% %%' $parameterBag->resolve(); $parameterBag->resolveValue('%foo% %%'); // will return '100% %%'

The only solution I see to fix this inconsistency is to manage the entire unescaping process in resolveValue

GuilhemN · 2015-12-31T13:19:11Z

@xabbuh Do you prefer this solution where the values fetched are resolved in the ParameterBag?

GuilhemN · 2016-01-02T00:18:18Z

My second proposition needs a more complex callback but in case we use the Container instead of a ParameterBag to resolve values, this is more natural (no need to mark the values as resolved).

Taluu · 2016-01-02T02:28:41Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php

+    private function getResolver()
+    {
+        if (null === $this->resolver) {
+            $self = $this;


no need for that as this is targetting master (which requires php 5.5)

You're right I didn't know that this changed in php 5.5.

it actually chnaged in php 5.4 :}

Taluu · 2016-01-02T16:05:29Z

src/Symfony/Component/DependencyInjection/ParameterBag/FrozenParameterBag.php

    {
        $this->parameters = $parameters;
        $this->resolved = true;
+        $this->resolver = $resolver;


Isn't the resolver useless here in the frozen bag ? Shoudn't everything already being resolved ?

FrozenParameterBag::resolveValue() can still be used.

GuilhemN · 2016-01-02T23:52:45Z

I'm wondering if instead of using a ParameterResolver in the ParameterBag it should not just extend the ParameterResolver class

Taluu · 2016-01-02T23:54:59Z

No, you wouldn't be able to change the resolver then (and thus the point of such a class would be next to none). But it could be defined as a trait

unkind · 2016-01-04T19:39:03Z

Generic names like ParameterResolver force many devs to depend on a class instead of an interface. If class has 1-to-1 interface, it shouldn't have generic name. It becomes more clear when you don't use the Interface suffix: http://verraes.net/2013/09/sensible-interfaces/

GuilhemN · 2016-01-04T20:07:28Z

@unkind you're right, makes even more sense now that there is a trait... But are you sure CallableParameterResolver is a good name? Maybe an english native could help us?

GuilhemN · 2016-01-04T20:19:28Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterResolverTrait.php

+     *
+     * @throws ParameterNotFoundException if the parameter is not defined
+     */
+    abstract protected function getParameter($name, $resolving = array());


Not sure if this function should be named getParameter or something else to avoid conflicts...

I don't like having an abstract method in the trait. What about adding a private setParameterBag() method instead that must be called by the consuming class? This method could populate an internal ParameterBag property.

I'd like it to work on top of a any container (and a container may not use ParameterBag). I didn't find anything better to solve this abstraction issue.

GuilhemN · 2016-01-08T21:14:05Z

Other reviews?

nicolas-grekas · 2016-01-13T09:55:45Z

src/Symfony/Component/DependencyInjection/Tests/ParameterBag/ParameterBagTest.php

@@ -227,7 +159,7 @@ public function testEscapeValue()
    /**
     * @dataProvider stringsWithSpacesProvider
     */
-    public function testResolveStringWithSpacesReturnsString($expected, $test, $description)
+    public function testLegacyResolveStringWithSpacesReturnsString($expected, $test, $description)


please add the @group legacy annotation

GuilhemN · 2016-01-13T16:31:00Z

The build failure doesn't seem related...

GuilhemN · 2016-01-14T18:11:31Z

In fact it is... I'll try to fix them

GuilhemN · 2016-01-15T19:46:42Z

I reverted the last commits because this is a big subject and they just make this PR bigger than it has to be...
So the tests are fixed again.

TomasVotruba · 2016-03-03T01:49:24Z

@Ener-Getick Very nice PR! Thanks for great job

GuilhemN · 2016-08-10T19:53:45Z

Deprecation message updated to 3.2.
I would love to see that merged ! It would avoid me having to copy-paste this trait...

xabbuh · 2016-10-21T06:50:21Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterResolverTrait.php

+     * @throws ParameterCircularReferenceException if a circular reference is detected
+     * @throws RuntimeException                    when a given parameter has a type problem.
+     */
+    public function resolveValue($value, array $resolving = array())


Imo the public interface should not leak the $resolving argument (it's an implementation detail). We could add a private method that does the actual resolving an let resolveValue() call it with an empty array as the second argument.

And by the way, the ParameterResolverInterface even doesn't require this second argument.

It's fine for me.

Besides that, I'm wondering if we shouldn't make this method private by default: in userland it is unlikely to be used outside the class using the trait.

Edit: I agree, but we can't do that for bc reasons, see our previous conversation

xabbuh · 2016-10-21T06:51:09Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterResolverTrait.php

+/**
+ * Implementation of {@link ParameterResolverInterface}.
+ *
+ * @author Guilhem N. <egetick@gmail.com>


You should add the author(s) of the ParameterBag class too (the main logic was only slightly touched).

xabbuh · 2016-10-21T06:52:32Z

src/Symfony/Component/DependencyInjection/ParameterBag/ParameterResolverInterface.php

+ *
+ * @author Guilhem N. <egetick@gmail.com>
+ */
+interface ParameterResolverInterface


Do we really need an extra interface for this?

I was thinking using it for other classes than ParameterBag but indeed we can remove it unless someone explicitly requests it.

nicolas-grekas · 2016-11-05T09:48:49Z

Shouldn't this one be closed in favor of #20276?

GuilhemN · 2016-11-11T08:35:30Z

@nicolas-grekas the goal is not the same. I'd like to use this to resolve values at runtime (in annotations for example) without copying a large portion of code like what we did in FOSRestBundle.

xabbuh · 2016-12-29T16:31:14Z

I agree with @GuilhemN and I would like to see this in 3.3 too.

nicolas-grekas · 2017-09-26T12:13:16Z

@xabbuh would you like to take over this PR? I personally don't get its point but I think that's just because I'm not in this part of the business :)

xabbuh · 2017-09-26T20:01:27Z

I'll try to do that by the end of the week. If someone wants to take over before, just feel free to do so.

nicolas-grekas · 2017-12-04T08:31:03Z

Could this be solved differently with #25288?

GuilhemN · 2017-12-06T06:24:48Z

@nicolas-grekas it looks like it does indeed, 👍 for your approach :)

nicolas-grekas · 2017-12-06T10:13:41Z

Cool, closing then. Cheers.

carsonbot added the Status: Needs Review label Dec 30, 2015

GuilhemN mentioned this pull request Dec 30, 2015

[DependencyInjection] Create a new trait or class to resolve values with the container #17160

Closed

GuilhemN reviewed Dec 30, 2015
View reviewed changes

Taluu reviewed Jan 2, 2016
View reviewed changes

GuilhemN force-pushed the PARAMETERRESOLVER branch from a06941d to 4623434 Compare January 2, 2016 13:26

Taluu reviewed Jan 2, 2016
View reviewed changes

GuilhemN reviewed Jan 4, 2016
View reviewed changes

nicolas-grekas reviewed Jan 13, 2016
View reviewed changes

GuilhemN force-pushed the PARAMETERRESOLVER branch from 5082a50 to a5c5247 Compare January 13, 2016 16:11

GuilhemN force-pushed the PARAMETERRESOLVER branch from a5c5247 to 126f934 Compare January 14, 2016 17:37

javiereguiluz added DependencyInjection Feature labels Mar 3, 2016

GuilhemN force-pushed the PARAMETERRESOLVER branch 2 times, most recently from dbe15d2 to 3a65c98 Compare August 10, 2016 19:50

GuilhemN force-pushed the PARAMETERRESOLVER branch from 3a65c98 to 8c0875c Compare August 11, 2016 10:41

xabbuh reviewed Oct 21, 2016

View reviewed changes

GuilhemN force-pushed the PARAMETERRESOLVER branch 2 times, most recently from 91e6f1a to 83f3954 Compare October 26, 2016 21:04

[DependencyInjection] Introduce a new ParameterResolver

2ff482a

GuilhemN force-pushed the PARAMETERRESOLVER branch from 83f3954 to 2ff482a Compare October 26, 2016 21:18

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

xabbuh self-assigned this Oct 11, 2017

nicolas-grekas closed this Dec 6, 2017

GuilhemN deleted the PARAMETERRESOLVER branch March 17, 2019 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Introduce a new ParameterResolver #17192

[DependencyInjection] Introduce a new ParameterResolver #17192

[DependencyInjection] Introduce a new ParameterResolver #17192

[DependencyInjection] Introduce a new ParameterResolver #17192

Conversation

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment