Fire an event when a Panels display variant is saved

1. +++ b/src/PanelsEvents.php
   @@ -0,0 +1,23 @@
   +final class PanelsEvents {
   
   +++ b/src/PanelsVariantEvent.php
   @@ -0,0 +1,40 @@
   +class PanelsVariantEvent extends Event {
   

   Looks good, follows the naming and docs conventions of core.

2. +++ b/src/Plugin/PanelsStorage/PageManagerPanelsStorage.php
   @@ -67,6 +71,8 @@ class PageManagerPanelsStorage extends PanelsStorageBase implements ContainerFac
      public function save(PanelsDisplayVariant $panels_display) {
   +    parent::save($panels_display);
   +
        $id = $panels_display->getStorageId();
   

   This is the part that worries me. Like this code, Panelizer doesn't call parent::save.

   IMO, the actual event dispatching should happen in \Drupal\panels\Storage\PanelsStorageManager::save(). The way the code reads, that should be the only thing ever calling ::save() on PanelsStorage

3. +++ b/src/Storage/PanelsStorageBase.php
   @@ -3,7 +3,50 @@
   +    // The event dispatcher is optional here in order to maintain backwards
   +    // compatibility.
   +    $this->eventDispatcher = $event_dispatcher ?: \Drupal::service('event_dispatcher');
   

   Good looking out, since without this Panelizer would break.

4. +++ b/src/Storage/PanelsStorageBase.php
   @@ -3,7 +3,50 @@
   +    // Allow event subscribers to react to the variant being saved.
   +    $event = new PanelsVariantEvent($panels_display);
   +    $this->eventDispatcher->dispatch(PanelsEvents::VARIANT_SAVE, $event);
   

   Couldn't be more straightforward

5. +++ b/tests/src/Unit/PanelsStorageTest.php
   @@ -82,7 +92,13 @@ class PanelsStorageTest extends UnitTestCase {
   -    $panels_storage = new PageManagerPanelsStorage([], '', [], $this->entityTypeManager->reveal());
   +    $panels_storage = new PageManagerPanelsStorage(
   +      [],
   +      '',
   +      [],
   +      $this->entityTypeManager->reveal(),
   +      $this->eventDispatcher->reveal()
   +    );
   

   *eyeroll for formatting change*

6. +++ b/tests/src/Unit/PanelsStorageTest.php
   @@ -108,7 +124,18 @@ class PanelsStorageTest extends UnitTestCase {
   +    $this->eventDispatcher
   +      ->dispatch(PanelsEvents::VARIANT_SAVE, Argument::type(PanelsVariantEvent::class))
   +      ->shouldBeCalled();
   

   Yayyyy

1. +++ b/src/Plugin/PanelsStorage/PageManagerPanelsStorage.php
   @@ -18,6 +18,8 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
    class PageManagerPanelsStorage extends PanelsStorageBase implements ContainerFactoryPluginInterface {
    
      /**
   +   * The entity type manager.
   +   *
       * @var \Drupal\Core\Entity\EntityTypeManagerInterface
       */
   
   +++ b/src/Storage/PanelsStorageBase.php
   +++ b/src/Storage/PanelsStorageBase.php
   @@ -4,6 +4,8 @@ namespace Drupal\panels\Storage;
   
   @@ -4,6 +4,8 @@ namespace Drupal\panels\Storage;
    
    use Drupal\Component\Plugin\PluginBase;
    
   +/**
   + * Base class for Panels storage plugins.
   + */
    abstract class PanelsStorageBase extends PluginBase implements PanelsStorageInterface {
   

   out of scope now

2. +++ b/src/Storage/PanelsStorageManager.php
   @@ -84,6 +97,10 @@ class PanelsStorageManager extends DefaultPluginManager implements PanelsStorage
        $storage->save($panels_display);
   +
   +    // Allow event subscribers to react to the variant being saved.
   +    $event = new PanelsVariantEvent($panels_display);
   +    $this->eventDispatcher->dispatch(PanelsEvents::VARIANT_SAVE, $event);
   

   Is there any reason to move the event before the actual save call? Maybe if an event subscriber wanted to throw an exception or modify the thing before it is saved...

   Also this makes me think it should be named VARIANT_PRE_SAVE or VARIANT_POST_SAVE to indicate which is happening.

3. +++ b/tests/src/Kernel/PanelsStorageManagerTest.php
   @@ -0,0 +1,70 @@
   +  public function testEvents() {
   

   This test does a lot of specific things to set up a valid $page and $variant, but the only assertion is on Argument::type(PanelsVariantEvent::class)

   To follow-up my earlier point of when the event is actually fired, I think a clearer test would be to do something like

   $this->container->get('event_dispatcher')->addListener(PanelsEvents::VARIANT_SAVE, function (PanelsVariantEvent $event) {
     $event->getVariant()->disable();
   }));
   

   And then check that the variant was disabled afterwards

4. +++ b/tests/src/Unit/PanelsStorageTest.php
   @@ -82,7 +82,12 @@ class PanelsStorageTest extends UnitTestCase {
   -    $panels_storage = new PageManagerPanelsStorage([], '', [], $this->entityTypeManager->reveal());
   +    $panels_storage = new PageManagerPanelsStorage(
   +      [],
   +      '',
   +      [],
   +      $this->entityTypeManager->reveal()
   +    );
   
   @@ -108,7 +113,12 @@ class PanelsStorageTest extends UnitTestCase {
   -    $panels_storage = new PageManagerPanelsStorage([], '', [], $this->entityTypeManager->reveal());
   +    $panels_storage = new PageManagerPanelsStorage(
   +      [],
   +      '',
   +      [],
   +      $this->entityTypeManager->reveal()
   +    );
   

   Not gonna highlight them all, but also out of scope now

That test looks great! If it passes, this is good to go.

Looks good. Committed.