Add event manager as soft dependency to translator #4187
Conversation
|
Awesome, very much needed for creating a web UI for handling translations! 👍 |
|
Naming of events is a bit strange: getTranslatedMessage.missing-translation Why first part is camelCased and second part dash-separated (I'm just asking the question, I always have trouble choosing names for events) (btw you could add class constants too, that would simplify things) |
| $events->setIdentifiers(array( | ||
| __CLASS__, | ||
| get_called_class(), | ||
| 'module_manager', |
There was a problem hiding this comment.
Indeed, whoopS… We need traits :D
|
Agree with @bakura10. Besides these notes, I am good for it. Thanks. |
|
I agree with @bakura10 -- keep a consistent style in the event naming. Either don't prefix the events with the method name, or use camelCasing in the second segment of the event name:
I think with those changes, it's ready. |
|
What abut simply "noMessagesLoaded", "missingTranslation" ? (note: camelCased was used here, so I suggest we stick with that: https://github.com/zendframework/zf2/blob/master/library/Zend/Mvc/ResponseSender/SendResponseEvent.php#L20) |
|
@bakura10 I'm fine with those, too -- I just want to see consistency! :) |
|
I think we shouldn't use the method's name as prefix for event names in the shipped ZF2 events, so IMO @bakura10 suggestion is the best choice. |
| { | ||
| $events->setIdentifiers(array( | ||
| __CLASS__, | ||
| get_called_class(), |
There was a problem hiding this comment.
Does get_called_class() and get_class($this) have the same behavior when called in a non-static method?
There was a problem hiding this comment.
Actually, I remember something about this: http://us2.php.net/manual/en/function.get-called-class.php#94331
@DASPRiD change this to get_class($this), please.
There was a problem hiding this comment.
No! Why? get_called_class() is even faster and has the same result.
There was a problem hiding this comment.
@prolic Read the link -- it has unreliable behavior when used in a non-static method; it was built to work inside a static method, not an instance method. We changed the EventManagerAware trait to use get_class($this) for the same reason.
There was a problem hiding this comment.
Thanks for the hint. Then we have some more bugs in ZF2. I will provide fixes for them tomorrow.
E.g. see: https://github.com/zendframework/zf2/blob/master/library/Zend/Mvc/Controller/AbstractController.php
@wryck7 We have messaged since well before stable that event names should typically be |
| * | ||
| * @var EventManagerInterface | ||
| */ | ||
| protected $events; |
There was a problem hiding this comment.
Doesn't $eventManager make more sense here?
|
Changes applied, ready to merge. |
|
good to merge |
| ) | ||
| ); | ||
| } | ||
|
|
||
| $this->messages[$textDomain][$locale] = null; |
There was a problem hiding this comment.
Question: might we want to change the logic here to allow setting the messages based on the last return from triggering the event? In other words, if $results->last() is an array, we could set the messages...
…r-integration Add event manager as soft dependency to translator Conflicts: library/Zend/I18n/composer.json
- If a string value is returned from the `EVENT_MISSING_TRANSLATION` event, we should use it as the translation. - If a `TextDomain` is returned from the `EVENT_NO_MESSAGES_LOADED` event, we should use it for the text domain.
ghost
assigned …ure/translator-event-manager-integration Add event manager as soft dependency to translator Conflicts: library/Zend/I18n/composer.json
- If a string value is returned from the `EVENT_MISSING_TRANSLATION` event, we should use it as the translation. - If a `TextDomain` is returned from the `EVENT_NO_MESSAGES_LOADED` event, we should use it for the text domain.
This PR allows users to attach to two new events in the translator:
By default, events are disabled, and the translator will not try to create an event manager instance. This makes it a rather nice soft dependency, without having it as requirement.