Remove Drupal\page_manager\Plugin\PageAwareVariantInterface

I'm adding to the sprint board because a Panels issue is blocked on this one: #2511576: Remove dependency on Page Manager

I'm going to give this one a shot!

Alright! Here's my first pass at this. I'd really appreciate some review particularly around the changes to the render caching stuff.

This should pass more tests, but it's gotten kind of ugly because any form that needs to edit a variant needs to be passed the page id, because the variant is part of the page config entity. To get around that, I've passed the page_id in as configuration, however, this is totally wrong, wrong, wrong!

In #2550879: Use CTools Wizard API to add/edit Pages (and move plugin UI using PluginWizardInterface), variants are turning into their own independent config entities which means we won't need to pass around the page_id, which is a much better solution to this problem.

Ok, this one should finally pass! I wasn't getting the render cache stuff quite right before.

1. Ah, yes, docs! Added. :-) It's needed because of:

+++ b/src/Plugin/DisplayVariant/BlockDisplayVariant.php
@@ -177,11 +163,8 @@ class BlockDisplayVariant extends VariantBase implements ContextAwareVariantInte
-            // Each block needs cache tags of the page and the block plugin, as
-            // only the page is a config entity that will trigger cache tag
-            // invalidations in case of block configuration changes.
-            'tags' => Cache::mergeTags($page->getCacheTags(), $block->getCacheTags()),
+            'keys' => ['page_manager_block_display', $uuid, 'block', $block_id],
+            'tags' => $block->getCacheTags(),

Since we don't have access to the $page when adding the blocks to the render array, we need to add the $page->getCacheTags() a layer up when we do!

The recursiveness is because we don't actually know what the render array from the display variant is going to look like, because it doesn't have to be a BlockDisplayVariant, it could be anything.

2. No, because its DisplayVariantAddForm and the variant was just instantiated a couple lines up.

3. Sure! I put it second so the comment would clearly only apply to that and not 'page_title'.

4. See point #1! It's no longer calling $page->getCacheTags() when building the render array initially, that's been moved to PageViewBuilder.

Discussed with Tim and we're going to postpone this on making the variant into it's own config entity: #2551633: Make variants into their own config entity

I'm been thinking about this more since yesterday, and I think:

1. It might be possible to work around the "'page_id' in configuration" issue without doing #2551633: Make variants into their own config entity by using the tempstore. If we hold the variant configuration in the tempstore key'd by uuid, we should be able to have our dialog routes take only the variant uuid! Then we copy the configuration from the tempstore into the Page entity on submit.
2. Passing 'page_title' in via configuration is wrong - it'll break on any display variant that has a schema but doesn't have a 'page_title' key. :-/ I think we're either going to have to add a new interface (VariantWithTitleInterface?) to mark which variants take a title, OR simply live without having a default value for this field.

Well, it turns out I was right about #12.2, but not #12.1. Just in adding a new block to the variant we need to know the contexts in order to figure out what blocks are available - which means we need to know the page!

So, here's a new patch with #12.2.

I'm going to try a couple more ideas before re-postponing on #2551633: Make variants into their own config entity.

Ok! Here's a new version of this patch that manages to kill the "'page_id' in configuration" hack. It does it by moving the form for editing the blocks from BlockDisplayVariant into DisplayVariantEditForm.

This will unfortunately prevent Panels from customizing the UI for editing blocks (which it will need to do, but doesn't now) but I think we can add that back, either by (a) leveraging the wizard stuff in #2550879: Use CTools Wizard API to add/edit Pages (and move plugin UI using PluginWizardInterface) which will (eventually) provide a way for variants to add wizard steps, or (b) by adding the plugin id of the display variant to the form id used by DisplayVariantEditForm, and then altering it. Obviously, (a) is preferable. :-)

Since Panels isn't presently customizing this UI, and we will find a method for doing that eventually, I'm personally OK with merging this now and leaving it temporarily impossible.

This needs a re-roll - start working on it last week but ran out of time. But I can at least update the issue status. ;-)

Started getting excited about this so I'm doing the re-roll tonight (rather than in the morning as planned :-))! No interdiff as this is a pretty straight re-roll.

This doesn't make any of the changes discussed with @EclipseGC or @timplunkett on IRC. That'll come in the next patch.

And here's a version that remove VariantWithTitleInterface/Trait and the default value that is given to the "Page title" for new variants. As discussed with @timplunkett and @EclipseGC on IRC, it's impractical to provide a default title in all situations and Page Manager in D7 didn't do it - so, we're just dropping that feature. This will have no effect on existing sites, it just means that when adding a new variant, you have to enter the "Page title" from scratch rather than starting with the Page label.

Biggest issue so far is the addition of variant-specific logic to DisplayVariantEditForm::submitForm().

Well, it's temporary until we get the Wizard stuff in. :-)

