Behat / Symfony2Extension

Symfony2 extension for Behat
MIT License
392 stars 106 forks source link

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

Closed glennmcewan closed 7 years ago

glennmcewan commented 7 years ago

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 commented 7 years ago

and CI complains about your change

glennmcewan commented 7 years ago

@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 commented 7 years ago

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?

sroze commented 7 years ago

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 commented 7 years ago

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

sroze commented 7 years ago

@stof are you happy with these changes as well?

glennmcewan commented 7 years ago

Rebased, just to make the commit log cleaner.

@stof do you mind reviewing please?

sroze commented 7 years ago

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