Skip variant plugins that are incompatible with Page Manager

Thanks, Brant, for opening this issue! This is something that has been annoying me too. :-) However, I'd propose we just filter out the core display variants all together because neither really makes sense on a page manager page.

Er, that is filter out of the UI. So in my proposal this would filter in the form rather than hook_display_variant_plugin_alter().

With the right theme and page_manager plus the "Page with blocks" Variant one essentially has a "page_manager_everywhere" type of solution that they can do things with.

Interesting! I didn't know that was possible. :-) If you have the time, could you explain how to do that? Is it a matter of page_manager not having the UI built to configure that particular display variant?

Looking at the code for Drupal\block\Plugin\DisplayVariant\BlockPageVariant it looks like by default it loads the block configuration directly from the 'block.repository' service, which has the global block configuration. Were you envisioning page_manager injecting a special BlockResponsitoryInterface that got it's configuration from the page manager config entity?

Although, page manager is still building content to be displayed in the main content block. So, we'd need to have some alternate mode where instead we subscribed to RenderEvents::SELECT_PAGE_DISPLAY_VARIANT, right?

Anyway, this is squarely in the territory of code that I don't know very well at all. :-)

I've been thinking about this more, and I don't personally think it makes sense (at least based on my current understanding of the code) to try and re-use Drupal\block\Plugin\DisplayVariant\BlockPageVariant to implement functionality like "Panels Everywhere".

Firstly, we'd still want the user to be able to choose the variant type for their "page template" page. For example, it should be fine to use the "Panels" variant or any other normal variant for use in page_manager.

It also just seems like more trouble than it's worth to try to re-use the "Page with blocks" variant. It's code is so specific to what it's doing, and we'd probably have to write support code that's more complicated than it, just to re-use it. :-)

Here's how I'd imagine implementing "Panels Everywhere":

1. Provide some configuration form to chose a Page Manager page that is the page template (or have a single page hard-coded in the system like in Drupal 7)
2. Implement an event subscriber that subscribes to RenderEvents::SELECT_PAGE_DISPLAY_VARIANT which:
   1. Loads the configured Page Manager page
   2. Setups up the required contexts
   3. Executes the selection conditions until it finds the right variant
   4. Sets that variant as the one which will render the page

So, rather than using the "Page with blocks" variant, we're swapping it out for a variant that was configured with Page Manager.

Circling all the way back to the topic of this issue: I still think it makes sense to filter out the core display variants from the page_manager UI. Or at the very least hide them in a collapsed fieldset or something? While the "Page with blocks" variant works, I just don't think it does something useful enough that anyone would actually want to use it in the real world.

But I'd love to hear what you had in mind! Since I barely know this code at all, I could be missing something BIG. :-)

I should just open a new issue about implementing "Panels Everywhere", but... I realized that the last part of what I described above won't work exactly:

4. Sets that variant as the one which will render the page

The PageDisplayVariantSelectionEvent only takes the plugin id, we can't give it a display variant instance. :-/

We could still accomplish this by creating a new display variant which does steps #2.1 through #2.4 in comment #8, and return the plugin id of that variant in PageDisplayVariantSelectionEvent. But then we're making a new useless display variant that doesn't necessarily make a lot of sense in the page_manager UI, just like "Simple page" or "Page with blocks".

So, it's basically like we need to have display variants that are specific to core, and display variants that are specific to page manager, and we can't really use one for the other. :-( This feels like a weakness in core. If there was a way for PageDisplayVariantSelectionEvent to let use initialize the display variant, we could at least use page manager's variants in core!

@tim.plunkett: What do you think of this? If you also think this is a weakness in core, I'll open a core issue for it. Not sure how the patch to fix it would look, but we can start discussing.

I guess in some ways people see UX and others see end-user oriented "censorship" causing DX confusion.

Ah, ok, I think that's the source of our differing perspectives. :-) I'm looking at removing that option as a way to improve UX.

In any case, thanks for having this discuss with me! It led me to dig into code I hadn't dug into before, get some ideas about Panels Everywhere and a core issue that I'm going to file when I have chance!

I filed a core issue which will allow Page Manager variants to be used by core: #2512062: VariantInterface extends ConfigurablePluginInterface so PageDisplayVariantSelectionEvent should allow passing configuration to the Variant (to enable Panels Everywhere)

As discussed in IRC, lets find a way to get rid of those types that make no sense for page manager.

Just remove them in the UI as a first step with hardcoded keys? Or should we add an interface and only show those that implement it? I think it would be best if we could say to only show configurable variants, but the problem is that those in core also claim to be configurable and have configuration although they're not, not really.

I say we just hardcode the keys and remove those specific core variant types. I'd guess that new variant plugins added in contrib would be configurable, so I'd hope that it's only these two from core that are problematic in the future.

Added patch for deleting 'Simple page' and 'Page with blocks' option. Added test cases. Added screenshot for UI change.

+++ b/src/Controller/PageManagerController.php
@@ -197,6 +197,9 @@ class PageManagerController extends ControllerBase {
     foreach ($this->variantManager->getDefinitions() as $variant_plugin_id => $variant_plugin) {
+      if ($variant_plugin_id == 'simple_page' || $variant_plugin_id == 'block_page') {
+        continue;
+      }
       $build['#links'][$variant_plugin_id] = [

Please add a comment here. Explain that those two are provided by Drupal Core and are not compatible with Page Manager since they are not configurable. So we remove them to improve UX for users.

Maybe also use in_array() to make it easier to extend in case there are more later that we'd want to ignore.

Changes made based on comment #17. Interdiff added.

Thinking more about the comment, we can improve the wording a bit. The sentence is long and quite hard to understand. Try this:

-
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.
-

Make sure to use as much as possible of the allowed 80 characters, don't use separat lines per sentence.

Added changes. Added interdiff.

This implements what I discussed with @dsnopek. Nothing else left to do I think.

+++ b/src/Controller/PageManagerController.php
@@ -197,6 +197,13 @@ class PageManagerController extends ControllerBase {
+      if(in_array($variant_plugin_id, array('simple_page', 'block_page'))) {

If anyone gets to it before I have a moment to commit this, please add a space after the if, and switch to [] for the array.

Changes made based on comment #23. Interdiff added.

Committed, thanks!

Woohoo! Thanks, everyone! :-)