Make variants into their own config entity

I'm not sure I understand the point of this.

As long as variants still have a hard dependency on the page (and I don't see how they could not, with static and route-based contexts in blocks, just to name one example), I'm not sure what we gain by making it separate.

I do see a number of downsides however, it makes dependencies more complex as you have a cross-dependency between the two, that's something the config system can't really handle. If a variant depends on the page and the page on the variant, which one should a config import going to import first?

Yes but again, I don't see how the display *can* be unaware of the page. Haven't look at the patch yet, but i don't see how you intend to deal with the context problem? And when it's not, then the single-variant feature is kinda pointless since it won't work ;)

We created a module called widget which is basically mini-panels as a block and we are still using an old branch of the layout project for that, one of the things that it has there is https://github.com/md-systems/layout/blob/old/src/LayoutRendererBlockAnd..., which in turn uses https://github.com/md-systems/layout/blob/old/src/Plugin/Layout/LayoutBl...), a very simple and generic interface that abstracts the concept of something that provides blocks, regions and context that is then rendered into a template.

Might be considerable easier to bring such a concept into ctools and then just use that in BlockDisplayVariant? See https://github.com/md-systems/widget/blob/master/src/Plugin/Block/Widget... how our usage of that looks like.

To be fair, I'm also worried about the fact that we are using page manager in production and such a massive change would result in considerable effort for me to write an upgrade path for our configuration.

But if everyone else thinks this direction makes sense and actually *works* (having variants that do not depend on the page in any way in the end) then don't let me hold you back. I'd just like to avoid a situation where we do all this refactoring and then in the end don't actually gain anything from it.

Getting involved to help push this forward ideologically.

The primary benefit is actually #3 in the issue summary. Node routes (as an example) have a long history of having variants introduced by feature plugins. When you take over node/{node}, and then you require a single module to own all variants, you're limiting your ability to introduce new node types and corresponding variants for them, which will make site-building an absolute pain, much less more generic site-building attempts.

Likewise, while I'm not certain that variants will be useful for mini-panels, they definitely share a lot of architectural overlap with panelizer (and a little overlap with mini-panels), so moving to this more abstracted feature element will likely result in a "DRY"er solution.

Finally, @berdir specifically, I feel your pain about upgrade path. While I am uncomfortable with so many people using the current page_manager implementation in production D8 sites, I recognize it's happening. Regardless of that, we can't let that fact retard progress on the D8 branch of page_manager, and its current UI and arch was Tim's effort to get SOMETHING, but I don't think anyone wants to maintain (or deal with) the current code in its current form for the next however many years. However, we should probably make a concession to reality here and commit to providing an upgrade path for this change. I've been debating that for a while, and I'm leaning toward including one.

Eclipse

Thanks for responding.

Don't worry too much about my upgrade path worries, I'm aware of the risk of using page manager in the current state and will deal with it. The upgrade path doesn't need to happen here, it could be added later once someone actually needs to do and test that (It won't work anyway otherwise ;)).

My main point is that I'm not sure that we're fully aware if the implications of this and we should test scenarios like config deployments, define who depends on whom and how things like static and route based contexts are supposed to work if a block in a variant uses it, as they are provided by the page at the moment (static could move I guess, route based not so much...)

Right, so I can describe how that should work, and as we refactor, if we find that our current reality doesn't match what I describe, we need to change it.

