Resolve issue where Symfony2 collection parameters fail to be injected #119

glennmcewan · 2017-05-31T15:53:40Z

This allows collection parameters defined in Symfony 2 to be injected. Fixes #117.

Currently, they fail to do so as there is a constraint that preg_replace_callback must return a string, but when resolving parameters from Symfony 2's container, you may either get back a string, constant, or collection (see here).

I've swapped to using preg_match which will return a string, and I have changed the method signature of replaceParameters so that it returns the container value of the parameter. Then, if it is not a string, we return it to the caller (to avoid another issue passing it to @escape), otherwise the escape method is still called on it, and then returned.

stof · 2017-05-31T15:59:15Z

src/Behat/Symfony2Extension/Context/Argument/ServiceArgumentResolver.php

@@ -60,10 +60,6 @@ public function resolveArguments(ReflectionClass $classReflection, array $argume
     */
    private function resolveArgument($argument)
    {
-        if (is_array($argument)) {
-            return array_map(array($this, 'resolveArgument'), $argument);


this does not resolve argument recursively for arrays, which look wrong to me, as it does not allow services inside an array anymore

Very strange, I wasn't aware that was in the diff before I committed. I've re-instated that chunk, re-tested my scenarios locally and it's running. Unit tests aren't running on my machine right now so I'm relying on Travis.

stof · 2017-05-31T16:05:33Z

and CI complains about your change

sroze

Can you add a test case as well please?

sroze · 2017-05-31T16:12:55Z

src/Behat/Symfony2Extension/Context/Argument/ServiceArgumentResolver.php

-        return preg_replace_callback('/(?<!%)%([^%]+)%(?!%)/', function($matches) use ($container) {
-            return $container->getParameter($matches[1]);
-        }, $argument);
+        $cleanKey = preg_replace('/(?<!%)%([^%]+)%(?!%)/', '$1', $key);


You could probably use preg_match instead then

Good point, as explained in my comment below, preg_replace worked for some cases, except I needed to check if there was a match before I proceeded. I have updated to use preg_match.

sroze · 2017-05-31T16:13:30Z

src/Behat/Symfony2Extension/Context/Argument/ServiceArgumentResolver.php

     */
-    private function replaceParameters(ContainerInterface $container, $argument)
+    private function replaceParameters(ContainerInterface $container, $key)


As you change the behaviour of this method, can you also change its name? Also, why this name change (key) ?

No reason for changing the key, this PR was not in a ready state, still undergoing work as I was waiting for the test results from the CI process.

Has the behaviour of the method really changed? I don't think it has. I don't see the benefit of changing the name here, as it does still replace parameters, nothing new other than it will handle parameter collections.

sroze · 2017-05-31T16:14:07Z

src/Behat/Symfony2Extension/Context/Argument/ServiceArgumentResolver.php


-        return $this->escape($withParameters);
+        if (!is_string($resolvedParam)) {


You can use the ternary operator in the return instead of a if

I can use the ternary operator, but could you tell me the benefit in this case please?

To be fair, I won't argue about it, just my personal preference by reading the code. You can leave it this way.

Preferences are fine, I have no real preference either way; it was just the smallest issue on this PR at the time. My preference is that it's more readable to have a conditional block rather than a ternary in this case, equally I won't argue the point as I don't have any strong concerns either way.

Now the PR is complete, however, I don't mind changing it if that's the style for this repo.

glennmcewan · 2017-06-01T10:50:50Z

@stof / @sroze, could not get unit tests running yesterday so I left it to Travis to run the tests for me.

They failed as the preg_replace method was replacing all strings, and resolving everything that went through that method from the container. It should have instead only pulled values from the container which preg_replace affected, so I have swapped to preg_match and only pulling values from the container if preg_match caught something.

I've also written and tested a Behat scenario for this, where we have a collection-type parameter registered in Symfony's container. Added a couple of new methods to the testapp's FeatureContext to handle this.

glennmcewan · 2017-06-01T15:46:56Z

Done a bit of thinking about how to tackle this nicely, still don't think it's quite refined enough. The unit tests are now passing, and I've added in new ones to cover cases where container parameters are collections.

I made the decision to first check if the given argument is one entire substitution, and in that case, go directly to the container and request the parameter. That handles the case where an argument wants a collection parameter. If that is not the case, then similar logic as before occurs, exception and Exception is thrown if a collection-type parameter is reference in a partial string substitution, because it makes no sense to put a collection-type parameter in to a string.

The builds, except HHVM (HHVM is no longer supported on Ubuntu Precise. Please consider using Trusty with `dist: trusty`.), are now passing. Any thoughts?

pmalisak · 2017-06-01T16:49:58Z

spec/Behat/Symfony2Extension/Context/Argument/ServiceArgumentResolverSpec.php

@@ -23,12 +23,25 @@ function it_resolves_parameters_starting_and_ending_with_percentage_sign_if_they
        ContainerInterface $container
    ) {
        $container->getParameter('parameter')->willReturn('param_value');
+        $container->hasParameter('parameter')->wilLReturn(true);


Nice spot. Resolved.

sroze · 2017-06-05T17:26:10Z

That's a good one, I'm happy with this. Regarding HHVM, as it's not supported anymore in Symfony 4, you can remove it from the travis.yml file :)

glennmcewan · 2017-06-06T08:17:58Z

@sroze thanks for confirming; I'll remove it in a separate PR as it's not a direct concern for this PR.

sroze · 2017-06-21T18:30:00Z

@stof are you happy with these changes as well?

…s (array arguments). Includes tests.

glennmcewan · 2017-09-07T08:47:42Z

Rebased, just to make the commit log cleaner.

@stof do you mind reviewing please?

Timed out ;-)

sroze · 2017-10-23T09:15:50Z

Travis is blocked on PHP 5.3; will remove the support anyway. Thank you @glennunipro!

stof previously requested changes May 31, 2017

View reviewed changes

sroze suggested changes May 31, 2017

View reviewed changes

pmalisak reviewed Jun 1, 2017

View reviewed changes

glennmcewan mentioned this pull request Jun 6, 2017

Remove HHVM from Travis CI Build Matrix #120

Merged

sroze approved these changes Jun 21, 2017

View reviewed changes

Update ServiceArgumentResolver to handle Symfony Container Collection…

19bb955

…s (array arguments). Includes tests.

sroze merged commit e8ca6b4 into Behat:master Oct 23, 2017

glennmcewan deleted the 117-array-parameters branch October 24, 2017 14:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve issue where Symfony2 collection parameters fail to be injected #119

Resolve issue where Symfony2 collection parameters fail to be injected #119

glennmcewan commented May 31, 2017 •

edited

Loading

stof May 31, 2017

glennmcewan May 31, 2017

stof commented May 31, 2017

sroze left a comment

sroze May 31, 2017

glennmcewan Jun 1, 2017

sroze May 31, 2017

glennmcewan Jun 1, 2017

sroze May 31, 2017

glennmcewan Jun 1, 2017

sroze Jun 5, 2017

glennmcewan Jun 6, 2017

glennmcewan commented Jun 1, 2017

glennmcewan commented Jun 1, 2017

pmalisak Jun 1, 2017

glennmcewan Jun 2, 2017

sroze commented Jun 5, 2017

glennmcewan commented Jun 6, 2017

sroze commented Jun 21, 2017

glennmcewan commented Sep 7, 2017

sroze commented Oct 23, 2017


		return $this->escape($withParameters);
		if (!is_string($resolvedParam)) {

Resolve issue where Symfony2 collection parameters fail to be injected #119

Resolve issue where Symfony2 collection parameters fail to be injected #119

Conversation

glennmcewan commented May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented May 31, 2017

sroze left a 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

glennmcewan commented Jun 1, 2017

glennmcewan commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Jun 5, 2017

glennmcewan commented Jun 6, 2017

sroze commented Jun 21, 2017

glennmcewan commented Sep 7, 2017

sroze commented Oct 23, 2017

glennmcewan commented May 31, 2017 •

edited

Loading