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

Missing use statement in eZ/Publish/Core/MVC/Symfony/Controller/Controller #2476

Merged
merged 1 commit into from Nov 9, 2018

Conversation

volkanakb
Copy link
Contributor

@volkanakb volkanakb commented Nov 8, 2018

Added Missing use Statement for ContainerInterface to \eZ\Publish\Core\MVC\Symfony\Controller\Controller

Copy link
Member

@kmadejski kmadejski left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @volkanakb. Could you please clean up the PR description?

@adamwojs adamwojs changed the title missing use statement in eZ/Publish/Core/MVC/Symfony/Controller/Controller Missing use statement in eZ/Publish/Core/MVC/Symfony/Controller/Controller Nov 9, 2018
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

@kmadejski @adamwojs this clearly lacks some unit test. Can we improve it either now or as a follow up?
For getters I had in mind mocking container->get but I'm not sure if that would uncover this particular bug (otherwise it would be pointless).

Or we could go for bigger improvement for master - injecting services instead of accessing them from the Container, (out of scope here)

@kmadejski
Copy link
Member

@alongosz I think that we should keep this PR simple and merge it as it is, but I totally agree that we should introduce tests for this part of the code base.

Or we could go for bigger improvement for master - injecting services instead of accessing them from the Container, (out of scope here)

I would definitely prefer to move forward this way.

@alongosz alongosz merged commit 26402b8 into ezsystems:6.7 Nov 9, 2018
@alongosz
Copy link
Member

alongosz commented Nov 9, 2018

Merged, thank you @volkanakb

No QA required. @kmadejski could you merge up?

@volkanakb
Copy link
Contributor Author

You are welcome. Thank you all for your efforts

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