In the case of context injection into plugins (since that's the lion share of your argument/question) it should be irrelevant where the context originates. If a page has defined routes (or takes over a route) with arguments we're upcasting into contexts, those contexts should be injected into the variant entity when executing it. As a practical example:

say we take over the node of type "article" display on /node/{node}. We'll get a node from the route, and the page knows about this. The page then loops over the variant entities it possesses. As it does so, it injects its contexts (from url parameters) into the variants. When we arrive at our "article" variant, the specific contexts relevant to this variant are loaded, our selection criteria are evaluated, and then our page is rendered. If we have a panel that renders something dependent upon the node from the url, this is not horribly relevant to the variant, because internally it maps it to a key by a name ('node' probably since it's {node}). In Drupal 7, we can take a variant from one page, and put it on another, so long as the parameters upcast from the url remain the same. Even if the key of the context were to change, Page Manager will try to guess at the correct context to use, which it does by matching the first context of the necessary type. This logic doesn't exist in the 8.x version yet, but long term should be on our road map. It's also useful for remapping plugin contexts when you remove a context from the variant's scope that had plugins mapped to use it (or when you change the url upcasting on a custom landing page).

TL:DR;
Contexts should be injected through out our process, we should never manually reach backward to the page specifically to get contexts. The plugin mapping that is saved into a variants configuration should be completely ignorant of WHERE its contexts came from.

1. +++ b/page_manager.routing.yml
   @@ -121,58 +121,44 @@ page_manager.display_variant_select:
      path: '/admin/structure/page_manager/manage/{page}/add/{display_variant_id}'
   
   @@ -207,3 +193,29 @@ page_manager.selection_condition_delete:
   +  path: '/admin/config/system/page_variant'
   
   +++ b/src/Entity/PageVariantEntity.php
   @@ -0,0 +1,243 @@
   + *     "collection" = "/admin/config/system/page_variant"
   

   Why the different path?

2. +++ b/src/Controller/PageVariantListBuilder.php
   @@ -0,0 +1,36 @@
   +class PageVariantListBuilder extends ConfigEntityListBuilder {
   

   Why do we need to list page variants in isolation from their pages?

3. +++ b/src/Entity/Page.php
   @@ -79,11 +77,11 @@ class Page extends ConfigEntityBase implements PageInterface {
   -  protected $display_variants = [];
   +  protected $displayVariants = [];
   

   Out of scope

First real look at this...

1. +++ b/config/schema/page_manager.schema.yml
   @@ -18,17 +18,17 @@ page_manager.page.*:
          sequence:
   -        - type: display_variant.plugin.[id]
   -          label: 'Display variant'
   -    access_logic:
   -      type: string
   -      label: 'Access logic'
   +        - type: mapping
   +          label: 'Legacy display variant configuration.'
   

   Haven't seen anything else yet but this looks weird...

2. +++ b/config/schema/page_manager.schema.yml
   @@ -46,6 +46,42 @@ page_manager.page.*:
   +    page:
   +      type: page_manager.page.[id]
   +      label: 'Parent page'
   

   Again, only just started. But why is the reference from the display to the page and not the page that has a list of displays?

   Config queries by default are slow, but if we do this, we should at least the optimized keys that it offers, see lookup_keys in Block entity.

3. +++ b/page_manager.links.action.yml
   @@ -2,4 +2,9 @@ entity.page.add_form:
      title: 'Add page'
      appears_on:
   -    - 'page_manager.page_list'
   +    - 'entity.page.collection'
   

   the rename makes sense but doesn't seem like something that needs to be in this issue?

4. +++ b/page_manager.routing.yml
   @@ -48,6 +48,60 @@ entity.page.disable:
   +entity.page.condition.add:
   +  path: '/admin/structure/page_manager/access/add/{machine_name}/{condition}'
   +  defaults:
   +    _form: 'Drupal\page_manager\Form\AccessConfigure'
   +    tempstore_id: page_manager.page
   +    _title: 'Add Condition'
   +  requirements:
   

   what are those routes exactly? and is the tempstore stuff really related to this?

5. +++ b/page_manager.routing.yml
   @@ -121,18 +175,20 @@ page_manager.display_variant_select:
   +    #_entity_access: page_variant.add
   

   You're looking for _entity_create_access: page_variant I think.

6. +++ b/src/Entity/Page.php
   @@ -45,8 +48,9 @@ use Drupal\page_manager\Plugin\VariantAwareTrait;
     *     "add-form" = "/admin/structure/page_manager/add",
   - *     "edit-form" = "/admin/structure/page_manager/manage/{page}",
   + *     "edit-form" = "/admin/structure/page_manager/manage/{machine_name}",
     *     "delete-form" = "/admin/structure/page_manager/manage/{page}/delete",
   

   That doesn't sound right.

7. +++ b/src/Entity/Page.php
   @@ -191,6 +193,7 @@ class Page extends ConfigEntityBase implements PageInterface {
        parent::postCreate($storage);
        // Ensure there is at least one display variant.
   +    /*
        if (!count($this->getVariants())) {
          $this->addVariant([
            'id' => 'http_status_code',
   

   Interesting problem, should a new page auto-create a display, or should it do this at runtime. or just display an errror?

8. +++ b/src/Entity/Page.php
   @@ -213,6 +217,12 @@ class Page extends ConfigEntityBase implements PageInterface {
        parent::postDelete($storage, $entities);
   +    /** @var $entity \Drupal\page_manager\Entity\Page */
   +    foreach ($entities as $entity) {
   +      foreach ($entity->getVariants() as $variant) {
   +        $variant->delete();
   +      }
   +    }
   

   Define config dependencies (make the variant depend on the page) and drupal will do that automatically for you.

9. +++ b/src/Entity/Page.php
   @@ -342,6 +352,71 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function save() {
   +    $variants = $this->getVariants();
   +    $return = parent::save();
   +    if ($return) {
   +      foreach ($variants as $variant) {
   +        $variant->save();
   +      }
   +    }
   +    return $return;
   +  }
   

   is it even valid to change a loaded variant as part of a page?

   If yes, then save() is the wrong place for this, needs to be in post/preSave(). Consider save() a final method, since you could just call $storage->save($page)

10. +++ b/src/Entity/Page.php
    @@ -342,6 +352,71 @@ class Page extends ConfigEntityBase implements PageInterface {
    +    foreach ($entities as $entity) {
    +      $variants = [];
    +      foreach (\Drupal::entityManager()->getStorage('page_variant')->loadByProperties(['page' => $entity->id()]) as $variant) {
    +        $variants[$variant->uuid()] = $variant;
    +      }
    +      $entity->set('display_variants', $variants);
    

    Yeah. even if you cache it, this still has overhead, while a stored list of display ID's would just be a loadMultiple(). And wasn't one point of this to be able to re-use a display? that's not possible if it has a single, forced reference on a page?

11. +++ b/src/Entity/Page.php
    @@ -363,8 +437,19 @@ class Page extends ConfigEntityBase implements PageInterface {
     
    +  public function __wakeup() {
    +    parent::__wakeup();
    +    foreach ($this->display_variants as $key => $variant) {
    +      $this->display_variants[$key] = unserialize($variant);
    +    }
    +  }
    

    This is very tricky because you don't know if the variant changed.. you might still have the page cached and it might use an old version of the variant. Probably have to load them again. But you might want to consider loading them only on-demand anyway.

12. +++ b/src/Entity/PageVariantEntity.php
    @@ -0,0 +1,234 @@
    + *   config_prefix = "page_variant",
    

    You don't need a config prefix if it's identical to the ID. only if you e.g. want to shorten the config name to just page_manager.variant instead of page_variant.

13. +++ b/src/Entity/PageVariantEntity.php
    @@ -0,0 +1,234 @@
    +   * {@inheritdoc}
    +   */
    +  public function toArray() {
    +    $properties = parent::toArray();
    +    $names = [
    +      'id',
    +      'label',
    +      'uuid',
    +      'variant',
    +      'variant_settings',
    +      'page',
    

    You can define them as a config_export property in the annotation.

14. +++ b/src/Entity/PageVariantEntity.php
    @@ -0,0 +1,234 @@
    +  /**
    +   * @todo Document!
    +   */
    +  public function getAccessLogic() {
    +    return $this->selection_logic;
    +  }
    

    a bit strange name mismatch...

Grml, yeah, apparently. sorry :)

How is this related to #2561963: Display Config Entity. It seems like there could be a lot of duplication of effort.

Thanks for the explanation! I've been trying to keep up with the Port Panels8 meetings but obviously missed that conversation.

Sounds like we're postponing this one dependent on the page-manager -> ctools move.

I'd suggest re-rolling this patch against patch on #2511566: Add an abstract version of Drupal\page_manager\Plugin\DisplayVariant\BlockDisplayVariant from Page Manager comment 10.

Also for what its worth EclipseGC hates the ConditionVariantInterface so we want to remove it from ctools pretty quickly.

But we also don't want to block the panels folks from using the newly moved code.

this will take a while, there are arguments around whether to do this or not. We're moving items to ctools first.

Also, from the Features perspective, in D7 we already do this using Features Override and we'll likely just need something similar for D8. I can see the case that the architecture of variants within pages is analogous to displays in a view. Having a config entity have a list of other related config entities is beyond the scope of just panel variants I think. So I agree with postponed.

oh wait, I was confusing "pages" with "panelizer". In D7 page variants *are* separate exportables. Sorry. So yeah, we are still debating ;)

OK, here is a version of the patch that refactors the entityManager->getStorage('page_variant') into protected methods.

Just nitpick fixes for now, added a couple @todos that we need to clean up.

Also rebased it on other nitpick changes I made to 8.x-1.x

High-level review. Looking at the entity types and cacheability aspects.

1. +++ b/src/Entity/PageVariantEntity.php
   @@ -0,0 +1,259 @@
   + *   id = "page_variant",
   ...
   +class PageVariantEntity extends ConfigEntityBase implements PageVariantEntityInterface {
   

   Shouldn't the class be named PageVariant? And the interface PageVariantInterface?

   That'd then match the ID. And the form classes.

   And every entity type in core.

2. +++ b/src/Entity/PageViewBuilder.php
   @@ -20,15 +21,24 @@ class PageViewBuilder extends EntityViewBuilder {
   +        // We need to add all variants because they all affect the selection of
   +        // the variant (ie. a variant's weight can change and bump ahead of the
   +        // others).
   +        foreach ($entity->getVariants() as $other_variant) {
   +          $display_variant->addCacheableDependency($other_variant);
   +        }
   

   I'd expect this to be taken care of in the Page and PageVariant config entities' logic. It should be encapsulated there, otherwise everything that deals with Pages and Page Variants will have to repeat this sort of logic.

   Look at e.g. \Drupal\aggregator\Entity\Item::postSave() for inspiration — we have a few of these entity types with very strong coupling amongst each other.

1. With that reasoning, it should be PageVariantPlugin and PageVariantEntity?
2. Ah, I see! Indeed :)

   But my overall point still stands I think: a page entity actually depends on multiple page variant entities: when the page is rendered, it selects a page variant to use. So doesn't it then make more sense to let the page variant entities use the same cache tags as the page entity they are associated with? (Just like e.g. feed item entities use the cache tag of the feed they belong to.)

Keep PageVariantEntity and PageVariantEntityInterface

For minimum consistency, we should then at least rename Page to PageEntity. And per my first point, actually also PagePlugin and PageVariantPlugin. Or… just go with Page and PageVariant everywhere, and rely on namespaces — that's what they're here for. You can still always call the instances' variable names $page_plugin and $page_entity.

Banish (ie. stop using) "display variant" or $display_variant any where in code or comments which is sometimes used to refer to the plugin - it's too ambiguous

If it's the same as "page variant", then yes, please consolidate all that. But I think that doesn't need to happen here per se; could be a follow-up.

Does the

postSave()

will when importing a .yaml via CMI?

Yes, it's called then too. Otherwise it'd be a poor abstraction :) (But given that such edge cases were commonplace in D7, I totally understand your skepticism :D)

It means adding fewer cache tags to the render array.

Yes, but more importantly: a system that is easier to reason about!

1. +++ b/src/Entity/PageVariantEntity.php
   @@ -0,0 +1,297 @@
   +  public function getPluginCollections() {
   +    return [
   +      'selection_criteria' => $this->getSelectionConditions(),
   +    ];
   +  }
   ...
   +  public function getVariantPlugin() {
   +    if (!$this->variantPlugin) {
   +      // @todo Inject this service!
   +      $this->variantPlugin = \Drupal::service('plugin.manager.display_variant')->createInstance($this->variant, $this->variant_settings);
   +    }
   +    return $this->variantPlugin;
   +  }
   

   Why doesn't getPluginCollections include the variant plugin? It can use a single plugin collection like the block->block_plugin relationship.

2. +++ b/src/Entity/PageVariantEntity.php
   @@ -0,0 +1,297 @@
   +    return Page::load($this->page);
   

   Do we want to store this? Or just rely on entity static caching?
   Also, ewwwwwwwww. Really wish this wasn't necessary.

3. +++ b/src/Entity/PageViewBuilder.php
   @@ -20,15 +21,17 @@ class PageViewBuilder extends EntityViewBuilder {
   +    if ($page_variant = $entity->getExecutable()->selectVariant()) {
   ...
   +      return $display_variant->build();
   

   Would we want to have a PageVariant view builder instead?

4. +++ b/src/PageExecutable.php
   @@ -58,19 +62,49 @@ class PageExecutable implements PageExecutableInterface {
   +   * @return boolean
   

   bool

5. +++ b/src/PageExecutable.php
   @@ -58,19 +62,49 @@ class PageExecutable implements PageExecutableInterface {
   +  public function canSelectPageVariant(PageVariantEntityInterface $page_variant) {
   

   This method seems a little out of place. Should it be on the page variant entity?

1. +++ b/src/Controller/PageManagerController.php
   @@ -326,4 +314,22 @@ class PageManagerController extends ControllerBase {
   +    $entity = $this->entityManager()->getStorage('page_variant')->create(['page' => $page->id(), 'variant' => $display_variant_id]);
   

   Nit: The stuff in create() should be broken up into several lines for readability.

2. +++ b/src/Entity/Page.php
   @@ -346,13 +342,84 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function addVariant(array $configuration) {
   +    if (empty($configuration['id'])) {
   +      throw new \Exception("Must pass an 'id' for the new page variant.");
   +    }
   

   One suggestion -- if the variant ID is required, maybe it should be an explicit argument to the method.

3. +++ b/src/Entity/Page.php
   @@ -346,13 +342,84 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function addVariantEntity(PageVariantEntityInterface $variant) {
   +    $this->displayVariants[$variant->id()] = $variant;
   +    return $variant->id();
   

   I see that variants are sorted by UUID in getVariants(). Why are they keyed by ID here instead of UUID? Should $this->displayVariants be re-sorted after adding the variant?

4. +++ b/src/Entity/Page.php
   @@ -346,13 +342,84 @@ class Page extends ConfigEntityBase implements PageInterface {
   +    if (!isset($variants[$variant_id])) {
   +      throw new \Exception('The requested display variant does not exist or is not associated with this page.');
   +    }
   

   Nit: Maybe this should be \InvalidArgumentException or something otherwise more specific than \Exception.

5. +++ b/src/Entity/Page.php
   @@ -346,13 +342,84 @@ class Page extends ConfigEntityBase implements PageInterface {
   +    if (!isset($this->displayVariants)) {
   +      $this->displayVariants = [];
   

   Why bother with an isset() check here? Is there some reason $this->displayVariants can't be defined with an empty array as its default value?

6. +++ b/src/Entity/Page.php
   @@ -346,13 +342,84 @@ class Page extends ConfigEntityBase implements PageInterface {
   +      foreach ($this->entityStorage()->loadByProperties(['page' => $this->id()]) as $variant) {
   

   This line is strange. $this->entityStorage() is getting the *variant* entity storage, yet this is on the page entity class. entityStorage() should probably be renamed to getVariantStorage() or similar.

7. +++ b/src/Entity/Page.php
   @@ -346,13 +342,84 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function variantSortHelper($a, $b) {
   

   $a and $b should probably be type hinted.

8. +++ b/src/Entity/PageVariantEntity.php
   @@ -0,0 +1,334 @@
   +  /**
   +   * The ID of the display variant plugin.
   +   *
   +   * @var string
   +   */
   +  protected $variant;
   

   I kind of wish this was named pluginId or similar, rather than the more ambiguous "variant".

9. +++ b/src/Entity/PageVariantEntity.php
   @@ -0,0 +1,334 @@
   +  /**
   +   * @var array
   +   */
   +  protected $contexts = [];
   

   This one needs an explanation :)

10. +++ b/src/Entity/PageVariantEntity.php
    @@ -0,0 +1,334 @@
    +  public function setWeight($weight) {
    +    $this->weight = $weight;
    +    return $this;
    

    $weight isn't type hinted so maybe this should be $this->set('weight')?

11. +++ b/src/Entity/PageVariantEntity.php
    @@ -0,0 +1,334 @@
    +  public function getSelectionLogic() {
    +    return $this->selection_logic;
    +  }
    

    For consistency, should this be $this->get('selection_logic')?

12. +++ b/src/Entity/PageVariantEntity.php
    @@ -0,0 +1,334 @@
    +    foreach ($conditions as $condition) {
    +      if ($condition instanceof ContextAwarePluginInterface) {
    +        /** @var \Drupal\Core\Plugin\Context\ContextHandlerInterface $context_handler */
    +        $context_handler = \Drupal::service('context.handler');
    +        $context_handler->applyContextMapping($condition, $contexts);
    +      }
    +    }
    

    Micro-optimization -- the call to \Drupal::service() can go outside the foreach loop.

13. +++ b/src/Form/DisplayVariantDeleteBlockForm.php
    @@ -83,11 +74,20 @@ class DisplayVariantDeleteBlockForm extends ConfirmFormBase {
    +  /**
    +   * @todo.
    +   *
    +   * @return \Drupal\ctools\Plugin\BlockVariantInterface
    +   */
    +  protected function getVariantPlugin() {
    

    @todo? :)

14. +++ b/src/Form/PageVariantAddForm.php
    @@ -0,0 +1,35 @@
    +/**
    + * Class PageVariantForm.
    + *
    + * @package Drupal\page_manager\Form
    + */
    +class PageVariantAddForm extends PageVariantFormBase {
    

    This docblock isn't very helpful.

15. +++ b/src/Form/PageVariantConfigureForm.php
    @@ -0,0 +1,67 @@
    +  /**
    +   * Returns a unique string identifying the form.
    +   *
    +   * @return string
    +   *   The unique string identifying the form.
    +   */
    +  public function getFormId() {
    

    Should be @inheritdoc.

16. +++ b/src/Form/PageVariantConfigureForm.php
    @@ -0,0 +1,67 @@
    +  /**
    +   * Form constructor.
    +   *
    +   * @param array $form
    +   *   An associative array containing the structure of the form.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The current state of the form.
    +   *
    +   * @return array
    +   *   The form structure.
    +   */
    +  public function buildForm(array $form, FormStateInterface $form_state) {
    

    Ditto.

17. +++ b/src/Form/PageVariantConfigureForm.php
    @@ -0,0 +1,67 @@
    +  /**
    +   * Form submission handler.
    +   *
    +   * @param array $form
    +   *   An associative array containing the structure of the form.
    +   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   *   The current state of the form.
    +   */
    +  public function submitForm(array &$form, FormStateInterface $form_state) {
    

    Ditto.

18. +++ b/src/Form/PageVariantDeleteForm.php
    @@ -0,0 +1,62 @@
    +  /**
    +   * The entity being used by this form.
    +   *
    +   * @var \Drupal\page_manager\PageVariantEntityInterface
    +   */
    +  protected $entity;
    

    Already defined in EntityForm, which this form extends.

19. +++ b/src/Form/PageVariantEditForm.php
    @@ -0,0 +1,304 @@
    +        '#attached' => [
    +          'library' => [
    +            'core/drupal.ajax',
    +          ],
    +        ],
    

    I don't see how this is being used -- is there any reason to attach drupal.ajax here?

20. +++ b/src/Form/PageVariantEditForm.php
    @@ -0,0 +1,304 @@
    +      '#attached' => [
    +        'library' => [
    +          'core/drupal.ajax',
    +        ],
    +      ],
    

    Again, I don't see how drupal.ajax is being used here, so does it need to be attached?

21. +++ b/src/Form/PageVariantEditForm.php
    @@ -0,0 +1,304 @@
    +  public function save(array $form, FormStateInterface $form_state) {
    +    // @todo This feels very wrong.
    +    $variant_plugin = $this->getVariantPlugin();
    

    Missing doc comment.

22. +++ b/src/Form/PageVariantFormBase.php
    @@ -0,0 +1,187 @@
    +/**
    + * Class PageVariantForm.
    + *
    + * @package Drupal\page_manager\Form
    + */
    +abstract class PageVariantFormBase extends EntityForm {
    

    This doc comment is not informative.

23. +++ b/src/Form/PageVariantFormBase.php
    @@ -0,0 +1,187 @@
    +   * @param \Drupal\Core\Entity\Query\QueryFactory $entity_query
    +   *   The entity query factory.
    +   */
    +  public function __construct(QueryFactory $entity_query) {
    

    Shouldn't this be an interface instead of a concrete implementation?

24. +++ b/src/PageInterface.php
    @@ -48,6 +47,55 @@ interface PageInterface extends ConfigEntityInterface, EntityWithPluginCollectio
    +  /**
    +   * @todo.
    +   *
    +   * @param \Drupal\page_manager\PageVariantEntityInterface $variant
    +   *
    +   * @return string
    +   */
    +  public function addVariantEntity(PageVariantEntityInterface $variant);
    

    s/@todo/actual info

25. +++ b/src/PageVariantEntityInterface.php
    @@ -0,0 +1,112 @@
    +  /**
    +   * Gets the plugin id of the display variant plugin.
    +   *
    +   * @return string
    +   */
    +  public function getVariantPluginId();
    

    Nit: s/plugin id/plugin ID

26. +++ b/src/PageVariantEntityInterface.php
    @@ -0,0 +1,112 @@
    +  /**
    +   * Gets selection criteria by condition id.
    +   *
    +   * @param string $condition_id
    +   *   The id of the condition.
    +   *
    +   * @return \Drupal\Core\Condition\ConditionInterface
    +   */
    +  public function getSelectionCondition($condition_id);
    

    Nit: s/id/ID

27. +++ b/src/PageVariantEntityInterface.php
    @@ -0,0 +1,112 @@
    +  /**
    +   * Removes selection criteria by condition id.
    +   *
    +   * @param string $condition_id
    +   *   The id of the condition.
    +   *
    +   * @return $this
    +   */
    +  public function removeSelectionCondition($condition_id);
    

    Ditto.

Small fixes. Too lazy to run all the tests locally.

Saving variants also needs to rebuild the router. We have this code on the page, is there any better way to do this @dsnopek?

Okay my @todo from before was the last bug in PageManagerAdminTest. What else is left?

#66

1) Fixed
2) Removed this method
3) Should always sort by ID
4) Yep, fixed
5) We do that to help prevent loading them all each time
6) Renamed.
7) We usually don't do this, not sure why. Undone for now
8) Not sure how I feel, unchanged for now.
9) Unchanged
10) Nah, setters are good. We want to discourage usage of the generic set/get
11) Redundant.
12) Unchanged, but yes. Also it should be in a protected method (the service call)
13) Unchanged
14) Unchanged
15/16/17) Fixed
18) This is for typehinting purposes.
19/20) This is needed for the AjaxFormTrait calls to work
21) Fixed
22) Unchanged
23) This has no interface :( But it's a factory, not as bad as the actual class
24) Unchanged
25) Fixed
26/27) Fixed

I also removed the variant ID munging. This will likely break many tests. More to do!

---

So #66 points 9, 12, 13, 14, 22, 24 need addressing still.

Fixed an issue with saving. The way variants are saved is similar to plugin collections, so we fix it in copyFormValuesToEntity.

This fixes access and the rest of #66. I still have some work to do, just posting progress.

I spent some time on the plane home looking into PageVariantViewBuilder, and it SO VASTLY simplified things, I want to include it here.

So! The final blocker here....
The breadcrumbs are completely broken now.

First we have the page edit form:
/admin/structure/page_manager/manage/{page}
Then we have the variant edit form:
/admin/structure/page_manager/variant/{page_variant}

Either we should make it
/admin/structure/page_manager/manage/{page}/variant/{page_variant}
or we have to write a custom breadcrumb class.

But there is no way to get from a variant back to it's page, and it is infuriating.

I spent some time on the plane home looking into PageVariantViewBuilder, and it SO VASTLY simplified things, I want to include it here.

Nice :)

Reviewed just that bit:

1. +++ b/src/Entity/PageVariantViewBuilder.php
   @@ -0,0 +1,46 @@
   +class PageVariantViewBuilder extends EntityViewBuilder {
   

   Hrm… it seems misleading for this to extend EntityViewBuilder, because…

2. +++ b/src/Entity/PageVariantViewBuilder.php
   @@ -0,0 +1,46 @@
   +    return $variant_plugin->build();
   

   … that is where all the building happens!

   It seems more honest/clear to make this implement the interface directly and make several of the methods no-ops.

I do that because EntityViewBuilder is a shitty interface. It should be called FieldableEntityViewBuilderInterface...

Also, PageViewBuilder (which we're replacing), does the same thing.

But that means that e.g. \Drupal\Core\Entity\EntityViewBuilderInterface::viewFieldItem() is implemented, which AFAICT makes no sense for page variant entities?

I just gave it a try… and proved myself more wrong than right :)

This is basically what you want:

class PageVariantViewBuilder implements EntityViewBuilderInterface {
  public function buildComponents(array &$build, array $entities, array $displays, $view_mode) {
    throw \LogicException;
  }

  public function view(EntityInterface $entity, $view_mode = 'full', $langcode = NULL) {
    // c/p from patch
  }

  public function viewMultiple(array $entities = array(), $view_mode = 'full', $langcode = NULL) {
    // c/p from patch
  }

  public function resetCache(array $entities = NULL) {
    // Duplicate from EntityViewBuilder.
  }

  public function viewField(FieldItemListInterface $items, $display_options = array()) {
    throw \LogicException;
  }


  public function viewFieldItem(FieldItemInterface $item, $display_options = array()) {
    throw \LogicException;
  }

  public function getCacheTags();
    // Duplicate from EntityViewBuilder.
  }

}

But, note two bits are duplicated from EntityViewBuilder. Well… except they shouldn't be, because ::view() doesn't actually add the cache tags from getCacheTags(), which means resetCache() is pointless to implement.

So, either:

• extend EntityViewBuilder like your patch does, update it to use EntityViewBuilder::getCacheTags() in view(), and override the nonsensical methods to throw a logic exception
• implement the interface directly, and also throw logic exceptions from getCacheTags() and resetCache()

Gahh I hate EntityViewBuilderInterface so much.
But you're right.

We can't safely throw exceptions in restCache(). But WhyTF is buildComponents public?!

This fixes the path structure. I think I'm happy with this now?!

Looks good to me! Let's get this in.

Eclipse

Thanks everyone!