Skip to content

add support for Parameter references #29

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bendavies
Copy link

Hi,

There was missing support for use of the Parameter class, i.e creating services like this:

$definition = new Definition(new Parameter('app.service.class'));
$definition->addArgument(new Parameter('app.array'))

I couldn't see a way to put the fixing logic in one place. Possibley a refactor would make it cleaner.

Also I've not been able to really write a test for the array parameter fix, as your ArgumentValidatorTest only tests failure cases, unless you want a non failure test for this particular edge case.

Also, there is no check that the container actually has the parameter requested, so that's another edge case.

Let me know how what you think!

Thanks

@matthiasnoback
Copy link
Owner

Thanks! I'll try to look into this soon.

@matthiasnoback
Copy link
Owner

It seems that the Parameter class has been added in Symfony 2.7. Do you know how this works? For example, does class: %app.class% in Yaml get translated to new Definition(new Parameter('app.class'))? What about '%app.class%%some_other_parameter'? If that doesn't work/isn't allowed, your PR looks good to me :)

To add a test which covers your modification to ArgumentValidator you might also add a functional test, loading some Yaml which results in a definition with a Parameter object.

@xabbuh
Copy link
Contributor

xabbuh commented Feb 19, 2016

@matthiasnoback
Copy link
Owner

@xabbuh Interesting! Didn't know that.

@matthiasnoback
Copy link
Owner

This seems like something that would be useful to tackle. There was one open question from myself ("What about '%app.class%%some_other_parameter'?"). If any of you has time to look into this, please let me know if this is safe to merge. I think it is, but that's intuition ;)

@xabbuh
Copy link
Contributor

xabbuh commented Feb 8, 2017

@matthiasnoback For nested parameters you may want to take a look into the logic triggered by the resolve() method of the ParameterBag class.

@Guite
Copy link

Guite commented Dec 1, 2017

It seems this is required for making this usable with Symfony 3.4.

Sensio\Bundle\DistributionBundle\Composer\ScriptHandler::clearCache
PHP Fatal error:  Uncaught Matthias\SymfonyServiceDefinitionValidator\Exception\InvalidServiceDefinitionsException: Service definition validation errors (11):
- services_resetter: Type-hint "Traversable" requires this argument to be a reference to a service or an inline service definition
- session.storage.native: Argument of type "string" should have been an array
- security.role_hierarchy: Argument of type "string" should have been an array
- doctrine.dbal.connection_factory: Argument of type "string" should have been an array
- doctrine: Argument of type "string" should have been an array

at least for session.storage.native it seems that a parameter is used: https://github.com/symfony/symfony/blob/3.4/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml#L30 which is recognised as a string although it represents an array

@stof
Copy link

stof commented Dec 1, 2017

Parameter actually exists since 2.0.0

@Guite
Copy link

Guite commented Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants