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
Comment | File | Size | Author |
---|---|---|---|
#45 | 2864816-45.patch | 13.28 KB | Wim Leers |
Comments
Comment #2
Wim LeersComment #3
Wim LeersComment #5
dawehnerSometimes 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 theurl.site
cache context everywhere.Comment #6
Wim LeersI’ve contemplated the same!
Comment #7
dawehnerI guess the way to implement is to call out to the renderer?
Comment #8
Wim LeersActually, 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.
Comment #9
Wim LeersFixed formatting of IS.
Comment #10
Wim LeersImporting 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.
Comment #11
Wim LeersThis patch then shows how the HAL module's link managers could use it.
Comment #12
Wim LeersTo 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.
Comment #13
Wim LeersAnd this then adds the cacheability bubbling test coverage.
Need feedback now!
Comment #15
Wim LeersBTW, 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.
Comment #16
Wim LeersComment #17
Wim LeersOh, and ever since #2113345: Define a mechanism for custom link relationships, the
url.site
cache context is present on all responses for bothhal_json
andjson
, 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!Comment #18
Wim LeersComment #19
borisson_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.
I think this needs a docblock.
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.
Comment #20
dawehnerI 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.
Could we make this an optional change to avoid breaking, if possible?
<3
Comment #21
Wim LeersAlright, on it.
Comment #22
Wim LeersFixed #19.1/#20.1. And fixed #19.2.
Next: test coverage expansion.
Comment #23
Wim LeersModernizing more of
HalLinkManagerTest
so we can more easily add cacheability bubbling test coverage.Comment #24
Wim LeersAnd then finishing the updates to
HalLinkManagerTest
to fully test the changes being made here.Comment #25
dawehnerIs this okay that we need to do the same computation every time we ender this?
Comment #26
Wim LeersI've extracted #12 + #23 to a separate issue, to keep the size/scope smaller here. See #2915490: Modernize & harden HalLinkManagerTest.
Comment #27
Wim LeersComment #28
Wim Leers#2915490: Modernize & harden HalLinkManagerTest landed! Rebased #24 cleanly on top of HEAD.
Comment #29
Wim LeersThe answer is:
, because:Also:
.Comment #30
dawehnerI 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.Comment #31
Wim Leers100% 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? :)
Comment #32
tstoecklerSo 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?Comment #33
Wim LeersNo.
That's why the IS says this:
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.Comment #34
tstoecklerAhh 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.Comment #35
Wim LeersNo 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.Comment #36
dawehnerIt 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.Comment #37
dawehnerI'm fine with the patch as it is though.
Comment #38
larowlanThis should link to the change record right? So I think we need a change record too?
Comment #39
Wim LeersGood 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".
Comment #41
Wim LeersCompletely unrelated test changes, seemingly with the database. Retesting.
Comment #42
Wim LeersFYI: #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!
Comment #43
larowlanShould we use the constant here?
Comment #44
Wim LeersYes!
Comment #45
Wim LeersPer #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.
Comment #46
Wim Leers#2910211: Allow computed exposed properties in ComplexData to support cacheability. landed, this is now unblocked.
Comment #49
Wim LeersYay!
Comment #52
xjmThis 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?
Comment #53
Wim LeersTt definitely wasn’t supposed to be backported. Thanks!
Comment #55
timmillwoodHal is now referencing a rest class but doesn't depend on rest, so it breaks!
Comment #56
timmillwoodJust opened #2931765: Regression: \Drupal\hal\LinkManager\LinkManagerBase implicitly depends on REST module to fix this.
Comment #57
Wim LeersBecause the https://www.drupal.org/project/jsonapi module was based off of the HAL module originally, this problem also exists in that module. See #2952714: Group module's GroupAccessResult::allowedIfHasGroupPermission(s)() does not include cacheability.