Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[DI] Fix indexed arguments #22205

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

Closed
wants to merge 1 commit into from
Closed

[DI] Fix indexed arguments #22205

wants to merge 1 commit into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Mar 28, 2017

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

small breakage with 3.2 :)

Foo:
  class: Foo
  arguments: { index_0: first, index_1: 2nd, index_2: third }

is now producing

Invalid key "index_0" found in arguments of method "__construct" for service "Foo": only integer or $named arguments are allowed.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 28, 2017

To me, index_0 style arguments are valid only on child definitions, and should be resolved by ResolveDefinitionTemplatesPass, thus before ResolveNamedArgumentsPass.
Thus I'm not sure this PR applies, or could you please provide more details to "prove" the bug?

@ro0NL
Copy link
Contributor Author

ro0NL commented Mar 28, 2017

Oh sorry.. understand the confusion.. this is about plain & simply;

Foo:
  class: Foo
  arguments: { index_0: first, index_1: 2nd, index_2: third }

:))

edit: so used to new syntax.. but updated the issue.

@ro0NL
Copy link
Contributor Author

ro0NL commented Mar 28, 2017

in general; this pass looks very aggressive by default. Shouldnt it only deal with expected $something args only and skip anything other.

about the strategy, named args overwrite indexed ones and we should probably extraxt the resolved index from index_N for this PR as right now im assuming correct order :) imo. this could be as simply as https://github.com/symfony/symfony/pull/22204/files#diff-ba90e85087b3a48467101a7ee7d21691R53... only change something very specific and dont reorder anything. Both could use a AbstractRecursiveArgumentsPass pass actually..

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 28, 2017

extraxt the resolved index from index_N for this PR as right now im assuming correct order

AFAIK, this has never been the case. Ordering based on index_0 style as in your example would be a BC break. To me, the exception is correct: you need to fix the definition. It can even be misleading, since the index_N order does not apply. Such indexes are only valid in child definitions to me.

Currently, this PR and report is bogus, please provide more details if you think otherwise :)

@ro0NL
Copy link
Contributor Author

ro0NL commented Mar 28, 2017

It's really working in 3.2 :)

services:
    x:
      class: AppBundle\Foo
      arguments: { index_0: first, index_1: 2nd, index_2: third }
Foo {#243 ▼
  +a: "first"
  +b: "2nd"
  +c: "third"
}

edit: the order is preserved indeed

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 28, 2017

That's why it's documented as a bc break yes, one we chose to do, because such configs are bogus to begin with :)

@nicolas-grekas
Copy link
Member

See #22185

@ro0NL
Copy link
Contributor Author

ro0NL commented Mar 28, 2017

Ok my bad. I just did not expected it (should have) related to index_N notation as well.. considering it's special meaning. 👍 Sorry for wasting your time :(

@ro0NL ro0NL closed this Mar 28, 2017
@ro0NL ro0NL deleted the di/index-args branch March 28, 2017 17:45
@nicolas-grekas
Copy link
Member

No pb! A big thank for playing with master and reporting issues, even if this one is not valid :)

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

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.