Problem/Motivation

While working on #2825812-85: ImageItem should have an "derivatives" computed property, to expose all image style URLs, it was noticed that \Drupal\hal\LinkManager\LinkManagerBase::getLinkDomain() may return an string that includes the request/site URL but doesn't take into account the url.site cache context.

Proposed resolution

Figure out how and when to add the url.site cache context when using \Drupal\hal\LinkManager\LinkManagerBase::getLinkDomain(). Unfortunately, there's also \Drupal\hal\LinkManager\LinkManagerBase::setLinkDomain(), which allows arbitrary logic to set the link domain, which means it's literally impossible to know which context to vary by in case that was used. Therefore we should only add the url.site cache context in one case: when the normalizer actively uses the request/site URL. We should add the config:hal.settings cache tag when getLinkDomain() uses that instead. And in the extreme edge case of custom code calling setLinkDomain(), it's up to the module doing that to implement hook_hal_type_uri_alter() and hook_hal_relation_uri_alter() to add the necessary cacheability metadata (if any, because usually it will be hardcoded). (Note that setLinkDomain() is very likely only used for test coverage.)

Remaining tasks

Do it!

User interface changes

None

API changes

None

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

Wim Leers’s picture

Issue tags: +D8 cacheability
Wim Leers’s picture

Priority: Normal » Minor

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

Sometimes I'm wondering whether we should add the url.site cache context as required. Absolute URLs are potentially rendered quite often, also in custom code, so it might be worth to add the url.site cache context everywhere.

Wim Leers’s picture

I’ve contemplated the same!

dawehner’s picture

I guess the way to implement is to call out to the renderer?

Wim Leers’s picture

Actually, I think this can either be postponed on #2910211: Allow computed exposed properties in ComplexData to support cacheability., or it can import a few of the changes it was intended to do.

Wim Leers’s picture

Issue summary: View changes

Fixed formatting of IS.

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
3.75 KB

Importing the infrastructure that #2910211: Allow computed exposed properties in ComplexData to support cacheability. aims to add. @tedbow should be credited for this.

In the next patch, I'll build on this infrastructure.

Wim Leers’s picture

Issue summary: View changes
FileSize
6.41 KB
8.64 KB

This patch then shows how the HAL module's link managers could use it.

Wim Leers’s picture

FileSize
3.7 KB
12.28 KB

To properly test this, we first need to improve the existing test coverage, to allow cacheability test coverage to be added.

Rather than doing it for all of HAL's link manager test coverage, I'm only doing it for one bit for now, to get feedback before doing all the work.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
3.45 KB
14.2 KB

And this then adds the cacheability bubbling test coverage.

Need feedback now!

The last submitted patch, 12: 2864816-12.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

BTW, the reason @dawehner and I are working on this issue suddenly is because #2869426-26: EntityResource should add _entity_access requirement to REST routes indicated that this would likely help that issue move forward.

Wim Leers’s picture

Wim Leers’s picture

Oh, and ever since #2113345: Define a mechanism for custom link relationships, the url.site cache context is present on all responses for both hal_json and json, so we can't easily test this in functional tests (but #2869426: EntityResource should add _entity_access requirement to REST routes) might change that). Which is why #12 + #13 are expanding the existing kernel test coverage!

Wim Leers’s picture

borisson_’s picture

Status: Needs review » Needs work

Overall this looks very solid and I think the added testcases are very clear.

