Wizard API integration

So I think what we need to do is be able to trigger a wizard when the layout is changed because we need to be able to reassign blocks to new regions.

I will have a look at this.

Ok, so I have been working on this, and my patch is in no way complete (in fact there is probably loads wrong, but I'm sharing my work in progress), but I have run into a few difficulties that I need help with.

I am attaching a few screenshots to help.

When in the page edit wizard, looking at the layout set of the panels variant, I have removed settings from this step and added a button to trigger a 'change layout' wizard.

This currently has 2 steps, 'settings' and 'regions'.

Issues:

• Regions table drag: Isnt working as expected. I can drag regions form to old layout to be placed in a new layout region, but it isnt properly updating the select, and vice versa
• Tempstore update: I haven't got the tempstores being updated properly yet on finishing the layout change.
• Variant add wizard: How do we handle the variant wizard and the layout change wizard separately? Currently I have broken the ability to add a variant to a page because the layout form has changed.

I also need to merge @Juampy's patch into mine because we started at slightly separate steps...

Hi! This is my first contact with panels for D8.

1) I'm learning how Wizard API works and I think that we could have 2 different forms for the layout. The first one to add a new variant and a second one to edit the variant.
So in this case we can handle separately...

Does it make sense?

The attached patch is an example.

2) We could use a new wizard form for the edit form but I have no idea about how to add this wizard form inside a wizard form...

Oops Adding the forms here...

Ok! Let's try again:

1) I'm creating a Base Form that could be extended by the Edit and Add Forms. So, these forms are different and I'm overriding the buildForm() from the Edit to add a new custom button (Change Layout).

2) I don't know if this is the best way... but for the moment using "user.private_tempstore" to store the values when loading the form to change the layout.

3) Creating a new route to obtain the substep.

4) Detecting if the current path is a substep and then adding this form to the operation when loading the wizard. We can add different steps/forms.

+      'edit_layout' => [
+        'title' => $this->t('Layout'),
+        'form' => LayoutEditPluginSelector::class,
+        'op' => 'edit',
+        'step' => [
+          'move' => [
+            'form' => LayoutChangePluginSelector::class,
+          ],
+        ],
+      ],

To be honest I have no idea if this is the correct way but for the moment it works. Needs test and probably refactor but for the moment needs review :-)

Apologies to marthinal, my patch is based on an older version of the patch so we need to make sure this code can all be merged. I'm just posting this out there for the masses for the time being.

Main fix here is to remove PageManager specific code in the submit handler of the Layout step. For future reference (because there's likely more of this code that hasn't bit me yet) the objective is to update the cached_values['plugin'] object with new configuration. PM (and Panelizer or others) will extract the values they store from that object.

No interdiff since I'm totally doing a runby-patching. I think this was based on 11, and I'll get an interdiff together after I'm back from vacation.

Eclipse

Comments about #14.

// This is totally personal opinion and likely has no weight.
I am not sure having sub_steps is the right way to go. We should be able to do the same thing using the single step routing.

#15 seems like its on the right path but I am getting an error:

PHP Fatal error: Call to a member function getTempstoreId() on a non-object in /modules/contrib/panels/src/Form/LayoutPluginSelector.php

get('wizard') is returning null.

re: 17

Did you upgrade to the newest CTools alpha and/or checkout the newest code from git? I'm not having the same problem and would love steps to reproduce.

Eclipse

nvm, found it.

Things worth noting:

1. Added some routes here. I've not gotten access squared away on that yet, so you need PM installed and the 'administer pages' permission to mess with it just yet. That shouldn't be a huge burden since they can't be tested any other way currently.
2. Killed the specific version requirement on CTools. This is for testing purposes only currently and we can discuss what the appropriate ctools version would be. I'm using HEAD.
3. This patch introduces a new plugin type called a Panels Pattern. This plugin provides customizations to the context stack and the ability to provide custom url objects for the normal block placement system.
4. Cached values are super easy to get when you're IN the wizard, but the second you step out of the wizard it can be quite difficult to get from a non-wizard route. PageManager specifically complicates this with the variant approach, so I've introduced a CachedValuesGetterTrait for Panels related block placement forms. This standardizes (as all implementations should) on the 'plugin' key in the cached values array and makes some additional checks to see if we're in a page_manager wizard or not. I expect Panelizer and MiniPanels can both easily use this. I'll be attempting to prove that tomorrow with Panelizer.
5. The layout change forms should conditionally appear within the greater wizard now and they should actually work. YAY
6. PanelsContentForm replaces the traditional dependency on PageManager's block placement form. This Panelizer patch #2664682: Implement admin UI for editing Panelizer defaults has versions of this and all the block add/edit/delete forms which have all been ported into this patch. That should reduce the size of the Panelizer patch and remove the hard dependency we had on PageManager for a bit. Likewise the new version of this form does not have a custom tempstore, so it should appropriately reflect changes made to layout amongst other frustrations that have been eliminated.
7. On that same point, the blocks should now be able to be dragged from one region to another and work reliably.
8. Display Builder plugins may now provide their own wizard steps into the process.
9. Panels Displays now have getter/setter methods for the Pattern plugins

TODOS:

1. Need to remove the old layout change wizard

Use HEAD of both ctools and page_manager for best results on this.

Eclipse

This patch does not address 21 at all but is a set of minor changes to help support panelizer & mini-panels style approaches as well as removing the change layout wizard class and route.

Eclipse

Updated the getBuilder() method to include a fallback to 'standard'.

Updated \Drupal\panels\Plugin\DisplayVariant\PanelsDisplayVariant::buildConfigurationForm to always display the builder form elements. This nullified the need for the form alter I added.

Eclipse

Nice, #26 fixed my Page Manager (dev) content section. It just didn't exist and I could not add any blocks.

https://www.drupal.org/node/2731877

Removed non-essential layout related methods from the PanelsDisplayVariant class.
Moved variant title to the content page so that it happens AFTER contexts are created.

This oughta fix the tests.

EDIT: ...locally, that is. They pass against the latest HEAD of Page Manager.

New in #30:

+++ b/src/Tests/PanelsTest.php
@@ -74,22 +81,12 @@ class PanelsTest extends WebTestBase {
-    // Check the default value and change a layout setting.
-    $this->assertText('Blah');
-    $this->assertFieldByName("variant_settings[layout_settings][setting_1]", "Default");
-    $edit = [
-      'variant_settings[layout_settings][setting_1]' => 'Abracadabra',
-    ];
-    $this->drupalPostForm(NULL, $edit, 'Save');
-
-    // Go back to the variant edit form and see that the setting stuck.
-    $this->drupalGet('admin/structure/page_manager/manage/foo/variant/panels_1');
-    $this->assertFieldByName("variant_settings[layout_settings][setting_1]", "Abracadabra");

This is removing the test code that verifies that we can change the settings on layouts with settings! We should really keep this in some form rather than remove it...

@dsnopek: Ordinarily I would agree, but at the moment the wizard workflow does not support configuring the layout settings, which is why that code breaks the tests, and why I removed it. I could certainly comment it out rather than outright delete it, though...

@EclipseGc, confirm?

So, this patch removes the ability to configure layout settings? Then that needs to get fixed! The failing tests are a pretty good reminder for us. :-)

If not, this'll render any layouts with settings unusable, and we've supported layouts with settings for a long, long time and people are using it. The Display Suite layouts (which will appear in Panels) have layout settings, and will render invalid HTML (since they allow selecting the tags used, ie. div, article, section, etc) if they can't save their settings. I believe some of the contrib modules that just add layouts also use settings. This really great video tutorial creates a layout with settings. So, we gotta support this.

1. +++ b/src/Annotation/PanelsPattern.php
   @@ -0,0 +1,15 @@
   +/**
   + * @Annotation
   + */
   +class PanelsPattern extends PluginID {}
   

   The plugin type needs to be documented here.

2. +++ b/src/CachedValuesGetterTrait.php
   @@ -0,0 +1,29 @@
   +  protected function getCachedValues(SharedTempStoreFactory $tempstore, $tempstore_id, $machine_name) {
   

   Missing a sorely needed doc comment.

3. +++ b/src/CachedValuesGetterTrait.php
   @@ -0,0 +1,29 @@
   +    list($machine_name, $variant_id) = explode('--', $machine_name);
   

   This is causing PHP notices (offset 1 not found), which leads me to believe that the machine name should be sanity checked before being split out.

4. +++ b/src/Controller/Panels.php
   @@ -0,0 +1,162 @@
   +  public function addBlock($tempstore_id, $machine_name, $plugin_id) {
   +
   +  }
   

   Is this supposed to be empty? Also, missing doc comment.

5. +++ b/src/Form/LayoutChangeRegions.php
   @@ -0,0 +1,226 @@
   +  /**
   +   * @var \Drupal\layout_plugin\Plugin\Layout\LayoutPluginManagerInterface
   +   */
   +  protected $manager;
   +
   +  /**
   +   * @var \Drupal\user\SharedTempStoreFactory
   +   */
   +  protected $tempstore;
   

   Missing doc comments.

6. +++ b/src/Form/LayoutChangeRegions.php
   @@ -0,0 +1,226 @@
   +  public function __construct(LayoutPluginManagerInterface $manager, SharedTempStoreFactory $tempstore) {
   

   Needs a doc comment.

7. +++ b/src/Form/LayoutPluginSelector.php
   @@ -0,0 +1,135 @@
   +  /**
   +   * @var \Drupal\layout_plugin\Plugin\Layout\LayoutPluginManagerInterface
   +   */
   +  protected $manager;
   +
   +  /**
   +   * @var \Drupal\user\SharedTempStoreFactory
   +   */
   +  protected $tempstore;
   

   Missing descriptions.

8. +++ b/src/Form/LayoutPluginSelector.php
   @@ -0,0 +1,135 @@
   +  public function __construct(LayoutPluginManagerInterface $manager, SharedTempStoreFactory $tempstore) {
   

   Missing doc comment.

9. +++ b/src/Form/LayoutPluginSelector.php
   @@ -0,0 +1,135 @@
   +    $options = [];
   +    foreach ($this->manager->getDefinitions() as $plugin_id => $definition) {
   +      $options[$plugin_id] = $definition['label'];
   +    }
   

   It doesn't look like $options is ever used in this method except for this.

10. +++ b/src/Form/LayoutPluginSelector.php
    @@ -0,0 +1,135 @@
    +    if ((string)$form_state->getValue('op') == $this->t('Change Layout')) {
    

    The result of $this->t() is what should get the string cast; $form_state->getValue('op') will return a string natively, I think.

    Alternatively, why not compare op in $form_state to $form['update_layout']['#value'], so that it doesn't need to be repeated twice?

11. +++ b/src/Form/LayoutPluginSelector.php
    @@ -0,0 +1,135 @@
    +      $variant_plugin->setLayout($form_state->getValue('layout'), $form_state->getValue('layout_settings') ? $form_state->getValue('layout_settings') : []);
    

    The final argument can be shortened to $form_state->getValue('layout_settings') ?: [] for brevity.

12. +++ b/src/Form/PanelsAddBlockForm.php
    @@ -0,0 +1,81 @@
    +   * Constructs a new VariantPluginFormBase.
    

    Wrong class name.

13. +++ b/src/Form/PanelsBlockConfigureFormBase.php
    @@ -0,0 +1,203 @@
    +      '#options' => $this->getVariantPlugin()->getRegionNames(),
    +      '#default_value' => $this->getVariantPlugin()->getRegionAssignment($this->block->getConfiguration()['uuid']),
    

    Why aren't these using $this->variantPlugin?

14. +++ b/src/Form/PanelsContentForm.php
    @@ -0,0 +1,253 @@
    +  /**
    +   * @var string
    +   */
    +  protected $tempstore_id;
    

    Needs a description.

15. +++ b/src/Form/PanelsDeleteBlockForm.php
    @@ -0,0 +1,116 @@
    +  /**
    +   * Get the tempstore.
    +   *
    +   * @return \Drupal\user\SharedTempStoreFactory
    +   */
    +  protected function getTempstore() {
    +    return \Drupal::service('user.shared_tempstore');
    +  }
    

    This oughta be injected.

16. +++ b/src/Form/PanelsDeleteBlockForm.php
    @@ -0,0 +1,116 @@
    +    return \Drupal::request()->attributes->get('destination');
    

    This should also be injected.

17. +++ b/src/PanelsPatternManager.php
    @@ -0,0 +1,23 @@
    +  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
    +    $this->alterInfo('panels_pattern_info');
    +    $this->setCacheBackend($cache_backend, 'panels_pattern_plugins');
    +
    +    parent::__construct('Plugin/PanelsPattern', $namespaces, $module_handler, 'Drupal\panels\Plugin\PanelsPattern\PanelsPatternInterface', '\Drupal\panels\Annotation\PanelsPattern');
    

    This needs a doc comment!

18. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
    @@ -212,6 +219,37 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
       /**
    +   * @return \Drupal\panels\Plugin\PanelsPattern\PanelsPatternInterface
    +   */
    +  public function getPattern() {
    

    Needs a description in the doc comment.

19. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
    @@ -212,6 +219,37 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
    +  public function setPattern($pattern) {
    

    Needs a doc comment.

20. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
    @@ -212,6 +219,37 @@ class PanelsDisplayVariant extends BlockDisplayVariant {
    +    if ($pattern instanceof PanelsPatternInterface) {
    +      $this->pattern = $pattern;
    +      $this->configuration['pattern'] = $pattern->getPluginId();
    +    }
    +    elseif (is_string($pattern)) {
    +      $this->pattern = NULL;
    +      $this->configuration['pattern'] = $pattern;
    +    }
    +    else {
    +      throw new \Exception("Pattern must be a string or PanelsPatternInterface object");
    +    }
    

    It makes sense here to simply set $this->pattern to a plugin instance, if $pattern is a string -- that way, if the string is invalid, the pattern manager will throw a PluginNotFoundException and make things nice and clear if a garbage $pattern was passed in.

21. +++ b/src/Plugin/PanelsPattern/DefaultPattern.php
    @@ -0,0 +1,79 @@
    +  public function getMachineName(array $cached_values) {
    

    Needs a doc comment.

22. +++ b/src/Plugin/PanelsPattern/DefaultPattern.php
    @@ -0,0 +1,79 @@
    +  public function getDefaultContexts(SharedTempStoreFactory $tempstore, $tempstore_id, $machine_name) {
    

    Also needs a doc comment.

23. +++ b/src/Plugin/PanelsPattern/DefaultPattern.php
    @@ -0,0 +1,79 @@
    +    return !empty($cached_values['contexts']) ? \Drupal::service('ctools.context_mapper')->getContextValues($cached_values['contexts']) : [];
    

    Ideally, the context mapper should be injected.

24. +++ b/src/Plugin/PanelsPattern/DefaultPattern.php
    @@ -0,0 +1,79 @@
    +  public function getBlockListUrl($tempstore_id, $machine_name, $region = NULL, $destination = NULL) {
    

    Doc comment needed.

25. +++ b/src/Plugin/PanelsPattern/DefaultPattern.php
    @@ -0,0 +1,79 @@
    +  public function getBlockAddUrl($tempstore_id, $machine_name, $block_id, $region = NULL, $destination = NULL) {
    

    Doc comment.

26. +++ b/src/Plugin/PanelsPattern/DefaultPattern.php
    @@ -0,0 +1,79 @@
    +  public function getBlockEditUrl($tempstore_id, $machine_name, $block_id, $destination = NULL) {
    

    Ditto for the rest of these methods.

27. +++ b/src/Plugin/PanelsPattern/PanelsPatternInterface.php
    @@ -0,0 +1,72 @@
    +interface PanelsPatternInterface extends PluginInspectionInterface {
    

    All the methods of this interface are missing descriptions of what they do. Kind of a big problem for an interface :)

Ok, this patch fleshes out the layout settings/region remapping significantly and makes them actually work. Need to get the test coverage for that back and operational.

Interesting points:

Pretty much all layout plugins extend from LayoutBase which implements ConfigurablePluginInterface and PluginFormInterface. This makes it really hard to tell which layouts are actually configurable, so you will actually find some reflection in the determination of what steps we provide. This is the only way to reliably allow layout settings to be available as their own step. I think we can probably bikeshed whether it should be its own step, but it was the most effective way of supporting custom layout regions which are configurable imo.

The region remapping works and even has some validation, but for some reason when the page loads for the first time, regions which DO map to an existing region (by exact name match) don't appear there. I was unable to figure out why.

Overall, this should be nailing down the functionality we want/need and now we just need to make sure we like the format. I'm not yet convinced that I do. Remapping, in particular should perhaps happen in a dedicated modal or something and the wizard steps don't autoforward through the layout process on change, which feels like a bug (though it's intention in PM edit scenarios so we need to figure out what we want).

I'm pretty happy with how this is coming along, all the discrete parts are present, now lets test them and get them organized properly.

Eclipse

1. +++ b/src/Form/LayoutChangeRegions.php
   @@ -131,49 +138,42 @@ class LayoutChangeRegions extends FormBase {
   +          '#default_value' => isset($new_regions[$region]) ? $region : 'unassigned',
   

   What happens if the new layout has a region called 'unassigned'? (Unlikely, but possible). Maybe this should be something like __unassigned__ or similar, to decrease the possibility of a collision.

2. +++ b/src/Form/LayoutChangeSettings.php
   @@ -91,12 +102,52 @@ class LayoutChangeSettings extends FormBase {
   +          if (substr($next_params['step'], 0 -7) == 'regions') {
   

   Why are we doing substr() here? I'm guessing there's a damn good reason, but it needs a comment to explain.

3. +++ b/src/Form/LayoutChangeSettings.php
   @@ -91,12 +102,52 @@ class LayoutChangeSettings extends FormBase {
   +        if (substr($next_params['step'], 0 -7) == 'regions') {
   +          $cached_values['layout_change']['layout_settings'] = [];
   +        }
   +        else {
   +          $plugin->setLayout($layout, []);
   +          $cached_values['plugin'] = $plugin;
   +          unset($cached_values['layout_change']);
   +        }
   

   This is repeated a few times in this method. Can it be split off into a private utility method?

4. +++ b/src/Plugin/DisplayVariant/PanelsDisplayVariant.php
   @@ -399,12 +401,25 @@ class PanelsDisplayVariant extends BlockDisplayVariant implements PluginWizardIn
   +      if ($method->class != 'Drupal\layout_plugin\Plugin\Layout\LayoutBase') {
   +        $operations['settings'] = [
   +          'title' => $this->t('Layout Settings'),
   +          'form' => LayoutChangeSettings::class,
   +        ];
   +      }
   

   It seems to me that rather than check if the method is not in LayoutBase, we should be checking to see if it's in the plugin class. Unless I'm missing something, that seems like the more certain way to be sure that the plugin is actively implementing buildConfigurationForm().

Took care of the minor low-hanging problems in the patch.

Ok, updated the route requirements to allow each wizard that uses these steps to inject its own access criteria.

Eclipse

This patch addresses comments in 37 and 35. It's not been manually tested but any failures here should be minor oversights which I will fix next.

37.1: Yep ok. It's unlikely to be a problem, but now it's REALLY unlikely.

37.2: Yeah that could use a comment.

37.3: I'm currently preventing this step from showing up unless the layout is actually implementing or extending from a class which overrode the PluginFormInterface::buildConfigurationForm() method. This prevents LayoutBase::buildConfigurationForm() from giving us a false positive for this layout being configurable and providing a form. However, unfortunately, "configurability" and providing a form element are actually two completely separate things with separate interfaces. Yes this logic is identical, and I can probably abstract it into a protected method. I have taken a stab at doing that, but the better thing to do is to fix our abstract layout base class so that it doesn't needlessly implement these methods. To that end, I've filed an issue: #2749375: LayoutBase incorrectly implements PluginFormInterface and ConfigurablePluginInterface

37.4: I disagree. One could certainly imagine a group of subclasses whose parent provides a relevant method for this behavior. What we know for certain is that LayoutBase does not, and any class directly using that method is not providing a form. This is why we should really get the house in order on this topic (i.e. the issue I filed for 37.3) because then we'd know for certain. Right now I'm just taking the best guess that I'm most comfortable with.

35.1: Yup, done

35.2: Yup, done

35.3: You already took care of this. (Yay phenaproxima)

35.4: You already took care of this too. (Yay phenaproxima)

35.5: And this (Yay phenaproxima)

35.6: This too (Yay phenaproxima)

35.7: yup (Yay phenaproxima)

35.8: yup (Yay phenaproxima)

35.9: yup (Yay phenaproxima) (And I fixed a deprecated function call we had right there too).

35.10: Yeah, should have done the comparison to the form element for sure, let's see how this works out. Not sure why I'm casting that to a string and them comparing it to an object... but hey it worked (yay D8).

35.11: yup (Yay phenaproxima)

35.12: yup (Yay phenaproxima)

35.13: So I had to read this a few times before I decided I agree with you. In isolation I'd rather use the getter or as we discussed in chat assign a variable based on the result of the getter, but since this method is also assigning the value of the property, I have fewer issues using the property directly. so again phenaproxima++.

35.14: yup (Yay phenaproxima)

35.15: yup (Yay phenaproxima)

35.16: yup (Yay phenaproxima)

35.17: yup (Yay phenaproxima)

35.18: Done

35.19: Done

35.20: Seems sensible however, our other get/set methods that I copied here don't do that and we should probably update all of those methods at the same time. I say we do this in a follow up.

35.21: Done

35.22: Done

35.23: Done

35.24: Done

35.25: Done

35.26: Done

35.27: Done

Ok, I added some missing docs, various clarifications and class docs. Fixed tests and a little bit of code that was preventing those from working.

Eclipse

Restoring layout settings test coverage. I think we're at "needs review". Tests should pass against a local checkout that includes head of CTools and head of PageManager.

Eclipse

I can say that the last patch still fixes Page Manager :) . Without it I can't add any blocks to Panel pages.

So I did a UI review of this patch. In general, getting there! I think many of the UI bugs I found are not actually related to the wizard, but just in case I'll post them here.. and if there are better issues already made for these or if we need to make them, then thats pretty straightforward as well.

1) Tests are sorta working. I managed to get through all of them before getting an error at the very end "Call to a member function getUsername() on a non-object in docroot/core/modules/simpletest/src/WebTestBase.php" but I think this might be environment related.

2) Adding a variant. The label in D8 is confusing, and probably should be hidden by default. If someone wants to change it later, they can. Also did we resolve the conflicting namespaces? Multiple variants should be able to be called 'Panels'

3) The builder step for configuring a variant is a bit confusing for regular builders. It also doesn't show IPE for some reason.

4) Editor vs Standard vs IPE show the content pane. When using the standard button and flipping to editor, it erases the page title.

5) Block dialog box is now abnormally large. Not sure if thats a bug in ctools block or something wizards introduced?

6) Placing blocks with IPE can fail due missing contexts
We've had this issue before, but I thought we fixed it in the IPE. However, just selecting the 'authored by' pane should not have thrown this error.

Some of these issues are blockers to the next release of panels, but might not be a blocker per-se to getting the wizards patch in. The sooner it is in, the sooner we can start resolving the issues it may or may not have caused ;)

Thanks for the reviewing, @japerry!

The sooner it is in, the sooner we can start resolving the issues it may or may not have caused ;)

Just wanted to add my 2 cents on this last point...

Now that Panels is -beta, I really think we should resolve any regressions before merging this patch. It's also a lot easier to find regressions in a single patch, rather than if we end up spreading this across several commits.

For this reason, I think we should aggressively punt anything that isn't a regression or new functionality in this patch to follow-up issues. :-)

Haha words, they don't come easy ;) But yup, I'm right there with ya.

I want to aim for no new regressions from the wizards patch, but make sure we split what is a regression caused by wizards (either in compatibility, other modules being broken, etc), and regressions that wizard api highlights. The latter of these should be punted to their own issue(s).

Ok, I'm sorry it took so long to respond to this, somehow I missed the review. My bad.

1.) Tests are reliably working for me and I have no current failures.
2.) Jakob and I went through this all together. I've pushed new code to PageManager (since this variant adding this is a feature of PM not panels nor its integration with PM) that fixes this whole label thing for variants. They are no longer required, they will be set to the administrative label of the selected variant if submitted empty and they are not disabled once they have a value, so they can be updated later. The default variant will be the alphabetically "lowest" variant, which is http status code. This is a change from D7, but I'm not sure how we'd determine which should be the default. I mean Panels... sure, but maybe this is a topic for core?
3.) Editor should likely be removed. It does nothing of note and is kinda confusing. IPE wasn't showing up because of it's requirement for panels storage on the PanelsDisplayVariant. The interdiff shows my fix for this.
4.) Jakob and I decided to ignore this since Editor should go away.
5.) Could not reproduce.
6.) Looks like an IPE bug to me unrelated to the wizard work. Since we're not touching anything on the front end, I would expect this to be a problem in HEAD. I did investigate this a bit and found nothing to debunk my basic assumptions here.

The short of this is that I think I've squared away the issues that were problems in this patch as well as an issue in PM. Anything else?

Eclipse

Tests did pass, and after review with Kris in #48 I think we're good.

Committed.

Followup issues: #2757331: Do we need EditorDisplayBuilder ? #2759463: Panels IPE Loses Route Parameter Contexts