Also, I don't think this any worse than what we're doing for the selection conditions. We have an interface that defines the functions we're calling, and the form (so the form state) comes from the edit form.

If you can come up with a way to do this better, that'd be awesome, but as far as incremental changes go, IMO this isn't so bad...

Attempted some hackery to get the patches added back to the issue. Hopefully it won't mess something else up. :-)

Looks like it's passing again :-)

I think the cacheable metadata should probably be the DisplayVariant plugin's problem. Each plugin should have the knowledge to gather cacheable metadata specific to its use case ....

So, it is providing all the cacheable metadata specific to it's use-case.

However, for whatever reason, we need to add the page cacheable metadata to that. I'm not entirely sure on why, but the current code does it, and if you remove that, then the tests (and manual testing) break.

I wonder if we could just provide it at the top-level? We'd need someone more knowledgeable about render caching to say if that's really the same thing and doesn't have a negative impact on performance, though.

I may experiment with some variations that add the caching data differently to see if they pass the tests...

Thanks, Wim! My knowledge of render caching is minimal, I was just trying to preserve what the code is already doing, assuming it was right. :-)

Here's a new patch that just applies the cache stuff to the top-level. Unfortunately, I'm having trouble running the tests locally (I get a fun fatal about symfony/translation/TranslatorInterface.php) so let's see if the testbot has better luck!

Hrm. Well, that didn't work! Re-testing the patch from #21 just to make sure it wasn't from another change in HEAD.

Looks like #21 is still passing. So, providing the cache metadata at the top-level doesn't work for some reason. :-/

I think @Berdir is the one who originally wrote the code in HEAD that's adding the page cache metadata to the block cache metadata, so maybe he knows why it's necessary?

For reference here's the code in HEAD:

        $block_build = [
          '#theme' => 'block',
          '#attributes' => [],
          '#weight' => $weight++,
          '#configuration' => $block->getConfiguration(),
          '#plugin_id' => $block->getPluginId(),
          '#base_plugin_id' => $block->getBaseId(),
          '#derivative_plugin_id' => $block->getDerivativeId(),
          '#block_plugin' => $block,
          '#pre_render' => [[$this, 'buildBlock']],
          '#cache' => [
            'keys' => ['page_manager_page', $page->id(), 'block', $block_id],
            // Each block needs cache tags of the page and the block plugin, as
            // only the page is a config entity that will trigger cache tag
            // invalidations in case of block configuration changes.
            'tags' => Cache::mergeTags($page->getCacheTags(), $block->getCacheTags()),
            'contexts' => $block->getCacheContexts(),
            'max-age' => $block->getCacheMaxAge(),
          ],
        ];

        // Merge the cacheability metadata of blocks into the page. This helps
        // to avoid cache redirects if the blocks have more cache contexts than
        // the page, which the page must respect as well.
        $cacheability->addCacheableDependency($block);

http://cgit.drupalcode.org/page_manager/tree/src/Plugin/DisplayVariant/B...

Hrm, I also just noticed that at some point the patch switched from just merging cache tags to merging all cacheability metadata... Back in #14, we did this in the patch:

+  protected static function mergeCacheTags(array &$element, EntityInterface $entity) {
+    foreach (Element::children($element) as $name) {
+      if (isset($element[$name]['#cache'])) {
+        if (empty($element[$name]['#cache']['tags'])) {
+          $element[$name]['#cache']['tags'] = $entity->getCacheTags();
+        }
+        else {
+          $element[$name]['#cache']['tags'] = Cache::mergeTags($entity->getCacheTags(), $element[$name]['#cache']['tags']);
+        }
+      }
+
+      // Recurse to children elements.
+      static::mergeCacheTags($element[$name], $entity);
+    }
+  }

Not sure why we switched, but it's probably my fault. :-)

Anyway, would that code be more acceptable than what's in #21, since it's targeting only tags specifically?

Here's a re-roll after #2558261: Move code in BlockDisplayVariant::build() to a #pre_render

So, here's an attempt at explaining this based on the comments and the code in HEAD:

1. The blocks are being cached independently of the page (rather than just the whole page being cached)
2. The way the blocks are rendered depends on page entity, so if the page entity changes, the cache for all of the blocks needs to be cleared
3. The way HEAD is making this happen, is by merging the cache tags from the page entity into the cache tags for each block

However, in #32, Wim writes:

What happens in this patch (and in HEAD also apparently), is effectively "reverse bubbling". There should be no need for that, unless the blocks actually depend on their parent, in which case this is absolutely the wrong way to do this.

And so assuming I've got the above right, the question is: What is the right way to do this?

A random idea: Would it be better for display variants to optionally implement RefinableCacheableDependencyInterface so that before $variant->build() we could call $variant->addCacheableDependency($page) and then the variant will attach those cache tags to anything inside? Or is that still the reverse bubbling and done wrong?

Thanks!

I've been reading https://www.drupal.org/developing/api/8/render/arrays/cacheability to try and (finally) get a handle on render caching, and I think I understand what we need to do here:

Every element within the render array returned by the variant that has $element['#cache']['keys'] is cached independently. Anything within the render array can potentially depend on the page entity, so, anything that is cached independently needs also get the cache tags of the page entity.

@Berdir / @Wim Leers: Does that sound sane?

Attached is a patch which implements this.

Hrm. I suspect that #2558261: Move code in BlockDisplayVariant::build() to a #pre_render is the reason this isn't working. We might have to put the merging of cache tags into $element['#after_build'] or something - whatever we can get to run after $element['#pre_render']. :-/

Probably won't get to work on this more until next week...

I've been thinking about this a lot, and I'm now pretty convinced that applying the cache tags (and cacheable metadata, in general) in PageViewBuilder isn't the right way to do this.

Here's the problem described really generally:

1. The variant is passed it's configuration (like any plugin implementing ConfigurablePluginInterface) via $variant->setConfiguration()
2. Since the variant can be used by multiple systems, ie. in Page Manager, in core (ala Panels Everywhere), in mini-Panels, Panelizer, etc -- it has no idea where that configuration actually came from
3. However, in order to correctly implement render caching, we need to put information on the render array about the source of the configuration, so that the render cache can be cleared when the configuration changes
4. The variant itself is kind of a "black box" in that from the outside we don't really know what it's going to put in the render array and it could do clever stuff with caching, like setting $element['#cache']['keys'] on the blocks so they're cached independently or using $element['#pre_render'] to prevent loading the blocks if the full variant is in the cache, etc. These are good performance improvements! But only the variant itself is really fully qualified to know where the cacheable metadata should be added (and what should be added, ex. adding only the cache tags on the blocks)

Now, the current patch works by analyzing the render array after the variant has created it and tries to figure out where the cacheable metadata should go. We trying to "out clever" the variant on point #4, which is itself a little suspect :-)

This totally works for Page Manager, because we have the PageViewBuilder which can post-process the render array before ultimately returning it to the controller. But it won't work in core, where we can only give PageDisplayVariantSelectionEvent the name of the variant and config -- we have no opportunity to post-process the render array.

So, this and point #4 above, leads me to believe that we should actually be passing cacheable metadata to the variant (just like we pass it configuration - since the configuration and the cacheable metadata are linked) and allow the variant to add it to the render array where and how it sees fit.

One way to do this would be to have VariantInterface (in core) also extend RefinableCacheableDependencyInterface, and then we can call $variant->addCacheableDependency($page) before calling $variant->build(). Since this problem extends all the way to core (we'll need to change PageDisplayVariantSelectionEvent so we can pass the cache metadata in with the configuration too) that seems like the appropriate place to try and solve it.

But there's other ways to do this too...

Anyway, please let me know what you think!

Tim created a core issue to make the changes to VariantInterface and VariantBase that I described in #55:

#2569083: Allow passing cacheable metadata to display variants (to enable Panels Everywhere)

Here's an attempt at a new patch based on the core patch in #2569083: Allow passing cacheable metadata to display variants (to enable Panels Everywhere) - it won't pass testbot yet, but it is passing tests for me locally. Overall, this feels a lot better to me than the previous attempts :-)

Thanks, Wim!

If a block is render cached, then that means we want to be able to reuse it elsewhere. So the same block may appear in different pages. If that is not true, then there is no point in render caching individual blocks; we should just render cache the Page Manager Page.

Actually, I think this provides value even if the same block doesn't appear on other pages. Take these two scenarios:

Scenario #1: All the blocks on a page are cacheable, so their cacheable metadata bubbles up and the page itself is cacheable. So, the full page will be pulled out of the render cache. The fact that the individual blocks are cached doesn't really matter in practice (but they do take up space in the render cache.)

Scenario #2: One of the blocks isn't cacheable (ie. sets 'max-age' to 0) which bubbles up and the page itself isn't cacheable. But since all the other blocks are cacheable (and cached independently under their own cache key), they'll be pulled individually from the render cache.

I'm guessing this is the reason we're currently using the render cache the way we are. We could remove the cache key from the blocks if the full page is cacheable or something like that, as to not add them to the render cache when it's not necessary?

Could we add an optional interface that has a setCacheableConfiguration(ConfigBase $configuration) signature?

Would this mean passing the full config entity? Or can we extract a ConfigBase that is only a sub-set of the config entity?

The thing is, there's more stuff on the config entity than just the config for a single variant. (In fact, in current page_manager HEAD, the configuration for every variant on the page is stored in a single config entity.) And the variant shouldn't need to understand all the stuff on the full config entity or it's structure, especially since the goal of this issue is to decouple the BlockDisplayVariant from page_manager so we can re-use it in other Panels eco-system modules (ie. Panels, mini-Panels, Panelizer, etc) which will use their own config with it's own structure.

