Currently, any page that has this block on and has updated query string would have the Dynamic Page Cache invalidated. This happens because contexts should be url.path rather then url - this is to make sure that adding query path does not invalidate this block's dynamic page cache.

Also, contextsand tags are not merged with parent tags making it impossible to alter tags for this block plugin.

CommentFileSizeAuthor
#12 social_media-dynamic-page-cache-invalidated-url-query-2931644-12.patch1.16 KBscuba_fly
#11 social_media-dynamic-page-cache-invalidated-url-query-2931644-11.patch1.07 KBMiroslavBanov
#6 social_media-dynamic-page-cache-invalidated-url-query-2931644-6.patch2.24 KBMiroslavBanov
#5 social_media-dynamic-page-cache-invalidated-url-query-2931644-5.patch486 bytesMiroslavBanov
#2 social_media-dynamic-page-cache-invalidated-url-query-2931644-1.patch3.25 KBalex.skrypnyk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alex.designworks created an issue. See original summary.

alex.skrypnyk’s picture

alex.skrypnyk
he/him
Primary language English
Location Melbourne
CreditAttribution: alex.skrypnyk at DrevOps commented
FileSize
social_media-dynamic-page-cache-invalidated-url-query-2931644-1.patch3.25 KB

Attached patch resolves this issue by using getCacheTags() and getCacheContexts().

gargsuchi’s picture

gargsuchi CreditAttribution: gargsuchi at Salsa Digital for Department of Premier and Cabinet - Victoria, Australia commented
Status: Active » Needs review
MiroslavBanov’s picture

MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commented
Version: 8.x-1.2 » 8.x-1.x-dev
Status: Needs review » Needs work

There is no need to initialize the cache tags and contexts in the constructor and there is no need to call the methods at all, as BlockViewBuilder::viewMultiple will call them.

See for example SystemMenuBlock code for how this is done in core.

MiroslavBanov’s picture

MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
social_media-dynamic-page-cache-invalidated-url-query-2931644-5.patch486 bytes

This patch is only making the fix without making the other code style related changes.

MiroslavBanov’s picture

MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commented
FileSize
social_media-dynamic-page-cache-invalidated-url-query-2931644-6.patch2.24 KB

This one also includes the code style things from the previous patch. Both patches work for me equally well.

scuba_fly’s picture

scuba_fly
Location Netherlands, Vinkeveen
CreditAttribution: scuba_fly at LimoenGroen commented
Status: Needs review » Reviewed & tested by the community

I would go for patch #5 and commit the codestyle fixes in a different commit.

scuba_fly’s picture

scuba_fly
Location Netherlands, Vinkeveen
CreditAttribution: scuba_fly at LimoenGroen commented
Assigned: alex.skrypnyk » Unassigned
Status: Reviewed & tested by the community » Needs work

Hmm

#5 does not add the getCache methods.

I think we need a new patch.

MiroslavBanov’s picture

MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commented

@scuba_fly

#5 doesn't add the getCache methods, and #6 adds them.

So I am not sure are you saying both patches are not good and we need a new one? Please elaborate what this new patch should have? :)

scuba_fly’s picture

scuba_fly
Location Netherlands, Vinkeveen
CreditAttribution: scuba_fly at LimoenGroen commented

let's make a new patch that fixes this with getCache methods.

So patch #6 without the codestyle fixes.

The codestyle fixes are in this issue #3001090: Coding standard

The reason to do it separately, is so it is more clear, and less chance of merge conflicts, giving it a better change other patches still apply.

MiroslavBanov’s picture

MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commented
Status: Needs work » Needs review
FileSize
social_media-dynamic-page-cache-invalidated-url-query-2931644-11.patch1.07 KB

New patch per #10

scuba_fly’s picture

scuba_fly
Location Netherlands, Vinkeveen
CreditAttribution: scuba_fly at LimoenGroen commented
FileSize
social_media-dynamic-page-cache-invalidated-url-query-2931644-12.patch1.16 KB

Thank you @MiroslavBanov

Made a new patch that applies to the latest dev version.

Added in the 'config:social_media.settings'

Does this make sense?

+++ b/src/Plugin/Block/SocialSharingBlock.php
@@ -205,4 +196,21 @@ class SocialSharingBlock extends BlockBase implements ContainerFactoryPluginInte
+      'config:social_media.settings',

Added this line.

Cannot make an interdiff because of changes on the dev branch.

MiroslavBanov’s picture

MiroslavBanov CreditAttribution: MiroslavBanov as a volunteer and at FFW commented
Status: Needs review » Reviewed & tested by the community

It makes perfect sense. I also tested it on a new Drupal installation, and it worked as expected.

If trying to test, keep in mind that Toolbar module currently disables dynamic page cache, so test with Toolbar disabled.

  • scuba_fly committed fe6aeb3 on 8.x-1.x 
    Issue #2931644 by MiroslavBanov, scuba_fly, alex.skrypnyk: Dynamic Page...
scuba_fly’s picture

scuba_fly
Location Netherlands, Vinkeveen
CreditAttribution: scuba_fly at LimoenGroen commented
Status: Reviewed & tested by the community » Fixed

Thank you for your work and testing MiroslavBanov!

committed the last patch.

scuba_fly’s picture

scuba_fly
Location Netherlands, Vinkeveen
CreditAttribution: scuba_fly at LimoenGroen commented
ahebrank’s picture

ahebrank CreditAttribution: ahebrank commented

@scuba_fly, please see https://www.drupal.org/project/social_media/issues/3089976 -- commit https://git.drupalcode.org/project/social_media/commit/fe6aeb3 is missing a necessary `use` statement.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.