Event or Hook desireable

Hi Matthias,

Thanks for wanting to help out. I'm currently not in a position to work on this myself, but I'll happily accept patches :)

Thanks for your contribution @mfrosh.

I've reviewed your patch and found the following issues with it. It also needs test coverage.

1. +++ b/term_merge/src/TermMergeEvent.php
   @@ -0,0 +1,44 @@
   +  protected $termsToMerge;
   +  protected $targetTerm;
   

   What are these? Can you add typehints to them please.

2. +++ b/term_merge/src/TermMergeEvent.php
   @@ -0,0 +1,44 @@
   +   * @param \Drupal\node\Entity\Node $node
   +   * @param \Drupal\node\Entity\Node $orignode
   

   These typehints seem wrong to me :)

3. +++ b/term_merge/src/TermMergeEvent.php
   @@ -0,0 +1,44 @@
   +  public function getTermsToMerge() {
   +    return $this->termsToMerge;
   +  }
   +
   +  public function getTargetTerm() {
   +    return $this->targetTerm;
   +  }
   +
   +  public function setTermsToMerge($termsToMerge) {
   +    $this->termsToMerge = $termsToMerge;
   +  }
   +
   +  public function setTargetTerm($targetTerm) {
   +    $this->targetTerm = $targetTerm;
   +  }
   

   Please document these methods. What do they accept and what do they return.

4. +++ b/term_merge/src/TermMergeEvent.php
   @@ -0,0 +1,44 @@
   +}
   \ No newline at end of file
   

   Nitpick: Missing a newline.

+++ b/src/TermMergeEvent.php	(revision ee78b9915256b871ac1e37a2a3f11baedc43e614)
@@ -0,0 +1,84 @@
+  /**
+   * Set/Update terms to merge.
+   *
+   * @param array $termsToMerge
+   *   An array of terms to merge.
+   */
+  public function setTermsToMerge(array $termsToMerge) {
+    $this->termsToMerge = $termsToMerge;
+  }
+
+  /**
+   * Set/Update the target term.
+   *
+   * @param \Drupal\taxonomy\Entity\Term $targetTerm
+   *   Single target term object.
+   */
+  public function setTargetTerm(Term $targetTerm) {
+    $this->targetTerm = $targetTerm;
+  }

This event is fired after terms were already merged. Using these methods can therefore never have any effect, nor are the terms to merge or target term ever retrieved after the event was dispatched. I think they should be removed.

Besides that, I would still like test coverage for this before committing it.

Tests are added in the attached patch. The setters are removed too. Ready for a new review :)

Thanks for working on this @marcoweijenborg. I've taken a look at your patch and found the following nitpicks.

1. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   +/**
   + * Event used for Merges.
   

   Event used to notify subscribers that terms were merged.

2. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   + * Class TermMergeEvent.
   + *
   + * @package Drupal\term_merge
   

   These lines can go.

3. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   +class TermMergeEvent extends Event {
   

   /s/TermMergeEvent/TermsMergedEvent

4. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   +   * @var array
   

   @var \Drupal\taxonomy\Entity\Term[]

5. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   +  protected $termsToMerge;
   

   The merge process is nearly completed at this point. All that remains is the removal of the source terms. So let's call this $sourceTerms

6. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   +   * @param array $termsToMerge
   

   $sourceTerms

7. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   +   * Get all terms to merge.
   

   Retrieve the terms that are being merged into the target term.

8. +++ b/src/TermMergeEvent.php
   @@ -0,0 +1,64 @@
   +  public function getTermsToMerge() {
   

   getSourceTerms

9. +++ b/src/TermMerger.php
   @@ -87,6 +87,11 @@ class TermMerger implements TermMergerInterface {
   +    // Trigger term merge event
   

   This comment must end with a '.'

10. +++ b/src/TermMerger.php
    @@ -87,6 +87,11 @@ class TermMerger implements TermMergerInterface {
    +    $event = $dispatcher->dispatch('term_merge.merge_action', $event);
    

    Core uses specific final classes to define and document event names. Let's follow that practice.

    See \Drupal\Core\Entity\EntityTypeEvents.

11. +++ b/tests/src/Functional/IntegrationTest.php
    @@ -20,7 +20,7 @@ class IntegrationTest extends BrowserTestBase {
    -  public static $modules = ['node', 'taxonomy', 'term_merge'];
    +  public static $modules = ['node', 'taxonomy', 'term_merge', 'term_merge_test_events'];
    

    This array should be split over multiple lines.

12. +++ b/tests/term_merge_test_events/src/EventSubscriber/TermMergeEventSubscriber.php
    @@ -0,0 +1,33 @@
    +/**
    + * @file
    + * Contains \Drupal\term_merge_test_events\EventSubscriber\TermMergeEventSubscriber.
    + */
    

    Namespaced classes should not begin with a file doc comment

The improvements are added to the patch number 21.

I another couple of nits to pick, but we're almost there! :)

1. +++ b/src/TermMergeEventNames.php
   @@ -0,0 +1,25 @@
   +  const TERMS_MERGED = 'term_merge.merge_action';
   

   /s/merge_action/terms_merged

2. +++ b/src/TermMerger.php
   @@ -87,6 +87,11 @@ class TermMerger implements TermMergerInterface {
   +    // Trigger term merge event.
   

   I don't think this comment adds much. Reading the code it's clear that the term merge event is dispatched.

3. +++ b/src/TermMerger.php
   @@ -87,6 +87,11 @@ class TermMerger implements TermMergerInterface {
   +    $dispatcher = \Drupal::service('event_dispatcher');
   

   The event dispatcher should be injected through the constructor

4. +++ b/tests/term_merge_test_events/src/EventSubscriber/TermsMergedEventSubscriber.php
   @@ -0,0 +1,29 @@
   + * The TermsMergedEventSubscriber event subscriber.
   

   I think it would be better to describe what this class does instead of what it is.

5. +++ b/tests/term_merge_test_events/term_merge_test_events.info.yml
   @@ -0,0 +1,8 @@
   +name: 'Term merge test events'
   

   Drupal coding standards dictate that each word in a module name starts with a capital

Ok this should do it :) For some reason, the interdiff couldn't be created, so I only attached the patch.

Patch failed to apply, I attached a new patch.

Committed and pushed to 8.x-1.x-dev. Thanks!