Skip to content
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

Add visibility argument to service definition #795

Merged
merged 1 commit into from
Sep 19, 2016

Conversation

cedricziel
Copy link
Collaborator

@cedricziel cedricziel commented Sep 16, 2016

That's a fix for the fix in #793 and equally important.

The service definition missed the visibility argument since I at first added it dynamically (which was fixed in #793 - thx for that!). This patch simply adds the argument to the static definition so the 5th argument can now be set correctly.

Without the patch the container cannot be build and fatals with Symfony 3.2-dev

[Symfony\Component\DependencyInjection\Exception\OutOfBoundsException]                               
  Service "liip_imagine.cache.resolver.flysystem_resolver": The index "4" is not in the range [0, 3].

@cedricziel cedricziel changed the title Add visibility command to service definition Add visibility argument to service definition Sep 16, 2016
@robfrawley
Copy link
Collaborator

robfrawley commented Sep 16, 2016

Looks like Symfony learned to count in the latest development release! Good catch; thanks for the PR!

Copy link
Collaborator

@robfrawley robfrawley left a comment

Choose a reason for hiding this comment

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

LGTM

@robfrawley
Copy link
Collaborator

robfrawley commented Sep 16, 2016

@lsmith77 This is an important PR that fixes an oversight (introduced in my PR: #793) when running symfony/symfony:3.2@dev that resolves a fatal error for those running this bleeding edge Symfony release. It should be promptly merged, IMHO.

@lsmith77 lsmith77 merged commit 0e2937b into liip:master Sep 19, 2016
@cedricziel cedricziel deleted the addVisibilityArgument branch September 20, 2016 07:56
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.

3 participants