There's a coding standard thing that needs fixing (see #14) and I think we can improve the inline docs a little bit, but these are just nitpicks.

  1. +++ b/core/modules/hal/tests/src/Kernel/HalLinkManagerTest.php
    @@ -46,19 +48,95 @@ protected function setUp() {
    +  public function providerTestGetTypeUri() {
    

    I think this needs a docblock.

  2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -128,11 +138,18 @@ public function getResponseFormat(RouteMatchInterface $route_match, Request $req
    +   * In prior versions of Drupal 8, we allowed implicit bubbling of cacheability
    

    In prior versions to what? Should we add that this was added in 8.4?

    Something like In 8.4.x, we added functionality to explicitly pass in cache metadata, before that implicit bubbling was allowed.

dawehner’s picture

I really like this way of adding cacheablity metadata ... at least the public methods don't have additional parameters ... and we don't have to introduce another way or add the cacheability metadata to the renderer directly.

  1. +++ b/core/modules/hal/src/LinkManager/LinkManagerBase.php
    @@ -39,17 +41,32 @@ public function setLinkDomain($domain) {
    +  protected function getLinkDomain(array $context) {
    

    Could we make this an optional change to avoid breaking, if possible?

  2. +++ b/core/modules/hal/tests/src/Kernel/HalLinkManagerTest.php
    @@ -46,19 +48,95 @@ protected function setUp() {
    +      'site URL' => $base_test_case + [
    ...
    +      'site URL, with optional context to collect cacheability metadata' => $base_test_case + [
    

    <3

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
  1. Oh, yes, totally, that's why I did it that way in the first place. I just forgot to give it a default value, silly me!
  2. ❤️

Alright, on it.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +API-First Initiative
FileSize
2.7 KB
14.22 KB

Fixed #19.1/#20.1. And fixed #19.2.

Next: test coverage expansion.

Wim Leers’s picture

FileSize
3.54 KB
17.28 KB

Modernizing more of HalLinkManagerTest so we can more easily add cacheability bubbling test coverage.

Wim Leers’s picture

FileSize
5.17 KB
20.61 KB

And then finishing the updates to HalLinkManagerTest to fully test the changes being made here.

dawehner’s picture

+++ b/core/modules/hal/src/LinkManager/LinkManagerBase.php
@@ -39,17 +41,32 @@ public function setLinkDomain($domain) {
-        $this->linkDomain = rtrim($domain, '/');
...
+        return rtrim($domain, '/');

Is this okay that we need to do the same computation every time we ender this?

Wim Leers’s picture

Title: HAL LinkManager doesn't add 'url.site' cache context when needed » [PP-1] HAL LinkManager doesn't add 'url.site' cache context when needed
Related issues: +#2915490: Modernize & harden HalLinkManagerTest

I've extracted #12 + #23 to a separate issue, to keep the size/scope smaller here. See #2915490: Modernize & harden HalLinkManagerTest.

Wim Leers’s picture

Status: Needs review » Postponed
Wim Leers’s picture

Title: [PP-1] HAL LinkManager doesn't add 'url.site' cache context when needed » HAL LinkManager doesn't add 'url.site' cache context when needed
Assigned: Wim Leers » Unassigned
Status: Postponed » Needs review
FileSize
17.53 KB

#2915490: Modernize & harden HalLinkManagerTest landed! Rebased #24 cleanly on top of HEAD.

Wim Leers’s picture

Is this okay that we need to do the same computation every time we ender this?

The answer is: yes, it's okay, because:

  1. Dynamic Page Cache is caching the response (#2827797: ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster added explicit test coverage if you have doubts).
  2. It's a tiny amount of string manipulation.

Also: premature optimization is the root of all evil.

dawehner’s picture

Also: premature optimization is the root of all evil.

I totally agree with you, but we should keep in mind that there is some small behaviour change. There is a chance that someone relied on the behaviour that $this->linkDomain was cached. You never know what things people do. While I think the chance is super low here, I could easily imagine that in other similar cases this might be an issue.

Wim Leers’s picture

There is a chance that someone relied on the behaviour that $this->linkDomain was cached. You never know what things people do. While I think the chance is super low here, I could easily imagine that in other similar cases this might be an issue.

100% agreed! The performance implication is less important, but the potential reliance of custom code on an unintentional side effect built in to the system is important because it might break code. I do agree that the chance of somebody relying on this is extremely small though.

So … do we have an RTBC? :)

tstoeckler’s picture

+++ b/core/modules/hal/src/LinkManager/LinkManagerBase.php
@@ -39,17 +41,32 @@ public function setLinkDomain($domain) {
+  protected function getLinkDomain(array $context = []) {
     if (empty($this->linkDomain)) {

So I was wondering about the case where we don't go into this if-branch which can happen if the link domain is set externally via setLinkDomain(). Is there any way I can attach cacheability metadata if I'm using that?

Wim Leers’s picture

Is there any way I can attach cacheability metadata if I'm using that?

No.

That's why the IS says this:

Unfortunately, there's also \Drupal\hal\LinkManager\LinkManagerBase::setLinkDomain(), which allows arbitrary logic to set the link domain, which means it's literally impossible to know which context to vary by in case that was used. Therefore we should only add the url.site cache context in one case: when the normalizer actively uses the request/site URL. We should add the config:hal.settings cache tag when getLinkDomain() uses that instead. And in the extreme edge case of custom code calling setLinkDomain(), it's up to the module doing that to implement hook_hal_type_uri_alter() and hook_hal_relation_uri_alter() to add the necessary cacheability metadata (if any, because usually it will be hardcoded). (Note that setLinkDomain() is very likely only used for test coverage.)

Added some emphasis to clarify.

In other words: the setLinkDomain() setter should never have been a public API.

EDIT: could we still mark it @internal? Given that there's likely no code using this at all? Not even https://www.drupal.org/project/entity_pilot is using this.

tstoeckler’s picture

Ahh thanks for the explanation.

I had scrolled through the discussion above to see if it had been mentioned, but did not realize you were two steps ahead of me and added it two the IS directly. Sorry, I should have checked that.

I guess in theory we could allow (optionally) passing cacheability metadata along in setLinkDomain() but if you say that it's a bogus API in the first place, that seems more trouble than its worth. I think the patch makes perfect sense, then. This is not really a patch I can RTBC, though.

Wim Leers’s picture

Priority: Minor » Normal

No problem 🙂

I think committing this as-is makes sense, and if a legitimate need arises to pass cacheability metadata with setLinkDomain(), then we can still add that in the future. This patch is fixing the bug for 99.9% of users.

dawehner’s picture

In other words: the setLinkDomain() setter should never have been a public API.

It is so unlikely that I was totally able to find a user: default_content. Its used here to ensure both the import and the export work on the same domain.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I'm fine with the patch as it is though.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -152,14 +170,25 @@ protected function renderResponseBody(Request $request, ResourceResponseInterfac
+          @trigger_error('Implicit cacheability metadata bubbling (onto the global render context) in normalizers is deprecated since Drupal 8.4.0 and will be removed in Drupal 9.0.0. Use the "cacheability" serialization context instead, for explicit cacheability metadata bubbling.', E_USER_DEPRECATED);

This should link to the change record right? So I think we need a change record too?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.01 KB
17.57 KB

Good point! Change record created: https://www.drupal.org/node/2918937.

Also had to update that to say "8.5.0" instead of "8.4.0".

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 2864816-39.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Completely unrelated test changes, seemingly with the database. Retesting.

Wim Leers’s picture

FYI: #2910211: Allow computed exposed properties in ComplexData to support cacheability. is also adding the same exact changes to ResourceResponseSubscriber.

That issue needs it for field normalizers. This issue needs it for the HAL normalization's need for links to be in the normalization. Both need support for cacheability bubbling, this is further confirmation that this change is sound!

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/hal/tests/src/Kernel/HalLinkManagerTest.php
@@ -100,6 +126,16 @@ public function providerTestGetTypeUri() {
+          'cacheability' => (new CacheableMetadata())->setCacheTags(['config:hal.settings']),

@@ -162,6 +222,17 @@ public function providerTestGetRelationUri() {
+          'cacheability' => (new CacheableMetadata())->setCacheTags(['config:hal.settings']),

Should we use the constant here?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.43 KB
17.67 KB

Yes!

Wim Leers’s picture

Title: HAL LinkManager doesn't add 'url.site' cache context when needed » [PP-1] HAL LinkManager doesn't add 'url.site' cache context when needed
Status: Reviewed & tested by the community » Postponed
FileSize
13.28 KB

Per #2910211-17: Allow computed exposed properties in ComplexData to support cacheability., postponing this on #2910211. Attached is this patch rebased on top of that issue's patch. The only thing that changes, is that the ResourceResponseSubscriber hunk can be omitted entirely, since it's already part of #2910211.

Postponing this, this patch should only be tested after #2910211 has been committed.

Wim Leers’s picture

Title: [PP-1] HAL LinkManager doesn't add 'url.site' cache context when needed » HAL LinkManager doesn't add 'url.site' cache context when needed
Status: Postponed » Reviewed & tested by the community

  • catch committed 2170724 on 8.5.x
    Issue #2864816 by Wim Leers, dawehner, larowlan, tedbow: HAL LinkManager...

  • catch committed 66df3e1 on 8.4.x
    Issue #2864816 by Wim Leers, dawehner, larowlan, tedbow: HAL LinkManager...
Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed

Yay!

  • xjm committed fd497ef on 8.4.x
    Revert "Issue #2864816 by Wim Leers, dawehner, larowlan, tedbow: HAL...

xjm credited catch.

xjm’s picture

Status: Fixed » Needs work

This broke head on 8.4.x: https://www.drupal.org/pift-ci-job/812198

However the branch is set to 8.5.x, which I haven't gotten fail notifications for. Was this supposed to be backported or no?

Wim Leers’s picture

Status: Needs work » Fixed

Tt definitely wasn’t supposed to be backported. Thanks!

Status: Fixed » Closed (fixed)

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

timmillwood’s picture

+++ b/core/modules/hal/src/LinkManager/LinkManagerBase.php
@@ -2,6 +2,8 @@
+use Drupal\rest\EventSubscriber\ResourceResponseSubscriber;

@@ -39,17 +41,32 @@ public function setLinkDomain($domain) {
+        if (isset($context[ResourceResponseSubscriber::SERIALIZATION_CONTEXT_CACHEABILITY])) {
+          $context[ResourceResponseSubscriber::SERIALIZATION_CONTEXT_CACHEABILITY]->addCacheContexts(['url.site']);
+        }

Hal is now referencing a rest class but doesn't depend on rest, so it breaks!

Error: Class 'Drupal\rest\EventSubscriber\ResourceResponseSubscriber' not found in /core/modules/hal/src/LinkManager/LinkManagerBase.php on line 65 #0 /core/modules/hal/src/LinkManager/TypeLinkManager.php(72): Drupal\hal\LinkManager\LinkManagerBase->getLinkDomain(Array)
timmillwood’s picture

Wim Leers’s picture