Add event manager as soft dependency to translator #4187

Merged
merged 3 commits into from Apr 29, 2013

Conversation

Projects
None yet
6 participants
Member

DASPRiD commented Apr 5, 2013

This PR allows users to attach to two new events in the translator:

  • loadMessages.no-messages-loaded which will always fire when messages for a specific locale/text-domain combination could not be loaded
  • getTranslatedMessage.missing-translation which will always fire when a message in a specific locale/text-domain could not be found

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.

Contributor

macnibblet commented Apr 5, 2013

Awesome, very much needed for creating a web UI for handling translations! 👍

Contributor

bakura10 commented Apr 6, 2013

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)

@prolic prolic and 1 other commented on an outdated diff Apr 9, 2013

library/Zend/I18n/Translator/Translator.php
+
+ return $this->events;
+ }
+
+ /**
+ * Set the event manager instance used by this translator.
+ *
+ * @param EventManagerInterface $events
+ * @return Translator
+ */
+ public function setEventManager(EventManagerInterface $events)
+ {
+ $events->setIdentifiers(array(
+ __CLASS__,
+ get_called_class(),
+ 'module_manager',
@prolic

prolic Apr 9, 2013

Contributor

Ups :)

@DASPRiD

DASPRiD Apr 9, 2013

Member

Indeed, whoopS… We need traits :D

Contributor

prolic commented Apr 9, 2013

Agree with @bakura10.
Please add a class constant with the event name(s).
I saw you accidentally copied "module_manager" as event identifier, please remove that.

Besides these notes, I am good for it. Thanks.

Owner

weierophinney commented Apr 12, 2013

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:

  • loadMessages.no-messages-loaded becomes either no-messages-loaded or loadMessages.noMessagesLoaded
  • getTranslatedMessage.missing-translation becomes either missing-translation or getTranslatedMessage.missingTranslation

I think with those changes, it's ready.

Contributor

bakura10 commented Apr 12, 2013

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)

Owner

weierophinney commented Apr 12, 2013

@bakura10 I'm fine with those, too -- I just want to see consistency! :)

Contributor

iquabius commented Apr 13, 2013

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.

@iquabius iquabius and 2 others commented on an outdated diff Apr 14, 2013

library/Zend/I18n/Translator/Translator.php
+ }
+
+ return $this->events;
+ }
+
+ /**
+ * Set the event manager instance used by this translator.
+ *
+ * @param EventManagerInterface $events
+ * @return Translator
+ */
+ public function setEventManager(EventManagerInterface $events)
+ {
+ $events->setIdentifiers(array(
+ __CLASS__,
+ get_called_class(),
@iquabius

iquabius Apr 14, 2013

Contributor

Does get_called_class() and get_class($this) have the same behavior when called in a non-static method?

@prolic

prolic Apr 14, 2013

Contributor

Yes.

@weierophinney

weierophinney Apr 15, 2013

Owner

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.

@prolic

prolic Apr 15, 2013

Contributor

No! Why? get_called_class() is even faster and has the same result.

@weierophinney

weierophinney Apr 15, 2013

Owner

@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.

@prolic

prolic Apr 15, 2013

Contributor

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

@prolic

prolic Apr 15, 2013

Contributor

One fix quick done: zendframework#4235

Owner

weierophinney commented Apr 15, 2013

I think we shouldn't use the method's name as prefix for event names in the shipped ZF2 events

@wryck7 We have messaged since well before stable that event names should typically be ___FUNCTION__ in order to aid discovery. The only time we typically deviate from this is when multiple events may be triggered from within a method, or if the event triggered is only a small part of the workflow of a method.

@iquabius iquabius commented on the diff Apr 28, 2013

library/Zend/I18n/Translator/Translator.php
@@ -80,6 +82,20 @@ class Translator
protected $pluginManager;
/**
+ * Event manager for triggering translator events.
+ *
+ * @var EventManagerInterface
+ */
+ protected $events;
@iquabius

iquabius Apr 28, 2013

Contributor

Doesn't $eventManager make more sense here?

@iquabius iquabius commented on the diff Apr 28, 2013

library/Zend/I18n/Translator/Translator.php
@@ -80,6 +82,20 @@ class Translator
protected $pluginManager;
/**
+ * Event manager for triggering translator events.
+ *
+ * @var EventManagerInterface
+ */
+ protected $events;
+
+ /**
+ * Whether events are enabled
+ *
+ * @var bool
+ */
+ protected $eventsEnabled = false;
@iquabius

iquabius Apr 28, 2013

Contributor

$eventManagerEnabled would be more consistent with the methods' names.

@weierophinney

weierophinney Apr 28, 2013

Owner

We have commonly used $events throughout the framework.

On Saturday, April 27, 2013, Josias Duarte Busiquia wrote:

In library/Zend/I18n/Translator/Translator.php:

@@ -80,6 +82,20 @@ class Translator
protected $pluginManager;

 /**
  • \* Event manager for triggering translator events.
    
  • *
    
  • \* @var EventManagerInterface
    
  • */
    
  • protected $events;
  • /**
  • \* Whether events are enabled
    
  • *
    
  • \* @var bool
    
  • */
    
  • protected $eventsEnabled = false;

$eventManagerEnabled would be more consistent with the methods' names.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4187/files#r3990752
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@iquabius

iquabius Apr 28, 2013

Contributor

I've read a comment of yours saying this was due to a EventCollection the framework had before. Would it be acceptable to standardize this throughout the framework, using $eventManager instead?

@weierophinney

weierophinney Apr 28, 2013

Owner

No. It's fine as is, and with the plethora of examples showing this naming
that exist, changing it now would likely be more confusing.

On Sunday, April 28, 2013, Josias Duarte Busiquia wrote:

In library/Zend/I18n/Translator/Translator.php:

@@ -80,6 +82,20 @@ class Translator
protected $pluginManager;

 /**
  • \* Event manager for triggering translator events.
    
  • *
    
  • \* @var EventManagerInterface
    
  • */
    
  • protected $events;
  • /**
  • \* Whether events are enabled
    
  • *
    
  • \* @var bool
    
  • */
    
  • protected $eventsEnabled = false;

I've read a comment of yours saying this was due to a EventCollection the
framework had before. Would it be acceptable to standardize this throughout
the framework, using $eventManager instead?


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/4187/files#r3992190
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

Member

DASPRiD commented Apr 28, 2013

Changes applied, ready to merge.

Contributor

prolic commented Apr 28, 2013

good to merge

@weierophinney weierophinney commented on the diff Apr 29, 2013

library/Zend/I18n/Translator/Translator.php
@@ -511,6 +556,17 @@ protected function loadMessages($textDomain, $locale)
$messagesLoaded |= $this->loadMessagesFromFiles($textDomain, $locale);
if (!$messagesLoaded) {
+ if ($this->isEventManagerEnabled()) {
+ $this->getEventManager()->trigger(
+ self::EVENT_NO_MESSAGES_LOADED,
+ $this,
+ array(
+ 'locale' => $locale,
+ 'text_domain' => $textDomain,
+ )
+ );
+ }
+
$this->messages[$textDomain][$locale] = null;
@weierophinney

weierophinney Apr 29, 2013

Owner

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...

@prolic

prolic Apr 29, 2013

Contributor

+1, I missed that here.

@weierophinney weierophinney commented on the diff Apr 29, 2013

library/Zend/I18n/Translator/Translator.php
@@ -403,6 +435,18 @@ protected function getTranslatedMessage(
return $this->messages[$textDomain][$locale][$message];
}
+ if ($this->isEventManagerEnabled()) {
+ $this->getEventManager()->trigger(
+ self::EVENT_MISSING_TRANSLATION,
+ $this,
+ array(
+ 'message' => $message,
+ 'locale' => $locale,
+ 'text_domain' => $textDomain,
+ )
+ );
+ }
+
@weierophinney

weierophinney Apr 29, 2013

Owner

Might we want to return the result of the last listener, if it's a string?

@weierophinney weierophinney added a commit that referenced this pull request Apr 29, 2013

@weierophinney weierophinney [#4187] Intercept results of events
- 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.
d5c48a7

@weierophinney weierophinney added a commit that referenced this pull request Apr 29, 2013

@weierophinney weierophinney Merge branch 'feature/4187' into develop
Close #4187
c93037a

weierophinney merged commit e376710 into zendframework:develop Apr 29, 2013

@weierophinney weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge pull request zendframework/zendframework#4187 from DASPRiD/feat…
…ure/translator-event-manager-integration

Add event manager as soft dependency to translator

Conflicts:
	library/Zend/I18n/composer.json
ab97cde

@weierophinney weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

@weierophinney weierophinney [zendframework/zendframework#4187] Intercept results of events
- 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.
256b9bc

@weierophinney weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/4187' into develop f592cc2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment