VariantInterface extends ConfigurablePluginInterface so PageDisplayVariantSelectionEvent should allow passing configuration to the Variant (to enable Panels Everywhere)

Issue summary update and better (ie. shorter) title.

Added writing a PoC of Panels Everywhere to the task list.

Here's my first pass at a patch!

+++ b/core/lib/Drupal/Core/Render/PageDisplayVariantSelectionEvent.php
@@ -42,6 +49,7 @@ class PageDisplayVariantSelectionEvent extends Event {
   public function __construct($plugin_id, RouteMatchInterface $route_match) {
     $this->pluginId = $plugin_id;
+    $this->pluginConfiguration = array();
     $this->routeMatch = $route_match;
   }

I didn't add an argument to the constructor for the plugin configuration to minimize API change. The current patch only adds the getter/setter and doesn't change existing APIs. But I don't know if that actually makes sense. :-)

So, I'd love some feedback on that in particular!

It's likely only core that creates this event, so it'd probably be safe to change the constuctor's signature.

Here's a proof-of-concept implementation of Panels Everywhere, which includes the config for a Page Manager Page, and an event subscriber listening on RenderEvents::SELECT_PAGE_DISPLAY_VARIANT which returns the selected variant from that page:

https://github.com/dsnopek/panels_everywhere_poc

It won't work with the current 8.x-3.x-dev of Panels, because we still need to solve #2511570: Remove Drupal\page_manager\Plugin\PageAwareVariantInterface, but as a work around to see it working now, you can comment out the foreach loop in PanelsDisplayVariant::getContextAsTokenData().

This also includes its own display variant plugin, but this really shouldn't be necessary -- PanelsDisplayVariant should implement PageVariantInterface and do the extra work to support that. No issue created for that yet.

In any case, the most important bit is this code in the event subscriber:

  /**
   * Selects the page display variant.
   *
   * @param \Drupal\Core\Render\PageDisplayVariantSelectionEvent $event
   *   The event to process.
   */
  public function onSelectPageDisplayVariant(PageDisplayVariantSelectionEvent $event) {
    $page = $this->entityStorage->load('site_template');
    if ($variant = $page->getExecutable()->selectDisplayVariant()) {
      if ($variant instanceof PageVariantInterface) {
        $event->setPluginId($variant->getPluginId());
        $event->setPluginConfiguration($variant->getConfiguration());
      }
    }
  }

In this file: https://github.com/dsnopek/panels_everywhere_poc/blob/master/src/EventSu...

It shows how it's selecting the variant from the Page Manager Page. Theoretically, there could be several variants with different selection conditions. When it finds the right one, it gives the event the plugin id and the configuration, and HtmlRenderer will use it to render the page!

@andypost: Thanks for the review! Here's a new patch. :-)

I also added the beta evaluation to the issue summary! I'd appreciate feedback on that. I ended up changing the priority to Major, since that seemed to apply more than "prioritized change."

However, I'm not sure about the tests. Do we really need to test a simple getter and setter added to the event object? And the change to to HtmlRenderer is just passing the data from one getter to another setter. The amount of code required to test that would be large, and I think the value would be minimal. But if necessary I can add some tests!

Thanks again!

Er, sorry, I messed up rolling that patch. Here's a better one! The interdiff on #8 is correct, though.

Oh, jeez, my Git-fu is failing me this morning! Not sure how I could get the interdiff right, and the patch so wrong, but THIS one should be right. Sorry for all the extra comments.

Intediff on #8 still correct.

About adding tests:

I don't think it makes sense to unit test the getter and setter added to PageDisplayVariantSelectionEvent, since we aren't testing that getters and setters work anywhere else in core (at least that I could find). I could see adding tests for the change to HtmlRenderer::prepare(), however, there aren't currently any unit tests for HtmlRenderer at all and it's calling \Drupal::theme() directly, so at the moment it can't be tested. I created a new issue for that: #2535212: Add test coverage for HtmlRenderer

@tim.plunkett suggested creating a web test (rather than a unit test) that provides a variant that needs configuration, and makes sure that it was passed when the page was rendered. So, I'll take a stab at that next!

Alright! Here's a new patch with tests like the ones that @tim.plunkett recommended.

As recommended by @tim.plunkett, here's a FAIL patch that has the change to HtmlRenderer::prepare() removed to show that the tests fail. And I've attached a PASS patch too, which is identical to the patch in #12, but hopefully it'll reset the testbot to passing.

Oops! Thanks, I've fixed that. :-)

@Wim Leers: Thanks for the review! :-)

Why was this not necessary so far in the D8 port of Page Manager?

Page Manager (in both D7 and D8) allows you to take over the main content of the page. It's the Panels Everywhere module that allows you to take a Page Manager "Page" and use it to replace the whole page.

I think this is the first time that someone has tried to implement Panels Everywhere. With the change on this issue, it actually becomes quite elegant (as opposed to a hack in D7).

1. Or… can this be simplified to:

   $page_display->setConfiguration($event->getPluginConfiguration());
   

   Yes it can! My thinking was just to avoid calling the setter if there was nothing to set. But here is a new patch that does it that way.

2. These are generic names. IDK what the convention is, but given that this event only applies to a specific plugin type, shouldn't we be more specific here?

   i.e. (get|set)PageDisplayVariantConfiguration?

   Well, (get|set)PluginConfiguration() matches the existing (get|set)PluginId() method names. You end up using it like:

   $event->setPluginId('variant_plugin');
   $event->setPluginConfiguration(['config' => 'value']);
   

   So, if we give setPluginConfiguration() a more specific name, then to match, we really should give setPluginId() a more specific name. However, then this is an API change!

   In the interest of not making an API change, but only an API addition, I think we should leave this alone. But I don't really care about the names and would be happy to go with whatever the consensus is!

@Wim: Thanks again for the review!

#20.1: Done!

However, I'm not sure on the coding standards for doing chaining. Should both the setters be on their own line? For example:

$page_display
  ->setMainContent($main_content)
  ->setConfiguration($event->getPluginConfiguration());

I went the other way in the patch, but I can switch it?

#20.2: Done! Changed setPluginId() to return $this as well. Hopefully, changing the return value from NULL to $this qualifies as an API addition, rather than an API change?

#20.3: Done!

So, this patch has gotten a little messier - the one from #16 changed fewer things, but hopefully the changes are still minor enough to make this easy to get in. :-)

Thanks, Tim!

Also, hiding old patches.

Woohoo! Thanks! :-)