Only sending the cache data causes an issue during rendering.

CommentFileSizeAuthor
#10 broken-op-block.png14.96 KBkristiaanvandeneynde
#2 group-cache-data.patch2.63 KBbenjy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjy created an issue. See original summary.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented
Status: Active » Needs review
FileSize
group-cache-data.patch2.63 KB
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Your patch seems to do more than advertised. Wouldn't it suffice to hoist these two lines to the top of the function?

        // Create an operations element with all of the links.
        $build['#type'] = 'operations';

Edit: Also thanks for the patch! I had no idea Drupal trips over an empty build with cacheable metadata. :)

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented 
+++ b/src/Plugin/Block/GroupOperationsBlock.php
@@ -55,14 +60,15 @@ class GroupOperationsBlock extends BlockBase {
+    $cache->applyTo($build);

I just moved to using the CacheableMetadata API instead of manually constructing the array.

This applyTo() call will ensure all the correct cache keys are set on the cache array.

benjy’s picture

benjy CreditAttribution: benjy at PreviousNext commented

Any chance of getting this patch committed?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Yes

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Status: Needs review » Fixed

Cleaned it up a bit and committed with you as the author. Thanks for the patch!

P.S.: Forgive my brevity last week. I'm but alone as a maintainer and the issue queue is long. Sometimes I prefer to see a simple "bump" over an "are we there yet?" :)

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented
Status: Fixed » Needs work
FileSize
broken-op-block.png14.96 KB

Reverted the patch because it now displays the block everywhere, even if there are no operations.

Before I commit any more code for this I will need either a test or a way to reproduce the error.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at MachbarMacher commented
Status: Needs work » Needs review

This is simple: All the rest of the code is perfect. But moving that code outside the for-loop will create a block even when no link is in it.

Daring to set NR as the change is trivial.

  1. +++ b/src/Plugin/Block/GroupOperationsBlock.php
@@ -21,6 +24,8 @@ class GroupOperationsBlock extends BlockBase {
+    $build['#type'] = 'operations';
+    $cache = CacheableMetadata::createFromRenderArray($build);

  2. +++ b/src/Plugin/Block/GroupOperationsBlock.php
@@ -55,14 +60,15 @@ class GroupOperationsBlock extends BlockBase {
-        // Create an operations element with all of the links.
-        $build['#type'] = 'operations';
         // @todo We should have operation links provide cacheable metadata that
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented

This part of the code got a huge overhaul in #2906082: Figure out a way to cache lists using group permissions so maybe we should wait until that one lands