But if it's possible to make a new object which descends from ConfigBase which has only the configuration that applies to the variant, this totally workable!

However, I'm not really sure this can be an optional interface - any display variant that takes configuration at all will need to get the cache metadata information as well or it can't correctly provide cache metadata to the render API. So, if we do it like this, VariantInterface (in core) should extend this new interface and an implementation should be added to VariantBase (also in core).

All that said, I'm not sure this is better than the current proposal? I like that the cache metadata would get passed in the same setter (ie. setCacheableConfiguration()) as the configuration so it's clear they're linked, but since we'll need to muck around with making a ConfigBase that's a subset of the full config entity, it probably has worse DX...

(You're really getting the hang of this render caching and cacheability thing, aren't you? ;) :))

Thanks largely to the great handbook pages on the topic :-)

But… this is starting to sound like we need another value object

I think the ConfigSubset object would definitely help with passing around config and preserving cacheable metadata! (And probably a good idea independent of this issue!)

If only ConfigurablePluginInterface::setConfiguration() took a ConfigBase, then this would be the obvious way to solve this. :-/

Returning to the proposal of adding a VariantInterface::setCacheableConfiguration(ConfigBase $config):

Since we're stuck with ConfigurablePluginInterface as it is, and cachable metadata is basically required for every variant that takes configuration, we'd have to say something like "on classes implementing VariantInterface, please don't ever call $variant->setConfiguration(), only ever call $variant->setCacheableConfiguration()". Which would be super weird if you're passing the $variant to some function that works generically with ConfigurablePluginInterface and doesn't know (or care) that this is a VariantInterface with special rules.

The DX of saying "if you call $variant->setConfiguration() you must also be sure to call $variant->addCacheableDependency() for anything the configuration varies on" (like with the currently proposed core patch) - while totally not ideal, seems more straight forward to me.

So, yes, for BC reasons, this is the best choice. But… what about the dozens, possibly hundreds of implementations of ConfigurablePluginInterface in the lifetime of Drupal 8? Many of them will be plugins that end up rendering things. And thus will need cacheability metadata for the configuration they're given. And so this work-around will need to be repeated many, many times in D8 contrib/custom.

That's why I just want to pose the question: Wouldn't it be better to break BC in D8 HEAD now, so that D8 contrib modules won't have to figure this out all over again?

I agree with this completely, and would definitely follow/support/+1 a core issue around this! If you open it, I'll join you there. :-)

I'm not terribly confident that the rest of the core team will go for it given how close D8-rc1 seems to be, and how much code this will affect, but at least we have a fallback option ready.

So, since this issue is blocking loads of other issues in page_manager, ctools and panels, I personally think we should commit the patch from #54 right away. However, this will need to get fixed the "right way" at some point or Panels Everywhere will be impossible. :-/ I've e-mailed Tim for his thoughts on this!

As far as the "right way," I created an issue for @Wim's (and other) proposals to fix this for all plugins that implement ConfigurablePluginInterface:

#2571009: Plugins implementing ConfigurablePluginInterface that generate render arrays need to be passed cacheability metadata for their configuration! (or it's impossible to build render arrays with cacheability metadata)

If the core maintainers seem OK with any of those solutions, we can close the other core issue that's just for VariantInterface (ie. #2569083: Allow passing cacheable metadata to display variants (to enable Panels Everywhere)) -- if not, we have that as a fallback.

If Tim commits #54, I'll open a new issue to hold the patch from #57.

We could even, at least as a first step, just make the interface optional and check with an instanceof before calling those methods.. Then we don't have to wait on core at all and can just remove the instanceof at the end.

Discussed this with @Berdir on IRC. I think this makes sense! Then the committed patch will look much closer to the final solution once we have one of the patches in core. I'll roll a new patch in a few hours (hopefully) or tomorrow if I run out of time.

Alright! Here's an interim patch we can use until we fix this in core. It still definitely needs to be fixed in core to allow Panels Everywhere, but this keeps the Page Manager code pretty clean (although, the interdiff will need to be reverted when the core fix is in).

Woohoo, thanks! :-)

The goal is to fix that @todo in #2550879: Use CTools Wizard API to add/edit Pages (and move plugin UI using PluginWizardInterface) when we can move the block configuration to a wizard step that's returned by the variant. I suppose we could have put a link in the code comment, but hopefully, we'll kick ass on moving that forward at the Barcelona sprint. ;-)

Re: #73.1: Yeah, that's unrelated to the cacheabliity stuff - it's about the main point of this issue which was to decouple the display variant provided by page_manager from the Page entity, so that we could move it to CTools and re-use it in Panels, and other Panels eco-system modules.