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 localization for templates and vars, with CakePHP i18n table. #7

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cruamps
Copy link

@cruamps cruamps commented Mar 9, 2016

Notifier now accepts translated templates and vars.

Translated templates can be managed with addI18nTemplate()and getI18nTemplate().

Translated vars are stored into i18n table :
http://book.cakephp.org/3.0/en/orm/behaviors/translate.html#initializing-the-i18n-database-table
Notification table did not change.

Notifications are saved with notifyI18n().
Title and body of translated notification can be retrieved with $entity->getI18n('title', 'en')or $entity->getI18n('body', 'fr').

@bobmulder
Copy link
Contributor

First of all, thank you for the Pull Request! I am glad to see you added tests of your code, but the build failed because of the codestandard. You should fix that ;)

I would like to discuss about the fact that you created all kind of new methods. (with I18n in it). Why did you choose for that, instead of extending the existing methods?

Good work!

@bobmulder bobmulder self-assigned this Mar 12, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.6%) to 79.275% when pulling d2da383 on cruamps:master-i18n into 80bb26c on cakemanager:master.

@cruamps
Copy link
Author

cruamps commented Mar 24, 2016

Thank you @bobmulder
I am a beginner in opensource CakePHP development, so I did not know much about codestandard.
I just fixed that, and I will increase coverage soon.

I saw this kind of methods (with I18n) in some CMS plugins, and I did not want to alter your code for my first pull request.
How would you have done ? Maybe adding a parameter $options = [] including key i18n ?

@bobmulder
Copy link
Contributor

Thats no problem @cruamps. If you have questions, feel free to ask!

Well, when just thinking about how we would like to program, I was thinking about:

$notificationManager->addTemplate('newBlog', [
    'title' => 'New blog by :username',
    'body' => ':username has posted a new blog named :name',
    'lang' => 'en'
]);

The lang key is not required. When it's not given, the template would be 'saved' as default. When using the notifcation (so when the template is used), the code should recognize the prefered language...

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