Showing same content on new pages

Hi,

I had the same issue. The problem is that the PageBlockDisplayVariant class uses a uuid for the cache tags, but this uuid is always empty, so this results in having the same cache key for multiple variants.

I attached a patch that solves the issue, but I am not sure if this is the correct way to solve it, it needs some review from people which are more involved into this module, for example, this way we generate a uuid in the variant_settings property of the variant object which is actually different than the uuid of the variants itself. I think it would make sense to somehow use the same uuid. To be more clear, using this patch will generate a configuration like this one below, for a Variant:

uuid: 4ced4486-d845-4d52-9e00-e38d71dcea11
langcode: en
status: true
dependencies:
  config:
    - page_manager.page.sdk_overview
id: test_block_page
label: 'Test block page'
variant: block_display
variant_settings:
  selection_conditions: {  }
  blocks: {  }
  id: block_display
  label: null
  uuid: 0c91fcd6-b186-493f-ad00-f53237d47796
  weight: 0
  selection_logic: and
  page_title: 'A test page'
page: sdk_overview
weight: 0
selection_criteria: {  }
selection_logic: and
contexts: {  }

You can see that the uuid in the variant_settings is different than the one of the variant entity.

Applying the patch and re-saving the variants should make the pages work.

Yeah, I noticed that there are a lot of weird/empty property in the variant settings in the new config entity.

I don't think we need a UUID, this probably dates back to when the only thing to identify a variant with was the UUID. We should be able to switch to the variant ID now, assuming we can access that.

Looked into it, and this is a more complicated than we thought.

All that default configuration comes from both the core block plugin which defines default config for weight and uuid and uses that for a id() method. But doesn't set it anyore. And the selection stuff comes from ctools.

So, we kind of agreed in IRC that..

* The condition stuff in ctools on the BlockVariantBase class should go away. I think that means we drop ConditionVariantInterface completely and say that whoever stores variants is supposed to figure that out himself? Just like we did that for blocks in core, basically.
* We should have a nicer way to provide a value for the id() method. My suggestion would be a setUniqueId() method on an extended VariantInterface in ctools and an id() method that uses that and throws an exception if not set. It's still possible to override and use a UUID.
(* I also think the core class has too much half-implemented logic but it's too late to do something about that)

However, the approach in #3 is a good step to move forward here and just fix the bug. Which I think is critical because the module is *completely* broken if you don't use the developer settings.local.php which disables render caching.

I'm just changing the logic to use id() instead of hardcoding the uuid, didn't know that exists when I added this a long time ago. Then we can implement the things above and eventually get rid of those weird things.

Working on a test...

Ok, this isn't super nice, but it works. I'm adding a second page with a different display and in HEAD, that is failing by returning the other render cached page given I visit that one first.

This looks good to me! :-) Thanks, berdir!

This works like a charm! Thanks Berdir!

1. +++ b/src/Plugin/DisplayVariant/PageBlockDisplayVariant.php
   @@ -159,6 +157,11 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
   +    $form['uuid'] = [
   +      '#type' => 'value',
   +      '#value' => $this->configuration['uuid'] ? : $this->uuidGenerator->generate(),
   +    ];
   
   @@ -171,6 +174,9 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
   +    if ($form_state->hasValue('uuid')) {
   +      $this->configuration['uuid'] = $form_state->getValue('uuid');
   +    }
   

   This part looks great.

2. +++ b/src/Plugin/DisplayVariant/PageBlockDisplayVariant.php
   @@ -32,9 +32,7 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
   -      $this->configuration['uuid'],
   +      $this->id(),
   
   @@ -80,7 +78,7 @@ class PageBlockDisplayVariant extends BlockDisplayVariant {
   -            'keys' => ['page_manager_block_display', $this->configuration['uuid'], 'block', $block_id],
   +            'keys' => ['page_manager_block_display', $this->id(), 'block', $block_id],
   

   But if we're fixing UUID, why ditch it here?

Duh, id() returns $this->configuration['uuid']

It handles the former @todo:

-      // The UUID of this display.
-      // @todo should have an API for this?