Skip to content

Conversation

@TheDevilOnLine
Copy link
Contributor

@TheDevilOnLine TheDevilOnLine commented Jan 11, 2018

The php_translation.edit_in_place.activator service should be public to make the instructions from http://php-translation.readthedocs.io/en/latest/symfony/edit-in-place.html work

The php_translation.edit_in_place.activator service should be public to make the instructions from http://php-translation.readthedocs.io/en/latest/symfony/edit-in-place.html work
@Nyholm Nyholm self-requested a review January 11, 2018 15:11
@bocharsky-bw
Copy link
Member

👎 Hm, I'm against to make it public. I'd prefer to tweak the docs and explain how to inject this service into controllers. Also, we can add a note with an example how to make a public alias to this service if devs on old Symfony versions need it.

@rvanlaak
Copy link
Member

@bocharsky-bw Although we probably agree with you, having it public is a DX consequence of services being accessible in userland controllers that extend the FrameworkBundle controller class. Being able to call $this->get('php_translation.edit_in_place.activator') in the controller makes adoption for starting developers way easier ;-)

You could also see this not being public as a BC break, as this bundle also supports Symfony <4. So, until we decide that it should become private, explicitly adding public: true is the right way to go.

@bocharsky-bw
Copy link
Member

Makes sense, I'm fine to make it public then 👍

@Nyholm
Copy link
Member

Nyholm commented Jan 14, 2018

Thank you for the reviews.

@Nyholm Nyholm merged commit cb2703e into php-translation:master Jan 14, 2018
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.

4 participants