Skip to content

Adding support to bind scalar values to controller arguments #26658

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

Merged
merged 1 commit into from
Mar 25, 2018

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Mar 23, 2018

Q A
Branch? master
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25960
License MIT
Doc PR symfony/symfony-docs#9477

Hi guys!

This fixes (I think) the last rough edge with autowiring & error messages. 100% credit to @nicolas-grekas for this implementation - he has generously allowed me to steal his code in return for writing the test. I did test this on a real project.

Cheers!

@javiereguiluz javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Mar 23, 2018
@weaverryan
Copy link
Member Author

Failure is unrelated - unrelated segfault on a different component.

@fabpot
Copy link
Member

fabpot commented Mar 25, 2018

Thank you @weaverryan.

@fabpot fabpot merged commit 2c7198c into symfony:master Mar 25, 2018
fabpot added a commit that referenced this pull request Mar 25, 2018
…uments (weaverryan)

This PR was merged into the 4.1-dev branch.

Discussion
----------

Adding support to bind scalar values to controller arguments

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25960
| License       | MIT
| Doc PR        | symfony/symfony-docs#9477

Hi guys!

This fixes (I think) the last rough edge with autowiring & error messages. 100% credit to @nicolas-grekas for this implementation - he has generously allowed me to steal his code in return for writing the test. I did test this on a real project.

Cheers!

Commits
-------

2c7198c Adding support to bind scalar values to controller arguments
@weaverryan weaverryan deleted the bind-controller-scalars branch March 25, 2018 17:24
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Mar 25, 2018
…s (weaverryan)

This PR was squashed before being merged into the master branch (closes #9477).

Discussion
----------

Removing warning about bind + controller + scalar values

Docs for symfony/symfony#26658 :)

Commits
-------

af00dbc adding version note about scalar bind args
4312518 Removing warning about bind + controller + scalar values
$args[$p->name] = new Reference('value.'.$container->hash($bindingValue));
$container->register((string) $args[$p->name], 'mixed')
->setFactory('current')
->addArgument(array($bindingValue));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this service is here to build a scalar, I should we should mark it as non-shared, to avoiding registering the scalar in $this->services

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? what's the issue with sharing a scalar (or array btw?)
looks better shared to me, did I miss something?


if (!$bindingValue instanceof Reference) {
continue;
$args[$p->name] = new Reference('value.'.$container->hash($bindingValue));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use _value as prefix, to reduce the likeliness of conflicts with an actual service, as the YAML format forbids using the underscore prefix in service ids since 4.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in ba05588

@fabpot
Copy link
Member

fabpot commented Mar 27, 2018

Thinking about this one, how to you differentiate between something coming from the routing (/blog/post_id) and something from the container (same post_id)?

@javiereguiluz
Copy link
Member

If I remember this correctly, the difference is the priority of the resolvers ... this one that resolves container params has low priority (lower than the routing one, which always wins over this one).

@fabpot fabpot mentioned this pull request May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants