Use CTools Wizard API to add/edit Pages (and move plugin UI using PluginWizardInterface)

1. +++ b/page_manager.routing.yml
   @@ -82,32 +157,6 @@ page_manager.access_condition_delete:
   -#### Static Contexts
   

   This whole section is missing.

2. +++ b/page_manager.routing.yml
   @@ -153,7 +202,7 @@ page_manager.display_variant_select_block:
   -    _form: '\Drupal\page_manager\Form\DisplayVariantAddBlockForm'
   +    _form: 'Drupal\page_manager\Form\DisplayVariantAddBlockForm'
   
   @@ -161,7 +210,7 @@ page_manager.display_variant_add_block:
   -    _form: '\Drupal\page_manager\Form\DisplayVariantEditBlockForm'
   +    _form: 'Drupal\page_manager\Form\DisplayVariantEditBlockForm'
   
   @@ -169,7 +218,7 @@ page_manager.display_variant_edit_block:
   -    _form: '\Drupal\page_manager\Form\DisplayVariantDeleteBlockForm'
   +    _form: 'Drupal\page_manager\Form\DisplayVariantDeleteBlockForm'
   

   Bad merge

3. +++ b/page_manager.services.yml
   @@ -9,11 +12,6 @@ services:
   -  page_manager.static_context:
   -    class: Drupal\page_manager\EventSubscriber\StaticContext
   -    arguments: ['@entity.manager']
   -    tags:
   -      - { name: 'event_subscriber' }
   

   Same, all of the static context stuff is missing?

4. +++ b/src/Controller/PageManagerController.php
   @@ -184,7 +168,7 @@ class PageManagerController extends ControllerBase {
   -    return $this->redirect('page_manager.page_list');
   +    return $this->redirect('entity.page.collection');
   

   Little things like this we can merge in early, can you split this change out to another issue?

Related to static contexts, earlier today EclipseGC and I discussed making a change to the way contexts are passed to display variants:

#2550941: Allow passing contexts to display variants (to enable Panels Everywhere to use static context and relationships)

If we're able to get that into core, then we can implement static contexts using that!

EDIT: Er, actually, looking at the code, that is just totally unrelated. :-) Page Manager already has ContextAwareVariantInterface and can do the right thing. The core issue would just move that goodness into core, so that Panels Everywhere can use it.

However, in the meantime, the existing static context stuff should be worked back into the wizard patch.

Here's a re-rolled patch against the latest page_manager so it doesn't remove the static context stuff. That said, I didn't actually test it or attempt to port the static context stuff into the wizard, so I'm sure this still needs work. I also didn't address any of Tim's comments in #3

I did some manual testing on my re-rolled patch on #5 and trying to create a new Page results in a fatal error. :-/ The problem is that BlockDisplayVariant->buildConfigurationForm( ) needs $this->executable, but I'm not sure how construct a PageExecutable before the Page actually exists... This would be a lot simpler if we got #2511570: Remove Drupal\page_manager\Plugin\PageAwareVariantInterface done first!

I also did a quick read through the patch:

1. +++ b/page_manager.routing.yml
   @@ -11,18 +9,30 @@ page_manager.page_list:
   -    _entity_access: page.update
   +    #_entity_access: page.update
   +    _entity_create_access: page
   

   Is this debug code temporarily commenting out _entity_access? Or does the _entity_create_access replace it and the _entity_access line should be removed entirely?

   (I'm pretty ignorant of the routing system, sorry!)

2. +++ b/page_manager.routing.yml
   @@ -48,6 +58,71 @@ entity.page.disable:
   +    _title: 'Add Condition'
   

   Should this (and the route title that follow) use sentance case? For example: "Add condition" instead of "Add Condition"?

   I don't know the answer to that -- I'm really asking. :-)

3. +++ b/page_manager.services.yml
   @@ -1,4 +1,7 @@
      page_manager.current_user_context:
        class: Drupal\page_manager\EventSubscriber\CurrentUserContext
        arguments: ['@current_user', '@entity.manager']
   

   Just marking this code for myself to try and figure out how the current user context is handled if not through this event subscriber...

4. +++ b/src/Entity/Page.php
   @@ -106,12 +131,9 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  protected $menu_type = 'route';
   +
   +  protected $menu_settings = [];
    
   

   Missing docblock.

5. +++ b/src/Entity/Page.php
   @@ -248,7 +315,7 @@ class Page extends ConfigEntityBase implements PageInterface {
   -      'display_variants' => $this->getVariants(),
   +      //'display_variants' => $this->getVariants(),
   

   Debug code or should really be a deletion?

6. +++ b/src/Entity/Page.php
   @@ -331,7 +398,8 @@ class Page extends ConfigEntityBase implements PageInterface {
   -    return $this->getExecutable()->getContexts();
   +    //return $this->getExecutable()->getContexts();
   +    return $this->getParameters();
   

   This one really seems like it should be a deletion. We can have the PageExecutable pass the context directly to the variant, which may actually be happening here, but I haven't gotten to that part of the patch yet.

7. +++ b/src/Entity/Page.php
   @@ -342,6 +410,79 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function getMenuType() {
   +    return $this->menu_type;
   +  }
   +
   +  public static function preDelete(EntityStorageInterface $storage, array $entities) {
   +    parent::preDelete($storage, $entities); // TODO: Change the autogenerated stub
   +  }
   +
   +  public function getMenuSettings() {
   +    return $this->menu_settings;
   +  }
   

   Missing docblock!

8. +++ b/src/Entity/Page.php
   @@ -342,6 +410,79 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function addVariantEntity(PageVariantInterface $variant) {
   +    $this->display_variants[$variant->id()] = $variant;
   +    return $variant->id();
   +  }
   

   Missing docblock!

9. +++ b/src/Entity/PageVariant.php
   @@ -0,0 +1,159 @@
   +class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   

   I'm not sure about the PageVariant entity implementing PageVariantInterface from a "seperation of concerns" perspective.

   I'd feel better about the PageVariant entity having a function that would construct the the underlying variant plugin which is responsible for the runtime work, and then returning that to call directly.

10. +++ b/src/Entity/PageVariant.php
    @@ -0,0 +1,159 @@
    +  protected $weight;
    +
    +  protected $uuid;
    +
    +  protected $variant;
    +
    +  protected $variant_settings = [];
    

    Loads of missing docblocks. I think I'm going to stop pointing these out now. :-)

11. +++ b/src/Form/PageSelectionForm.php
    @@ -0,0 +1,67 @@
    +/**
    + * @file
    + * Contains \Drupal\page_manager\Form\PageAccessForm.
    + */
    

    This is a lie! This file (PageSelectionForm.php) really has PageSelectionForm.

12. +++ b/src/Form/PageVariantConfigureForm.php
    @@ -0,0 +1,66 @@
    +/**
    + * @file
    + * Contains \Drupal\page_manager\Form\PageVariant\ConfigureForm.
    + */
    

    Also a lie. Should be 'PageVariantConfigureForm'

13. +++ b/src/Form/PageVariantForm.php
    @@ -0,0 +1,70 @@
    +      '#description' => $this->t("Label for the PageVariant."),
    

    For human-readable interface strings we should decide what we're going to call it. Personally, I like "variant" best. Another option is "page variant" but I definitely don't think we should do "PageVariant"

14. +++ b/src/Form/SelectionConfigure.php
    @@ -0,0 +1,54 @@
    +/**
    + * @file
    + * Contains \Drupal\page_manager\Form\AccessConfigure;
    + */
    

    Another lie in SelectionConfigure. I'll stop pointing these out - it's just nits.

15. +++ b/src/PageInterface.php
    @@ -48,6 +56,11 @@ interface PageInterface extends ConfigEntityInterface, EntityWithPluginCollectio
       /**
    +   * @return \Drupal\Core\Plugin\Context\ContextInterface[]
    +   */
    +  public function getParameters();
    

    getParameters() is for context that is from the URL, right?

16. +++ b/src/PageVariantInterface.php
    @@ -0,0 +1,37 @@
    +interface PageVariantInterface extends ConfigEntityInterface {
    

    Could we put "Entity" into the name of this interface somewhere? Otherwise, it'll be difficult to tell the differents between this and PageVariantInterface from core which refers to the variant plugin.

Adding to sprint board.

Would it make sense to extract the changes which convert page variants to independent config entities into it's own issue? That would certainly help move #2511570: Remove Drupal\page_manager\Plugin\PageAwareVariantInterface foward...

I started an issue to extract the parts that convert page variants into their own config entities on this issue: #2551633: Make variants into their own config entity

Postponing on that, so we can get that chunk in first, and hopefully make it easier to review/integrate the wizard changes.

I'm copying part of a response that I posted on #2513910-9: Consider using page manager UI similar to Backdrop? because I think it might clarify why I think the wizard changes are necessary:

Here's the general problem from a low-level architecture stand-point:

1. Page Manager is about creating routes and having variants attached
2. The variants could be anything including plain HTTP responses - they don't have to know anything about blocks or layout
3. So, the variants need to be able to add things to the UI through their API somehow, while also taking UI settings for the things that are common across all variants (like "Menu" for example)
4. The Panels variant adds the concept of both layout and assigning blocks to those layouts - so, the Panels variant needs to provide a UI for setting up both those things
5. However, both Panels in D7 and Backdrop split the configuring of "which layout" and "what goes on the layout" to separate forms (which I think makes a lot of sense!) - so the API for variants to provide their UI for configuration shouldn't be returning a single form (ala ConfigurablePluginInterface, but returning a set of forms
6. We've chosen to call "a set of forms which edits a single entity" the Wizard API in CTools

So, I guess I would say, just pretend we didn't call it the Wizard API. :-) It's really about the fact that we'll need multiple forms to edit a single variant, and what those forms are should be entirely invisible to Page Manager, because they are provided by the variant.

Updated issue summary

This is unblocked now that #2551633: Make variants into their own config entity has been merged!

First, this'll need a re-roll on to the new code.

Assigning to myself! I've started working on the re-roll...

Here's a straight re-roll, that sort of works. It allows you to create a new page and add a variant to it, but it appears you're unable to configure the "Block page" variant beyond just the page title. So, the next step is iterating on this until you can add a variant fully with all it's configuration (including block placement).

Just wanted to post this patch with only a small number of changes because I'm working on some heavier stuff that might not get posted for a bit!

I attempted to make the page_manager.routing.yml more consistent with what's currently in page_manager HEAD. This was a little difficult because the page entities aren't actually created yet, so we can't rely on entity access. We'll need to do some custom access stuff at some point, checking special for unsaved pages and checking normal entity access when editing an existing page.

This is blocked on fixing #2607552: Pass $cached_values to WizardFormInterface::getOperations() in the Wizard API to allow us to correctly vary the wizard steps depending on whether we're creating a new page or editing an existing page.

Here's a long-overdue updated version of this patch! It works for both adding and editing of the page, but it depends on a number of patches to the CTools Wizard API:

#2616492: Allow overriding cached values for specific operations to make it possible to use the same form class for multiple steps

#2616494: On any reusable wizard step forms, pass $cached_values to methods return route info

#2619948: Cached values not in tempstore when first editing entity in wizard

Here's a new patch that makes some small bug fixes and changes the buttons on edit to be "Update" and "Update and save". But mostly it removes the menu stuff, which I moved into this patch on #2620036-2: Allow pages to have menu items

This new patch implements full CRUD with variants (adding, deleting, re-ordering) and removes a bunch of old code. The next step is finally to try and implement PluginWizardInterface!

Here's a new patch that actually does the dream of moving the configuration of variant plugins into wizard steps via PluginWizardInterface (which also means this now depends on this CTools patch: #2607382: Add PluginWizardInterface so plugins can be configured with wizard steps). I thoroughly messed up generating an interdiff so I'm not including one with this change - sorry!

This patch adds some vaguely page_manager-ish styling to the the edit wizard. I never liked the page_manager UI so it's a little bittersweet recreating it like this. :-/ But here it is.

Since this is no longer about a specific branch (it's all in patches now), I've retitled this issue to describe the actual change it's making.

Not a meta, so changing category.

Here's an attempt to get access correct on the routes using #2623888: Add ParamConverter that pulls entity out of the tempstore but there's a whole bunch of routes that won't work for... I've been trying to see if I can improve it to allow those to work too, but then I hit #2619948-11: Cached values not in tempstore when first editing entity in wizard again and it's blocking forward progress on that.

So, here's this patch is a couple commits backward in my Git repo, but it's the last thing that works as well as the previous patch.

I added a "Remaining tasks" section to the issue summary which has everything from my personal TODO that still needs to be done in this patch.

Here's a cursory review that I did while reading this patch to get a high-level overview. I don't know any Page Manager details. So it's mostly code style/cleanliness things. Hope it helps.

1. +++ b/page_manager.module
   @@ -0,0 +1,65 @@
   + * Preprocess function for page-manager-wizard-tree.html.twig
   

   Nit: missing trailing period.

2. +++ b/page_manager.routing.yml
   @@ -11,18 +11,30 @@ entity.page.collection:
   +    # @todo: this is really _entity_access: page.update
   +    _entity_create_access: page
   
   @@ -48,37 +60,63 @@ entity.page.disable:
   +    # @todo: this is really _entity_access: page.update
   +    _permission: 'administer pages'
   ...
   +    # @todo: should really check permission to edit entity somehow...
   +    #_entity_access: page.update
   +    _permission: 'administer pages'
   

   What's preventing us from doing what the @todo states? I guess because the entity is not available as a route parameter?

3. +++ b/page_manager.routing.yml
   @@ -48,37 +60,63 @@ entity.page.disable:
        _title: 'Add new access condition'
   

   s/new//

4. +++ b/src/Controller/PageManagerController.php
   @@ -64,12 +72,15 @@ class PageManagerController extends ControllerBase {
   +   * @param \Drupal\user\SharedTempStoreFactory $tempstore
   

   No interface to typehint on?

5. +++ b/src/Entity/Page.php
   @@ -138,6 +159,10 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function getDescription() {
   
   @@ -163,6 +188,40 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function getParameters() {
   ...
   +  public function getParameter($parameter) {
   

   Missing/incomplete docblock for these and other methods here.

6. +++ b/src/Form/AccessConfigure.php
   @@ -0,0 +1,67 @@
   +    /** @var $page \Drupal\page_manager\Entity\Page */
   ...
   +    /** @var $page \Drupal\page_manager\Entity\Page */
   ...
   +    /** @var $page \Drupal\page_manager\Entity\Page */
   ...
   +    /** @var $page \Drupal\page_manager\Entity\Page */
   
   +++ b/src/Form/AccessDelete.php
   @@ -0,0 +1,57 @@
   +    /** @var $page \Drupal\page_manager\Entity\Page */
   ...
   +    /** @var $page \Drupal\page_manager\Entity\Page */
   ...
   +    /** @var $page \Drupal\page_manager\Entity\Page */
   

   Typehint to the interface?

7. +++ b/src/Form/PageAccessForm.php
   @@ -0,0 +1,68 @@
   +  protected function getConditionClass() {
   +    return 'Drupal\page_manager\Form\AccessConfigure';
   +  }
   

   This looks pretty strange.

8. +++ b/src/Form/PageContextsForm.php
   @@ -0,0 +1,89 @@
   +      '#header' => array($this->t('Context'), $this->t('Type'), $this->t('Operations')),
   +      '#rows' => $this->renderRows($cached_values),
   +      '#empty' => t('No Contexts configured for this variant.')
   

   array() -> []

   t() -> $this->t()

   Here and elsewhere in this file.

9. +++ b/src/Form/PageContextsForm.php
   @@ -0,0 +1,89 @@
   +    $form['#attached']['library'][] = 'core/drupal.dialog.ajax';
   

   There's no AJAXy stuff here. If this is necessary, should have an @see to make that clear.

10. +++ b/src/Form/PageGeneralForm.php
    @@ -0,0 +1,183 @@
    +   * Constructs a new DisplayVariantAddForm.
    

    Outdated docs.

11. +++ b/src/Form/PageGeneralForm.php
    @@ -0,0 +1,183 @@
    +          //'contexts' => $this->t('Contexts'),
    

    Dead code, or needs a @todo?

12. +++ b/src/Form/PageParameterForm.php
    @@ -0,0 +1,152 @@
    +      '#header' => array($this->t('Argument'), $this->t('Position in path'), $this->t('Context assigned'), $this->t('Operations')),
    +      '#rows' => $this->renderRows($cached_values),
    +      '#empty' => t('No required conditions have been configured.')
    

    Same remarks as for PageContextsForm.

13. +++ b/src/Form/PageVariantConfigureForm.php
    @@ -0,0 +1,94 @@
    + * Contains \Drupal\page_manager\Form\PageVariant\ConfigureForm.
    

    Mismatch.

14. +++ b/src/Form/PageVariantDeleteForm.php
    @@ -7,35 +7,74 @@
    +  protected $tempstore;
    +
    +  /**
    +   * Constructs a PageVariantDeleteForm.
    +   *
    +   * @param \Drupal\user\SharedTempStoreFactory $tempstore
    +   *   The tempstore factory.
    +   */
    +  public function __construct(SharedTempStoreFactory $tempstore) {
    +    $this->tempstore = $tempstore;
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function create(ContainerInterface $container) {
    +    return new static(
    +      $container->get('user.shared_tempstore')
    +    );
    +  }
    

    I've seen this stuff several times now; looks like this could use a base class. Or perhaps a trait.

15. +++ b/src/Form/SelectionConfigure.php
    @@ -0,0 +1,81 @@
    + * Contains \Drupal\page_manager\Form\AccessConfigure;
    

    Mismatch.

16. +++ b/src/Form/SelectionDelete.php
    @@ -0,0 +1,48 @@
    + * Contains \Drupal\page_manager\Form\AccessDelete.
    

    Mismatch.

17. +++ b/src/Form/VariantPluginConfigureBlockFormBase.php
    @@ -36,6 +45,43 @@ abstract class VariantPluginConfigureBlockFormBase extends FormBase {
    +   * Constructs a new DisplayVariantAddForm.
    

    Mismatch.

18. +++ b/src/Form/VariantPluginContentForm.php
    @@ -0,0 +1,251 @@
    +   * Constructs a new DisplayVariantAddForm.
    

    Mismatch.

19. +++ b/src/Form/VariantPluginContentForm.php
    @@ -0,0 +1,251 @@
    +   * Get the tempstore id.
    

    s/id/ID/

Handed this one off to EclipseGC last week! I'm a little burnt out on the issue. But I'll be happy to come back with reviews (or even take it back) later.

Nice start, feels much more natural adding new pages.
Current patch fatals (on the overview page) though with devel and/or config translation enabled though, fair warning when testing.

Here's an interdiff between #24 and #31. I generated it quick and dirty without reviewing it, so no guarantee's it's correct, but it might be useful. :-)

I did a quick read through the interdiff, and have two small bits of review:

1. +++ b/src/Entity/PageVariant.php
   @@ -44,7 +44,7 @@ use Drupal\page_manager\PageVariantInterface;
   - *     "edit-form" = "/admin/structure/page_manager/manage/{page}/variant/{page_variant}",
   + *     "edit-form" = "/admin/structure/page_manager/manage/{machine_name}/{step}",
   

   I don't think this is right. There really isn't an independent route to edit a PageVariant (only a whole page) after this patch, so maybe we want to remove "edit-form" completely? Otherwise, other modules (like devel, config_translation, etc) will try to put their own routes below that route.

2. +++ b/src/Form/SelectionDelete.php
   @@ -14,7 +14,7 @@ class SelectionDelete extends ConditionDelete {
   -    return ['entity.page.edit_form', ['machine_name' => $this->machine_name, 'step' => 'selection']];
   +    return ['entity.page.edit_form', ['machine_name' => $this->machine_name, 'step' => 'page_variant__' . $cached_values['page_variant']->id() . '__selection']];
   

   The old return value is for adding a new Page, and then new return value is for editing. :-) This should have an if/else like in SelectionConfigure::getParentRouteInfo().

I've also updated the remaining tasks in the issue summary!

Here's a patch with the explicit 'parameters' mapping stuff removed!

I've put those changes on this follow-up issue instead: #2631802: Allow explicitly configuring the type of context from parameters

And here's a patch addressing my own review from #33.

Now, I'm going to refocus my attention away from actual functional changes (leaving those to EclipseGC) and start working on updating the tests. The UI itself is pretty stable, so it should be OK to work on the tests. And I think it'll make a good division of labor where we won't necessarily step on each others toes patch-wise!

@dsnopek regarding #33.1 : I think that route actually helps for config translation to create a dedicated page to translated properties of the variant (say title for instance). Haven't tested that patch though, but in my initial testing before (see #29) the overview actually crashed with config translation enabled - and devel too :/

@swentel: If those modules won't make up their own route if the config entity doesn't have its own edit route, then we might need to make a special route just for them, which would be unfortunate. :-/ In any case, after this patch, we don't have an obvious existing route for the page variant entity to be edited independently of the page entity, which is why I removed those from the entity type annotation in #35.

Here's a patch that gets the PHPUnit tests running, PageManagerTranslationIntegrationTest and a start on PageNodeSelectionTest. It makes a couple small functional changes to fix bugs discovered in working on the tests.

Just reviewing functionality (visually), I noticed the following:

1. When adding a page, removing a condition added at /admin/structure/page_manager/add/[machine_name]/access throws the following error: error: Call to a member function id() on null in /Users/samuel.mortenson/Sites/drupal/modules/page_manager/src/Form/AccessDelete.php on line 19. The same error is thrown when trying to configure a condition after the page is added.

2. When adding a page, removing a condition added at /admin/structure/page_manager/add/[machine_name]/selection throws the following error: Warning: call_user_func_array() expects parameter 1 to be a valid callback, class 'Drupal\page_manager\Form\SelectionDelete' does not have a method 'validate' in Drupal\Core\Form\FormValidator->executeValidateHandlers() (line 88 of core/lib/Drupal/Core/Form/FormValidator.php).

3. Once a page is added, adding a "Selection Criteria" condition throws the following error: Fatal error: Call to a member function get() on null in /Users/samuel.mortenson/Sites/drupal/modules/page_manager/src/Form/SelectionConfigure.php on line 58

4. The text "Configure Condition" is used when adding conditions, but what you're really doing is adding a new condition. Once a condition is added you click "Edit" to configure it, so it seems like there needs to be a standardization of terms used.

5. The deletion confirmation screen in a lot of places should have the "Cancel" button inline with the "Delete" button. You can see this by deleting a Page, Variant, or Condition.

6. "Add variant" and "Reorder variants" look like tabs on the top-right hand corner of the page configuration screen, but they just open modals. Could we add these to the kind-of-vertical-tabs on the left side of the page, nested under "Variants"?

7. To delete a Page, you click on the "Delete" tab, but to delete a Variant you have to go to the nested menu on the left and click on Variants -> Variant Name -> General, which has a "Delete this variant" button. Can we standardize this a bit?

My problems with PageNodeSelectionTest turned out to be a CTools bug: #2639526: Calling BlockDisplayVariant::setConfiguration() won't change results returned from plugin collections

Here's a patch that re-orders the stuff that test does to be slightly easier to read.

This patch gets all the tests, passing except for StaticContextTest because static contexts aren't yet supported (EclipseGC is working on it). That said, there are a number @todo's added, at least one nasty hack to get things working, AND there are some new UIs in this patch which aren't covered. But I'd say this gets us 90% of the way there!

Marking as "Needs review" just to make sure testbot gets the same results that I'm getting locally..

Bah! The tests did worse on testbot than locally on my machine, because this patch depends on changes in CTool 8.x-3.x-dev. Until we have a new CTools release, this patch will never pass. :-/

Here's an attempt to clean up the "nasty hack" from yesterday, so that it's clear exactly what it's doing and why, and to make it easier to remove later (when contexts are solely on the variant per #2631800: Store static contexts on the variant rather than the page). Today, I'm going to attempt to flesh out PageManagerAdminTest a bit more to cover all new UIs in this patch, and try to eliminate some of the other @todo's in there.

This patch adds tests for the 'Access conditions' and 'Selection criteria' steps in both the add and edit wizards. This also fixes the bugs that @samuel.mortenson encountered in #41.1, #41.2, and #41.3, and those things are covered by tests now. :-) I'll respond to the rest of Sam's review in my next comment.

Regarding the rest of the UI review in #41:

4. The text "Configure Condition" is used when adding conditions, but what you're really doing is adding a new condition. Once a condition is added you click "Edit" to configure it, so it seems like there needs to be a standardization of terms used.

Agreed! This actually comes from \Drupal\ctools\Form\ManageConditions so this will be a CTools change.

5. The deletion confirmation screen in a lot of places should have the "Cancel" button inline with the "Delete" button. You can see this by deleting a Page, Variant, or Condition.

Yeah, I'm not 100% sure on the cause of this but I think it comes from \Drupal\ctools\Form\ConditionDelete, so again, a CTools change.

6. "Add variant" and "Reorder variants" look like tabs on the top-right hand corner of the page configuration screen, but they just open modals. Could we add these to the kind-of-vertical-tabs on the left side of the page, nested under "Variants"?

So, they are at the top looking tab-ish because that's how they look in D7's page_manager, although, the proliferation of tabs everywhere is one of the things I personally hate about D7's page_manager. :-) However, they open in dialogs for purely technical reasons - they aren't actually wizard steps.

All that said, I'd personally be fine with them being moved to the tree thinger on the left. The question really is how closely we want to mimick the UI of D7's page_manager, which probably needs discussion.

7. To delete a Page, you click on the "Delete" tab, but to delete a Variant you have to go to the nested menu on the left and click on Variants -> Variant Name -> General, which has a "Delete this variant" button. Can we standardize this a bit?

In D7's page_manager, it had a second set of horizontal tabs for the variant, which is where the delete button is for the variant. I think I did it this way because it was easier to implement, and all the tabs hurt my soul. :-) But we could definitely switch back to that to match D7, but let's wait until we have a discussion about just how closely we want to mimick that.

Here's a CTools issue to fix one of my @todo's: #2640392: ConditionDelete puts a 'validate' method on '#validate' that doesn't exist

Updated the patch to reference it.

I'll try to pull CTools patches for #41.4 and #41.5, see if we can get this done in that project. For the bigger UX concerns, we can always open up a new issue after the technical portions of this are done. Given our release deadlines I don't want to slow down progress (too much). :-)

Thanks, Sam!

Here's a patch that addresses an issue where the label given for a variant would overwrite the label of the page. It turns out to come from a bug in the Wizard API:

#2640410: FormWizardBase/EntityFormWizardBase take the label/id from any form element called label/id on any step

But, for now, implemented a workaround that switches the label for the variant to be 'page_variant_label' rather than just 'label'.

Hrm. I went to dig into one of my hacks in the tests from yesterday (a strategically placed drupal_flush_all_caches()), and it appears to no longer be necessary! It must have been fixed by one of the other fixes I did earlier.

So, here's a new patch that just removes that!

This patch sets up the access on all the 'block_display' routes per the CTools "access object" stuff that EclipseGC added a couple days ago.

Looking through the patch, I think we're to a point where the tests are as far as they can be until EclipseGC finishes the work he's doing on getting static contexts working, and finishes up the couple trivial "Remaining tasks". So, I'm going to stop sprinting on this until that stuff is done!

Submitted some small UX patches to related projects:

#45.4 - #2640472: \Drupal\ctools\Form\ManageConditions submit text not clear
#45.5 - #2640464: Links styled as buttons not placed inside Dialog's button pane — prevents "cancel" link/button from showing up

Testing this for the first time, some feedback. Feel free to move parts of that to follow-ups or ignore. Didn't read everything that was mentioned already.

1. The fieldset on the label/id seems confusing to me. I've seen that @EclipseGc's UI for pathauto patterns had the same, not sure where this is coming from. I don't see a reason for that fieldset label, doesn't provide any additional information and complicates the UI.
2. Given that path is required, I'd move that above description.
3. The Variant type is a bit confusing, it's about which variant you want to create as the first one, right? Needs a better label + a description IMHO.
4. Also confused about the selection rules/access controls optional feature checkboxes. Selection rules apply to variants and are only relevant when you have more than one, why not let me decide if I want that while configuring the variant? Will there be more options later? If just access rules remains, then maybe we can find a better, less confusing UI for that? (e.g. do it later on, or handle it through multiple buttons)
5. Path could really use a description/example (placeholder) to explain things like arguments, if you need a leading / or not, etc. (The current UI doesn't have that, but the it also doesn't have a whole other bunch that we are adding ;) But follow-up is fine, e.g. as part of making arguments work properly).
6. Additionally to the button label mentioned in #45.4, I'd say that the button should be next to the select and not in the same area as the wizard buttons
7. I checked both optional checkboxes and I get a Selection Criteria step before actually configuring the variant. The UI doesn't give me any explanation about the difference between selection criterias and access rules. We need some intro text above each step, maybe through hook_help() ?
8. The machine name for variants is global, but the default suggestion will often result in duplicates. I think it would be useful if the default machine name would be prefixed with the page machine name?
9. A fair amount of functionality still seems to be missing? I can't actually configure a variant, static contexts UI isn't there, etc. What's the plan for completing this, how much will be part of the first patch? The patch is already huge and way too big for a code review I think but leaving out functionality also doesn't seem like an option for existing installations?

@Berdir: Thanks for the review! For the most part, I agree with all your points, however, I have couple comments on the last two...

#55.8: Actually, the default machine name for the first variant created does include the page machine name, so it's always unique. For variants added later, it's up to the user to keep the machine name unique just like it works in current page_manager 8.x-1.x.

#55.9: You can definitely edit a variant! Otherwise, we wouldn't have been able to get all the admin UI tests passing. :-) The only piece of functionality that should be missing at this point is the static contexts, which EclipseGC was working on last week, but never posted a patch - I'm not sure where he's at with it.

Updating "Remaining tasks" because we don't want to do: "On the Structure -> Pages listing page, show pages that are only in the tempstore and haven't been saved to configuration yet"

I started updating the StaticContextTest now that those are working(-ish)! However, I'm still getting the LogicException that #59 was meant to fix when adding contexts in the add wizard. :-/

Blergh. So, I tried finishing up the tests! However, the contexts weren't getting serialized at all, so in order pass down the contexts I had to add in a hack that is dehybrating and rehydrating the contexts manually from the tempstore. This allows blocks that need context to be added in the UI and configured, but testing with the EntityView block from CTools, it errors out fatally when actually trying to view the page and I'm not sure why. :-/

So, here's a patch that gets slightly further but static contexts are still not working for me...

Huzzah! Here's a patch that has all the tests passing locally. It depends on having the latest 8.x-1.x branch of CTools.

From the last patch, I had to make some small changes to the StaticContextConfigure form to prevent some PHP warnings (which get counted as 'exceptions', ie. failures, in simpletest). And, unset()'ting the plugin collections in __sleep() was causing the magic plugin collection serialization to not happen and broke previously passing tests. But other than that, it's just changes to the tests.

I've updated the "Remaining tasks" in the issue summary.

1. +++ b/src/Entity/Page.php
   @@ -37,6 +39,7 @@ use Drupal\page_manager\PageVariantInterface;
   + *     "description",
   
   @@ -70,6 +73,13 @@ class Page extends ConfigEntityBase implements PageInterface {
   +   * The description of the page entity.
   +   *
   +   * @var string
   +   */
   +  protected $description;
   +
   +  /**
   
   @@ -141,6 +151,13 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public function getDescription() {
   +    return $this->description;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   

   This should be a separate issue.

2. +++ b/src/Entity/Page.php
   @@ -174,6 +191,13 @@ class Page extends ConfigEntityBase implements PageInterface {
   +    // Save the page variants just in case they were modified (or added) and
   +    // not saved independently.
   +    foreach ($this->getVariants() as $page_variant) {
   +      $page_variant->save();
   +    }
   

   This should be a separate issue.

3. +++ b/src/Entity/Page.php
   @@ -297,8 +321,21 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public static function preDelete(EntityStorageInterface $storage, array $entities) {
   +    parent::preDelete($storage, $entities); // TODO: Change the autogenerated stub
   +  }
   

   This should be removed :)

4. +++ b/src/Entity/Page.php
   @@ -297,8 +321,21 @@ class Page extends ConfigEntityBase implements PageInterface {
   +    // If variants hasn't been initialized, we initialize it before adding the
   +    // new variant.
   +    if ($this->variants === NULL) {
   +      $this->getVariants();
   +    }
        $this->variants[$variant->id()] = $variant;
   +    $this->sortVariants();
   

   This should be a separate issue.

5. +++ b/src/Entity/Page.php
   @@ -331,23 +368,19 @@ class Page extends ConfigEntityBase implements PageInterface {
   -      @uasort($this->variants, [$this, 'variantSortHelper']);
   ...
   +      @uasort($this->variants, '\Drupal\page_manager\Entity\PageVariant::sort');
   

   Why this switch? And this should be a separate issue.

6. +++ b/src/Entity/Page.php
   @@ -356,21 +389,8 @@ class Page extends ConfigEntityBase implements PageInterface {
   -    // Avoid serializing plugin collections and the page executable as they
   -    // might contain references to a lot of objects including the container.
   -    $unset_vars = [
   -      'variants' => NULL,
   -      'accessConditionCollection' => 'access_variants',
   -      'executable' => NULL,
   -    ];
   -    foreach ($unset_vars as $unset_var => $configuration_key) {
   -      if (!empty($this->$unset_var)) {
   -        if ($configuration_key) {
   -          $this->set($configuration_key, $this->$unset_var->getConfiguration());
   -        }
   -        unset($vars[array_search($unset_var, $vars)]);
   -      }
   -    }
   +    // Avoid serializing the page executable as it represents runtime state.
   +    unset($vars[array_search('executable', $vars)]);
   

   Any insight here as to why we can change this?

7. +++ b/src/Entity/PageVariant.php
   @@ -207,6 +206,9 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   +      if (empty($this->variant_settings['uuid'])) {
   +        $this->variant_settings['uuid'] = $this->uuidGenerator()->generate();
   +      }
   

   This should be a separate issue.

8. +++ b/src/Entity/PageVariant.php
   @@ -232,17 +234,29 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   -    if (!$this->page) {
   +    $page = Page::load($this->page);
   +    if (!$page) {
          throw new \UnexpectedValueException('The page variant has no associated page');
        }
   -    return Page::load($this->page);
   +    return $page;
   

   This sorta makes sense, but it's also another problem because what if $this->page === NULL?

   This should be a separate issue.

9. +++ b/src/Entity/PageVariant.php
   @@ -232,17 +234,29 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   +    try {
   +      $page = $this->getPage();
   +    }
   +    catch (\UnexpectedValueException $e) {
   +      // This can happen adding a new page - it may only exist in the tempstore.
   +      // @todo Remove once contexts are stored only on the variant!
   +      $cached_values = $this->getTempstoreFactory()->get('page_manager.page')->get($this->page);
   +      if (!empty($cached_values) && !empty($cached_values['page'])) {
   +        $page = $cached_values['page'];
   +      }
   +    }
   

   Any reason not to put this into getPage()?

10. +++ b/src/Plugin/DisplayVariant/HttpStatusCodeDisplayVariant.php
    @@ -84,4 +84,11 @@ class HttpStatusCodeDisplayVariant extends VariantBase {
    +  public function __sleep() {
    +    $vars = parent::__sleep();
    +    unset($vars[array_search('stringTranslation', $vars)]);
    +    return $vars;
    +  }
    
    +++ b/src/Plugin/DisplayVariant/PageBlockDisplayVariant.php
    @@ -234,4 +247,10 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
    +  public function __sleep() {
    +    $vars = parent::__sleep();
    +    unset($vars[array_search('stringTranslation', $vars)]);
    +    return $vars;
    

    Why? Isn't that service, and therefore handled by the trait?

We postponed this to "Milestone 2" - so, other issues that could have depended on it that we need for "Milestone 1" are going to have to be done another way.

Re-rolling. This is a tricky one since a few things have changed and have been moved to page_manager_ui.

This is an attempt to manually re-roll the patch at #65. I couldn't finish rebasing 8.x-1.x on top of it because there were too many conflicts so I looked at the patch manually to understand the set of changes and compare them against HEAD.

Here are a few questions:

1. Many forms and other classes have been moved to page_manager_ui, but this patch adds a few extra classes that depend on them (for example, PageWizardBase references PageGeneralForm). What should we do?
2. Page and PageVariant don't define handlers in their annotations but instead they now do it through hook_entity_type_build(). How could we add the wizard handlers there?
3. Some of the tests have changed a lot. I managed to update most of them but I am sure that I missed stuff at PageNodeSelectionTest.php.

Aside form this, is there a chance to split this in smaller tasks? This patch is so big that it is very hard to keep it up to date.

IMO this should be a separate module called page_manager_wizard, and it should be installed *instead* of page_manager_ui, not in addition.

@tim.plunkett, thanks for the feedback. I discussed this with @EclipseGc and we agreed that since the patch relies on some classes that are in page_manager_ui, we will implement the wizard classes within page_manager_ui. I will see if it is possible to do this in small steps instead of submiting another big patch.

This looks much better now. Here I have:

* Moved Wizard files from page_manager to page_manager_ui (since they depend on page_manager_ui's forms).
* Removed the changes in the tests, as I first want to see this working locally and then I will update the tests.

I am about to start testing this manually and fix bugs in the patch.

Fixed a few things:
* Fix CSS library attachment
* Dynamic wizard handlers for Page.php
* Fix yaml format in routing

Now I am testing the UI. I am debugging why the node page can't be saved or updated. It looks like the page id is not being loaded from the temporary storage correctly. Will continue tomorrow.

Phew, this one was tricky, this line at Page::__sleep() was avoiding the Page's id to be saved in the shared storage:

unset($vars[array_search('executable', $vars)]);

The issue with the above line is that array_search() returned FALSE, which was evaluated as 0, which was the first key in the vars array. Now the Update and Save forms are working back again.

I will continue testing the patch manually and fixing bugs.

I am now looking at how to adjust the logic so the patch uses PageVariant::getStaticContexts() instead of Page::getStaticContexts(). This is something that changed at #2631800: Store static contexts on the variant rather than the page, which got committed after #65.

Fixed a few things:

* Fix wrong class path for VariantPluginContentForm (as forms were moved to page_manager_ui).
* Fix the type of PageVariant::contexts so it matches whith Page::contexts. This was causing an error in the UI.
* Fix further buggy statements at __sleep().

Now I am comparing 8.x-1.x against this patch to fix what I mentioned at #77.

The issue that I described at #77 was caused by some stalled data that I had at key_value_expire. I realised that in order to start over with this patch, I need to uninstall page_manager_ui and page_manager modules, and then run the following query:

delete from key_value_expire where collection like '%page_manager%'

Here I am fixing a bug where deleted variants were being listed at "Reorder variants" I don't really like the approach as the snippet was copied and pasted from another class. Should we move this to a trait?

I am done with manual testing so now I will start fixing the tests.

+++ b/src/Entity/PageVariant.php
@@ -117,9 +117,9 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
-  protected $contexts = NULL;
+  protected $contexts = [];

@@ -277,7 +277,7 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
-    $this->contexts = NULL;
+    $this->contexts = [];

This conflicts with the logic in getContexts(), which checks is_null

+++ b/page_manager_ui/src/Form/PageReorderVariantsForm.php
@@ -84,8 +84,17 @@ class PageReorderVariantsForm extends FormBase {
+        if (isset($variants[$page_variant->id()])) {
+          unset($variants[$page_variant->id()]);
+        }

unset() doesn't need an isset check first.

Fixed @tim.plunkett's feedback at #80.

Working on the tests now.

Fixed a few strict and critical errors so now tests complete running. I have started working on updating them so we should start seeing the number of failures going down in my subsequent patches.

Fixed half of PageManagerAdminTest.

Fixed errors for PageManagerAdminTest.php. Moving on to the other tests.

Had a quick look at PageManagerAdminTest and still getting a couple of fails (could be my dev environment but thought I'd check):

HTTP response expected 403, actual 200 | PageManagerAdminTest.php | 419 | Drupal\page_manager_ui\Tests\PageManagerAdminTest->doTestReorderVariants()

and

Page is rendered in bartik Value 0 is equal to value 1. | PageManagerAdminTest.php | 452 | Drupal\page_manager_ui\Tests\PageManagerAdminTest->doTestAdminPath()

are probably caching issues...

And then there are a couple of schema errors:

Uncaught PHP Exception Drupal\Core\Config\Schema\SchemaIncompleteException: "Schema errors for page_manager.page_variant.block_page with the following errors: page_manager.page_variant.block_page:variant_settings.blocks.058ec778-96c8-4ce6-9500-e4c7a4501b9d.level missing schema, page_manager.page_variant.block_page:variant_settings.blocks.058ec778-96c8-4ce6-9500-e4c7a4501b9d.depth missing schema" at /var/www/html/b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php line 98

It's weird @yanniboi, the test passed at the testbot. Are you using Drupal core 8.2.x, and the latest HEAD of ctools and page manager?

Nevermind, I just updated core, and all the errors went away :P

8.2.x? No I was still on 8.0.x :P

I have noticed a regression in adding/editing variants.

Currently as I understand it the only variant plugin that the new UI supports is the Block Page. In the process of adding support for 'Panels' I have come across the following.

Prior to the Page Manager Wizard UI, there was a 2 step process for adding a variant to a page.

1. Click on 'Add variant'. We are presented with a list of variant plugins to choose from (eg. Block Page/Panels/etc)
2. Configuring variant. We now have an entity add form where we can add a new variant and fill in information required by it, eg. Layout.

The new UI in only has a single form, which is a select list of plugins and a label. However more configuration is needed for different variant plugin types, eg. Panels requires at least the selection of a layout before it can be created, otherwise when you come to edit the plugin it throws errors.

I think the solution is to move towards an add/edit variant wizard where different variant plugin types can specify additional operations, eg. Layout, Selection Rules, etc.

This patch updates StaticContextTest.php and PageTest.php.

I discussed with @EclipseGc the StaticContextTest.php errors and found out that the contexts are for variants and not the page entity. I will now work on making the following change in the UI and classes:

@andrewbelcher found a piece of logic in page_manager that skips simple_page and block_page variants. I showed this to @EclipseGc yesterday and he agreed on doing the same in the Wizard API. I am posting this here as a remainder and I will add it to the patch once I am done with #93.

<andrewbelcher> juampynr: noticed one thing missed switching to the wizard: \Drupal\page_manager_ui\Controller\PageManagerController::selectVariant() filtered out simple_page and block_page from the available variant plugin ids - the new \Drupal\page_manager_ui\Form\PageGeneralForm::buildForm() doesn't do the same
<andrewbelcher> <?php
<andrewbelcher>       foreach ($this->variantManager->getDefinitions() as $plugin_id => $definition) {
<andrewbelcher>         // The following two variants are provided by Drupal Core. They are not
<andrewbelcher>         // configurable and therefore not compatible with Page Manager but have
<andrewbelcher>         // similar and confusing labels. Skip them so that they are not shown in
<andrewbelcher>         // the UI.
<andrewbelcher>         if (in_array($plugin_id, ['simple_page', 'block_page'])) {
<andrewbelcher>           continue;
<andrewbelcher>         }
<andrewbelcher>         $variant_plugin_options[$plugin_id] = $definition['admin_label'];
<andrewbelcher>       }

To make that a little more readable/copyable:

      foreach ($this->variantManager->getDefinitions() as $plugin_id => $definition) {
        // The following two variants are provided by Drupal Core. They are not
        // configurable and therefore not compatible with Page Manager but have
        // similar and confusing labels. Skip them so that they are not shown in
        // the UI.
        if (in_array($plugin_id, ['simple_page', 'block_page'])) {
          continue;
        }

        $variant_plugin_options[$plugin_id] = $definition['admin_label'];
      }

Just did a hangout with @yanniboi and @andrewbelcher. We discussed the following:

Andrew needs to be able to define dynamic form steps in the wizard. @dsnopek pointed him to #2687617: Dynamic steps when only the first is certain don't work

@yanniboi needs feedback on the issue that he described at #92 when adding/editing variants.

@andrewbelcher also mentioned that there seems to be something wrong with the finish method. In theory, it should be called just when you click on Save or Update and Save, so it saves both the Page and its Variants. However, it seems that this is not working this way.

Here is the first step forward for #93. Here I have:

• Changed Page Contexts to Page Parameters.
• Moved Contexts to each variant.

There are a few more things that I need to adjust for this to be fully functional. Here is a screenshot of how it looks like:

Here I have added @andrewbelcher's suggestion at #96, where core variants are excluded as they are not configurable.

I am figuring out what is wrong with tempstores because when you add a context to a variant it does not get listed.

@andrewbelcher shared with me in a hangout a discussion on managing tempstores that he had with @EclipseGc. Here is the summary:

The forms in this patch define separate tempstores, which leads to inconsistencies. There should be just one tempstore per page. Its contents are what should be passed to Wizard forms so they can update it. I started working on this yesterday so my plan is to complete the work this morning (Spanish time) and submit a new patch).

Fixed the issue with variant contexts not showing up. It wasn't an issue with tempstores but the logic not loading them properly.

@andrewbelcher, I have not made the change of making wizard forms to receive cached values instead of the tempstore id because the logic that I have reviewed so far seems to be working fine. I don't want to make too many changes on the patch until we get tests passing: then we can make improvements on it.

@yanniboi created a snippet to add a cancel button, which I will add in a following patch https://gist.github.com/yanniboi/65ac8a701eb5ca3c6c28.

Added the Cancel button suggested by @yanniboi at #103.

I am reviewing the rest of the forms to make sure that they use the correct tempstore.

Few fixes in this patch:

* Fixed context routes as they now belong to the variant and not the page.
* I removed the edit operation from Page parameters. Is this correct? See the following screenshot:

Now that contexts are working, I will get back to fix the tests.

@juampynr On page parameters you should be able to edit them so that you can select the type for the context (or none), but you shouldn't be able to add or remove them. In variant context you should be able to add/remove/edit (but type can't be none as you would remove).

@juampynr here is a micro patch to merge into yours that fixes the Page::toUrl('edit-form') (and removes an override on the list builder that worked round the problem).

Thanks @andrewbelcher! This patch adds your suggestion.

I am debugging a weird redirection that happens after adding a context and clicking Update (reported by @EclipseGc). Then I will continue fixing tests.

Fixes PageVariantTest.php.

Fixed parameter editing, as described by @andrewbelcher at #108. I made it possible to edit them at Page Parameters. The ParameterEditForm form was not prepared to use the temporary store so I updated it.

I see that some of the form classes could share a few methods to manage the temporary store, but for the moment I want to make the minimum amount of chances until tests pass. Once tests pass, we can start refactoring.

Please do not mistake this review as an endorsement of this issue.

Also I ignored all changes to page_manager_ui. This should still be a separate module. I only reviewed page_manager changes.

1. +++ b/src/Entity/Page.php
   @@ -139,6 +154,13 @@ class Page extends ConfigEntityBase implements PageInterface {
   +    // Save the page variants just in case they were modified (or added) and
   +    // not saved independently.
   +    foreach ($this->getVariants() as $page_variant) {
   +      $page_variant->save();
   +    }
   

   This is not in scope. If it is necessary, it should be in its own issue (with test coverage) that blocks this.

2. +++ b/src/Entity/Page.php
   @@ -300,7 +322,7 @@ class Page extends ConfigEntityBase implements PageInterface {
   -    if (!$this->contexts) {
   +    if (empty($this->contexts)) {
   

   Same as above

3. +++ b/src/Entity/Page.php
   @@ -309,8 +331,21 @@ class Page extends ConfigEntityBase implements PageInterface {
   +  public static function preDelete(EntityStorageInterface $storage, array $entities) {
   +    parent::preDelete($storage, $entities); // TODO: Change the autogenerated stub
   +  }
   

   Ahem.

4. +++ b/src/Entity/PageVariant.php
   @@ -116,9 +117,9 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   -   * @var \Drupal\Component\Plugin\Context\ContextInterface[]|null
   +   * @var \Drupal\Component\Plugin\Context\ContextInterface[]
   ...
   -  protected $contexts = NULL;
   +  protected $contexts = [];
   
   @@ -259,7 +292,7 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   -    $this->contexts = NULL;
   +    $this->contexts = [];
   

   Any explanation for this?

5. +++ b/src/Entity/PageVariant.php
   @@ -235,21 +246,43 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   -    if (!$this->page) {
   +    $page = Page::load($this->page);
   +    if (!$page) {
   ...
   -    return $this->getPageStorage()->load($this->page);
   

   This change makes no sense. Also using Page::load is a step backwards.

6. +++ b/src/Entity/PageVariant.php
   @@ -235,21 +246,43 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   -      $this->contexts = array_merge($static_contexts, $page_contexts);
   ...
   +    return array_merge($page->getContexts(), $this->loadContexts());
   

   This order seems to be backwards. It probably broke a unit test somewhere.

7. +++ b/src/Entity/PageVariant.php
   @@ -235,21 +246,43 @@ class PageVariant extends ConfigEntityBase implements PageVariantInterface {
   +      // @todo Remove once contexts are stored only on the variant!
   

   All @todos should have issues, unless they are to be fixed before commit, in which case you can use @fixme

8. +++ b/src/Plugin/DisplayVariant/HttpStatusCodeDisplayVariant.php
   @@ -84,4 +84,13 @@ class HttpStatusCodeDisplayVariant extends VariantBase {
   +  public function __sleep() {
   +    $vars = parent::__sleep();
   +    $key = array_search('stringTranslation', $vars);
   +    if ($key !== FALSE) {
   +      unset($vars[$key]);
   +    }
   +    return $vars;
   +  }
   
   +++ b/src/Plugin/DisplayVariant/PageBlockDisplayVariant.php
   @@ -300,4 +313,13 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
   +  public function __sleep() {
   

   This should not be necessary, the parent trait should handle it.

9. +++ b/src/Plugin/DisplayVariant/PageBlockDisplayVariant.php
   @@ -248,6 +249,18 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
   +  public function getWizardOperations($cached_values) {
   

   This seems like it would be better as an annotation.

10. +++ b/src/Plugin/DisplayVariant/PageBlockDisplayVariant.php
    @@ -248,6 +249,18 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
    +        'form' => '\Drupal\page_manager_ui\Form\VariantPluginContentForm',
    

    Use VariantPluginContentForm::class

11. +++ b/src/Tests/PageTestHelperTrait.php
    @@ -13,13 +13,6 @@ namespace Drupal\page_manager\Tests;
    -   * @see \Drupal\simpletest\TestBase::$container
    -   *
    -   * @var \Symfony\Component\DependencyInjection\ContainerInterface
    -   */
    -  protected $container;
    

    Now the methods in this trait refer to a property that doesn't exist.

12. +++ b/src/Tests/StaticContextTest.php
    @@ -49,22 +49,15 @@ class StaticContextTest extends WebTestBase {
    +    $this->drupalGet('admin/structure/page_manager/add');
    ...
    -    $this->drupalPostForm('admin/structure/page_manager/add', $edit_page, 'Save');
    ...
    +    $this->drupalPostForm(NULL, $edit_page, 'Next');
    

    This could have just changed one line, not 3

13. +++ b/tests/src/Unit/PageTest.php
    @@ -49,10 +49,12 @@ class PageTest extends UnitTestCase {
    -    $variant1->getWeight()->willReturn(0);
    ...
    +    $variant1->weight = 0;
    ...
    -    $variant2->getWeight()->willReturn(-10);
    ...
    +    $variant2->weight = -10;
    

    That not how mocks are supposed to work.

14. +++ b/tests/src/Unit/PageTest.php
    @@ -49,10 +49,12 @@ class PageTest extends UnitTestCase {
    +    $variant1->label()->willReturn('Variant 1');;
    ...
    +    $variant2->label()->willReturn('Variant 2');;
    

    Double semi-colon.

15. +++ b/tests/src/Unit/PageVariantTest.php
    @@ -49,12 +50,17 @@ class PageVariantTest extends UnitTestCase {
    +    $entity_manager = $this->prophesize(EntityManagerInterface::class);
    ...
    +    $container->set('entity.manager', $entity_manager->reveal());
    

    The entity manager is deprecated. Remove all code referencing it.

16. +++ b/tests/src/Unit/PageVariantTest.php
    @@ -49,12 +50,17 @@ class PageVariantTest extends UnitTestCase {
    +    $entity_manager->getEntityTypeFromClass('Drupal\page_manager\Entity\Page')->willReturn('page');
    

    Use Page::class

17. +++ b/tests/src/Unit/PageVariantTest.php
    @@ -70,8 +76,6 @@ class PageVariantTest extends UnitTestCase {
    -    $contexts = $this->pageVariant->getContexts();
    -    $this->assertSame($expected, $contexts);
    

    Why would you remove test coverage from an API test in a UI only issue?

18. +++ b/tests/src/Unit/PageVariantTest.php
    @@ -84,12 +88,12 @@ class PageVariantTest extends UnitTestCase {
    -      ['static' => 'static', 'page' => 'page'],
    +      ['page' => 'page', 'static' => 'static'],
    ...
    -      ['foo' => 'page'],
    +      ['foo' => 'static'],
    

    Same as above. No functional tests should be altered for UI only changes.

Thanks for the feedback @tim.plunkett. I would like to jump into a hangout with you and @EclipseGc to discuss the scope of this issue. I am all in for reducing it. Here are some responses:

1. Fine for me. Removed it. I am not creating an issue yet as I don't see under which scenario this would be needed since the variant data is stored in temporary store until the user clicks Save or Update and Save.

2. I changed this to empty() because the default value of the contexts array should be [] and not NULL. This is how other classes such as ContextAwarePluginBase or ConditionPluginCollection do it. Am I missing something? Also, having a NULL value by default was causing an error after adding a variant.

3. Removed. Does nothing.

4. Explained at point number 2.

5. Removed change.

6. I changed the order intentionally so when you edit a variant, Page parameters are listed first and then variant contexts second. See the following screenshot:

This makes more sense to me but I may be missing something. Can you confirm? This is why I made adjustments at the related unit test.

7. Removed.

8. I couldn't find any of the parents doing this. I will leave it there for the moment.

9. Noted. I will change it once I get tests passing.

10. Changed.

11. Restored property.

12. Adjusted.

13. The patch was already like this and I am not sure at what the change is attempting to do. I have removed it and now the test fails so I will investigate what is wrong with it.

14. Fixed.

15. Removed. It was needed due to the Page::load() statement removed in point 5.

16. That line was removed in point 15.

17. The method changed now that it pulls data from tempstore and merges contexts from the Page and the Variant. I have restored the test though because I need to understand better Unit tests to adjust it.

18. The patch modifies the method, hence the change in the test.

I am now debugging the following error when you open the Content section of a variant:

LogicException: Settings can not be serialized. This probably means you are serializing an object that has an indirect reference to the Settings object. Adjust your code so that is not necessary. in Drupal\Core\Site\Settings->__sleep() (line 67 of core/lib/Drupal/Core/Site/Settings.php).

@Juampy

8. HttpStatusCodeDisplayVariant extends VariantBase extends PluginBase which uses the DependencySerializationTrait. DependencySerializationTrait provides the __sleep() method.

1. We can do this manually in our wizard for now with an @todo and a separate issue to do it upstream, pending agreement.

6. the reason for that order is precedence. In array_merge() the latter override keys in the former. If there were duplicated keys, page should always take precedence. To solve the order, I would suggest switching to $page_contexts + $static_contexts as this will give you the right order and precedence.

This could easily be done in a separate issue and we can ignore the impact on UI in the mean time. Could probably do with test coverage for both order and precedence...

I think generally I agree with @tim.plunkett that we shouldn't be making changes to things in page manager (as opposed to UI) and things that do need changing should be in separate issues.

On that note, I've just realised my patch in #109 introduces UI code into p_m. I'll take a look tomorrow and see if I can find a way around that...

Got it now: TypedData.php is the class that implements the method that sets the serviceIds (stringTranslation being one of them). When serializing, serviceIds are unset(). Thanks @tim.plunkett and @yanniboi! This patch removes a couple instances that use it.

I am still debugging this weird error:

LogicException: Settings can not be serialized. This probably means you are serializing an object that has an indirect reference to the Settings object. Adjust your code so that is not necessary. in Drupal\Core\Site\Settings->__sleep() (line 67 of core/lib/Drupal/Core/Site/Settings.php).

@andrewbelcher, I will look at your feedback at #119 for merging page and variant contexts in a bit.

Restoring #115-1 as otherwise page variants don't get saved when you click on Update and save or Save.

@juampynr Rather than restoring that, you could do something like this in \Drupal\page_manager_ui\Wizard\PageWizardBase::finish:

  public function finish(array &$form, FormStateInterface $form_state) {
    parent::finish($form, $form_state);

    $cached_values = $form_state->getTemporaryValue('wizard');
    /** @var \Drupal\page_manager\Entity\Page $page */
    $page = $cached_values['page'];
    foreach($page->getVariants() as $variant) {
      $variant->save();
    }
  }

I would also be inclined to change \Drupal\page_manager_ui\Wizard\PageEditWizard::finish to:

  public function finish(array &$form, FormStateInterface $form_state) {
    // Delete any of the variants marked for deletion.
    $cached_values = $form_state->getTemporaryValue('wizard');
    /** @var \Drupal\page_manager\Entity\Page $page */
    $page = $cached_values['page'];
    if (!empty($cached_values['deleted_variants'])) {
      foreach ($cached_values['deleted_variants'] as $page_variant) {
        $page->removeVariant($page_variant->id());
      }
    }

    parent::finish($form, $form_state);
  }

Even if we move the first bit into \Drupal\page_manager\Entity\Page::postSave, the second bit deletes the variants before they are saved by either parent::finish() or Page::postSave(). Using ::removeVariant() is necessary as it removes it from the Page::$variants property, but that is also better as it means other things wont find it (and potentially re-save it) by calling Page::getVariants(). Even better still would be to store only the variant id in $cached_values['deleted_variants'] rather than the whole variant object, as that's also a bit unnecessary.

Fixed a couple things here:

* Errors when adding and editing contexts for a variant (the variant machine_name was not being passed on).
* Restored the __sleep() method for BlockDisplayVariant as it is not been called as we expected at #120. Without this, a block variant with an entity:node context that points to a node crashes when you click on "Content", as it tries to store stringTranslation into the tempstore.

Here I have adjusted the finish() methods based on @andrewbelcher's suggestion at #124. I just had to make a small adjustment at page::removeVariant() for this to work. Thanks @andrewbelcher!

@juampynr: Got two patches here.

update_for_2690047-2550879.patch is the fix for fatal errors on the add wizard (due to unsaved page) and is dependent on #2690047: Allow setting the Page on a PageVariant. Should solve lots of issues in the add process - particularly around context as that merges in things from the parent page.

filter_core_variants-2550879.patch applies the fix you've already done elsewhere to exclude the core display variant plugins from the selection - think this instance got missed.

@jaumpy, did you remove this at some point? We dont want this anymore :)

+++ b/page_manager_ui/src/Form/PageParametersForm.php
@@ -0,0 +1,129 @@
+    /** @var \Drupal\Core\StringTranslation\TranslatableMarkup $add */
+    $add = $form_state->getValue('add');
+    if ($add->getUntranslatedString() == 'Add new context') {
+      list(, $route_parameters) = $this->getOperationsRouteInfo($cached_values, $this->machine_name, $form_state->getValue('types'));
+      $form_state->setRedirect($this->getAddRoute($cached_values), $route_parameters);
+    }

So a few bits that I've noticed (I'm on latest but one patch with my add fixes applied - though from the interdiff I don't think anything touches these):

1. I think we normally only capitalise the first work (e.g. Page access rather than Page Access).
2. Page access should come after Page parameters, as you may want to make use of those configured parameters.
3. Page parameters only shows current user, which isn't in the path. This page should only show items from the path (which it doesn't show).
4. Submitting Page parameters gave me:
   

   PHP Fatal error:  Call to a member function getUntranslatedSt
   ring() on null in modules/page_manager/page_manager_ui/src/Form/PageParametersForm.php on line 94

I think some of these also apply to the edit wizard.

Ok, so my patch replaces the Add variant form on a Page with the Add variant wizard.

Changes:

1. Add variant and add variant step routes.
2. Selection Criteria and Context steps of the variant wizard are optional
3. Page Variant ids are always programatically generated using a "{$page->id()}-{$variant_plugin->id()}-{$delta}" pattern"
4. Both the page add wizard and the variant add wizard share the same PageVariantConfigureForm step form.
5. Updated PageManagerAdminTest to be aware of the currently required 'parameters' step.
6. Updated PageManagerAdminTest to be aware of the variant wizard steps and variant ids.