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

Translate phpcr lists #177

Merged
merged 1 commit into from
Sep 6, 2017

Conversation

EmmanuelVella
Copy link
Contributor

@EmmanuelVella EmmanuelVella commented Aug 25, 2017

I am targeting this branch, because it's a BC new feature.

Changelog

### Added
- Added phpcr lists translations

Subject

Hi, I just installed this bundle to translate phpcr elements, and I noticed the lists are not correctly translated.

I hope that's the correct way to achieve this !

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

seems sane to me, thanks! just a nitpick on property visibility.


/**
* @author Nicolas Bastien <nbastien.pro@gmail.com>
*/
class TranslatableAdminExtension extends AbstractTranslatableAdminExtension
{
/**
* @var LocaleChooser
*/
protected $localeChooser;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be private to prevent uncontrolled extension - unless there is a use case for it, properties should be private to make refactoring easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done !

@EmmanuelVella
Copy link
Contributor Author

Hi, possible to merge this before the next tag? Thank you !

@greg0ire greg0ire merged commit daa2c33 into sonata-project:2.x Sep 6, 2017
@EmmanuelVella
Copy link
Contributor Author

@greg0ire Thank you !

@EmmanuelVella EmmanuelVella deleted the phpcr-list-translation branch September 6, 2017 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants