IPE shouldn't depend on Page Manager

Recoverable fatal error: Argument 8 passed to Drupal\ctools\Plugin\DisplayVariant\BlockDisplayVariant::__construct() must be an instance of Drupal\Core\Block\BlockManager, none given, called in /Users/x/Sites/devdesktop/drupal/modules/panels/src/Plugin/DisplayVariant/PanelsDisplayVariant.php on line 115 and defined in Drupal\ctools\Plugin\DisplayVariant\BlockDisplayVariant->__construct() (line 97 of /Users/x/Sites/devdesktop/drupal/modules/ctools/src/Plugin/DisplayVariant/BlockDisplayVariant.php).

I'm getting this error when trying to add a Panels variant. Using dev everything as well as the related page_manager patch.

Ignore this, I was using an incompatible CTools release (dev).

When toggling a block form preview twice, I get this error:

Fatal error: Call to a member function getContexts() on null in /Users/x/Sites/devdesktop/drupal/modules/panels/panels_ipe/src/Form/PanelsIPEBlockPluginForm.php on line 332

It looks like the temporary value for "panels_display" is lost after the first PanelsIPEBlockPluginForm::submitPreview() call. I remember running into this with other persistent values like plugin_id, variant_id, and uuid, which is why I set them as Form values rather than temporary values (see PanelsIPEBlockPluginForm.php lines 162-165). We could re-load the panels_display every call based on the "variant_id" value, or figure out why temporary values are not working out for us.

OK, looked through this thoroughly. I don't see anything glaringly wrong (95% of this is documentation fixes), but I don't feel qualified to RTBC this patch; maybe @EclipseGc or @tim.plunkett should look over the more heavy-duty parts...

1. +++ b/panels_ipe/panels_ipe.info.yml
   @@ -5,4 +5,3 @@ core: 8.x
   -  - page_manager
   \ No newline at end of file
   

   Missing a new line!

2. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -45,6 +46,11 @@ class PanelsIPEPageController extends ControllerBase {
      /**
   +   * @var \Drupal\panels\Storage\PanelsStorageManagerInterface
   +   */
   +  protected $panelsStorage;
   

   Needs a description.

3. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -80,42 +88,43 @@ class PanelsIPEPageController extends ControllerBase {
   +   * @param \Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant $panels_display
   +   *   The current Panels display.
   

   Can the description be changed to something like "The Panels display to be saved"?

4. +++ b/panels_ipe/src/Controller/PanelsIPEPageController.php
   @@ -80,42 +88,43 @@ class PanelsIPEPageController extends ControllerBase {
   +   * @return \Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant
   +   *
   +   * @throws \Drupal\user\TempStoreException
   

   Need descriptions for @return and @throws.

5. +++ b/panels_ipe/src/Form/PanelsIPEBlockPluginForm.php
   @@ -47,6 +46,11 @@ class PanelsIPEBlockPluginForm extends FormBase {
      /**
   +   * @var \Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant
   +   */
   +  protected $panelsDisplay;
   

   Needs a description.

6. +++ b/panels_ipe/src/Form/PanelsIPEBlockPluginForm.php
   @@ -89,7 +93,7 @@ class PanelsIPEBlockPluginForm extends FormBase {
   +   * @param \Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant $panels_display
       *   The current PageVariant ID.
   

   Looks like the description needs to be updated.

7. +++ b/src/Plugin/DisplayBuilder/DisplayBuilderBase.php
   @@ -21,7 +21,8 @@ abstract class DisplayBuilderBase extends PluginBase implements DisplayBuilderIn
   +  public function build(PanelsDisplayVariant $panels_display) {
   +    $regions = $this->getRegionAssignments();
        return $regions;
   

   Isn't getRegionAssignments() a method of PanelsDisplayVariant? Why is it being called on $this?

8. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -31,6 +35,20 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
      /**
   +   * @var \Drupal\Core\Condition\ConditionManager
   +   *
   +   * @todo Remove when fixed in CTools: https://www.drupal.org/node/2642786
   +   */
   +  protected $conditionManager;
   +
   +  /**
   +   * @var \Drupal\Core\Block\BlockManager
   +   *
   +   * @todo Remove when fixed in CTools: https://www.drupal.org/node/2642786
   +   */
   +  protected $blockManager;
   

   The issue landed, so time for these to be shown the door! :)

9. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -75,20 +93,26 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
   +    // Inject dependencies as early as possible, so they can be used in
   +    // configuration.
   +    // @todo Remove when fixed in CTools: https://www.drupal.org/node/2642786
   +    $this->uuidGenerator = $uuid_generator;
   +    $this->blockManager = $block_manager;
   +    $this->conditionManager = $condition_manager;
   

   #2642786: Improve dependency injection in BlockDisplayVariant landed, so should these go away?

10. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
    @@ -135,6 +161,40 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
    +   * @param string $type
    +   *   The id of the IPE storage service to save with.
    

    Why does the description specifically mention IPE?

11. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
    @@ -135,6 +161,40 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
    +  public function getStorageType() {
    +    return $this->configuration['storage_type'] ?: NULL;
    +  }
    

    Will emit a notice if $this->configuration['storage_type'] is not set.

12. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
    @@ -135,6 +161,40 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
    +  public function getStorageId() {
    +    return $this->configuration['storage_id'] ?: NULL;
    +  }
    

    If storage_id is not set in $this->configuration, this will emit a notice. One possibility is to do return @$this->configuration['storage_id'].

13. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
    @@ -359,4 +433,28 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * @todo Remove when fixed in CTools: https://www.drupal.org/node/2642786
    +   */
    +  public function getSelectionConditions() {
    +    if (!$this->selectionConditionCollection) {
    +      $this->selectionConditionCollection = new ConditionPluginCollection($this->conditionManager, $this->getSelectionConfiguration());
    +    }
    +    return $this->selectionConditionCollection;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   *
    +   * @todo Remove when fixed in CTools: https://www.drupal.org/node/2642786
    +   */
    +  protected function getBlockCollection() {
    +    if (!$this->blockPluginCollection) {
    +      $this->blockPluginCollection = new BlockPluginCollection($this->blockManager, $this->getBlockConfig());
    +    }
    +    return $this->blockPluginCollection;
    +  }
    +
    

    #2642786: Improve dependency injection in BlockDisplayVariant appears to have landed, so I guess these can both be removed?

14. +++ b/src/Storage/PanelsStorageAccess.php
    @@ -0,0 +1,54 @@
    + * Contains \Drupal\panels\Storage\PanelsStorageAccess
    

    Missing a period.

15. +++ b/src/Storage/PanelsStorageAccess.php
    @@ -0,0 +1,54 @@
    +  /**
    +   * @var \Drupal\panels\Storage\PanelsStorageManagerInterface
    +   */
    +  protected $panelsStorage;
    

    Missing description.

16. +++ b/src/Storage/PanelsStorageAccess.php
    @@ -0,0 +1,54 @@
    +   * @param \Drupal\panels\Storage\PanelsStorageManagerInterface $panels_storage
    +   */
    +  public function __construct(PanelsStorageManagerInterface $panels_storage) {
    

    $panels_storage is missing a description.

17. +++ b/src/Storage/PanelsStorageAccess.php
    @@ -0,0 +1,54 @@
    +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
    +   *   The parametrized route
    

    Missing a period.

18. +++ b/src/Storage/PanelsStorageAccess.php
    @@ -0,0 +1,54 @@
    +   * @param \Drupal\Core\Session\AccountInterface $account
    +   *   The currently logged in account.
    

    Can $account be optional, to use the currently logged-in user?

19. +++ b/src/Storage/PanelsStorageAccess.php
    @@ -0,0 +1,54 @@
    +}
    \ No newline at end of file
    

    Missing a new line.

20. +++ b/src/Storage/PanelsStorageInterface.php
    @@ -0,0 +1,56 @@
    + * Contains \Drupal\panels_ipe\PanelsStorageInterface
    

    Missing period after class name.

21. +++ b/src/Storage/PanelsStorageInterface.php
    @@ -0,0 +1,56 @@
    +   *   The Panels display to save. Must have it's id within this storage service
    

    Nit: "it's" should be "its". And can this also be rephrased for clarity? Maybe something like "$panels_display->getStorageId() should return the display's ID as known to this storage service."

22. +++ b/src/Storage/PanelsStorageInterface.php
    @@ -0,0 +1,56 @@
    +   * @param $id
    +   *   The id for the Panels display within this storage service.
    +   * @param $op
    +   *   The operation to perform (ie. create, read, update, delete).
    

    Missing type hints.

23. +++ b/src/Storage/PanelsStorageInterface.php
    @@ -0,0 +1,56 @@
    +   * @param \Drupal\Core\Session\AccountInterface $account
    +   *   The user to check access for.
    

    Can $account be made optional to use the currently logged in user?

24. +++ b/src/Storage/PanelsStorageManager.php
    @@ -0,0 +1,72 @@
    + * Contains \Drupal\panels_ipe\PanelsStorageManager
    

    Missing a period after the class name.

25. +++ b/src/Storage/PanelsStorageManager.php
    @@ -0,0 +1,72 @@
    +  /**
    +   * @var \Drupal\panels\Storage\PanelsStorageInterface[]
    +   */
    +  protected $storage = [];
    

    Missing description.

26. +++ b/src/Storage/PanelsStorageManager.php
    @@ -0,0 +1,72 @@
    +   * @return \Drupal\panels\Storage\PanelsStorageInterface
    +   *
    +   * @throws \Exception
    +   */
    +  protected function getStorage($storage_type) {
    

    Missing descriptions in @return and @throws.

27. +++ b/src/Storage/PanelsStorageManager.php
    @@ -0,0 +1,72 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function save(PanelsDisplayVariant $panels_display) {
    +    $storage = $this->getStorage($panels_display->getStorageType());
    +    $storage->save($panels_display);
    +  }
    +
    

    This never throws the exception mentioned in the interface documentation. What gives?

28. +++ b/src/Storage/PanelsStorageManager.php
    @@ -0,0 +1,72 @@
    +}
    \ No newline at end of file
    

    Missing a new line here too.

29. +++ b/src/Storage/PanelsStorageManagerInterface.php
    @@ -0,0 +1,69 @@
    + * Contains \Drupal\panels_ipe\PanelsStorageManagerInterface
    

    Missing period after class name.

30. +++ b/src/Storage/PanelsStorageManagerInterface.php
    @@ -0,0 +1,69 @@
    +/**
    + * Interface for the Panels storage manager service.
    + */
    +interface PanelsStorageManagerInterface {
    

    Can this comment be expanded to explain a little more about what PanelsStorageManagerInterface is and does?

31. +++ b/src/Storage/PanelsStorageManagerInterface.php
    @@ -0,0 +1,69 @@
    +   * @param $storage_type
    +   *   The storage type to use for this storage service.
    

    Missing type hint.

32. +++ b/src/Storage/PanelsStorageManagerInterface.php
    @@ -0,0 +1,69 @@
    +   *   If the storage information isn't set, or there is no such Panels display.
    

    Er...what is meant by "if there is no such Panels display"? If we're passing the display in, doesn't it exist (even if only in memory)? Can this be clarified a bit?

33. +++ b/src/Storage/PanelsStorageManagerInterface.php
    @@ -0,0 +1,69 @@
    +   * @param \Drupal\Core\Session\AccountInterface $account
    +   *   The user to check access for.
    

    It would be handy to allow $account to be NULL, in which case it should default to the current user.

34. +++ b/src/Storage/PanelsStorageManagerInterface.php
    @@ -0,0 +1,69 @@
    +}
    \ No newline at end of file
    

    Missing a new line :)

Interdiff looks good! Two nitpicks:

1. +++ b/src/Storage/PanelsStorageManagerInterface.php
   @@ -42,10 +54,14 @@ interface PanelsStorageManagerInterface {
   +   *   $panels_display->getStorageId() must return the storage type and id as
   +   *   to the storage service.
   

   Needs to be "as known to the storage service", but that can be fixed on commit :)

2. +++ b/src/Storage/PanelsStorageManagerInterface.php
   @@ -42,10 +54,14 @@ interface PanelsStorageManagerInterface {
   +   *   storage service can't be found, or there is no Panels display found in
   +   *   storage service with the given id.
   

   Should be "...if there is no Panels display found in THE storage service with the given id." Fixable on commit.

Its looking pretty good, ran some manual tests and ran into one issue -- the block titles don't get saved. Tested against HEAD, which is working properly.

So did some more testing and now I cannot reproduce the title issue. Hopefully it was just a fluke. Needs more review!

Looks like everyone has reviewed this. David and I did a higher level review of this and I'm quite happy with what I see. Since Jakob has done a separate review, I went ahead and committed this since it will unblock a number of IPE issues.

Eclipse