Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In the event that #2795209: Allow streamed responses to be cached by Page Cache isn't eligible for cherry picking to 8.2.x, here's a patch that just does what this issue title says, so that BigPipe can implement a subclass of PageCache that includes a terminate() method (see that issue's patch for why) without needing to duplicate all of the caching logic that is currently inlined into fetch().
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | page_cache_store_response.patch | 2.67 KB | effulgentsia |
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #3
Wim LeersThis is pure refactoring.
Comment #4
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIn terms of whether this should be allowed in a patch release, I don't know, but I think so.
The summary of http://semver.org/ says "PATCH version when you make backwards-compatible bug fixes.", and this is not a bug fix. However, http://semver.org/#spec-item-7 says "Minor version... MAY be incremented if substantial new functionality or improvements are introduced within the private code.", and presumably, MAY means it is not a requirement, and this patch only touches private code (moves code from a protected
fetch()method into a new protectedstoreResponse()method, without changing the behavior offetch()in any way).Meanwhile, https://www.drupal.org/core/d8-allowed-changes#patch says "Internal code cleanup that improves maintainability and is not disruptive may sometimes be backported to a patch release, but this is always at committer discretion.", so we might be ok in terms of that as well.
Therefore, leaving at 8.2.x for now, and correspondingly moved #2657684-93: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits to that as well.
Comment #5
xjmSo to commit this to 8.2.x, we'd at least need to prefix the new method name with an underscore for that branch. That way we avoid risk of a method name collision with anything subclassing this class. See here: https://www.drupal.org/core/d8-bc-policy#underscore
Usually we only apply that strategy in case of backporting major bugfixes, though. I have some reservations about the precedent we'd be setting by backporting this exclusively to support a feature in an experimental module. Need to think about that more, so tagging.
The change itself looks great to me and is a clean issue scope. We also don't need to hold committing this to 8.3.x on either bit above.
Comment #6
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI discussed this with @catch, and he has reservations about trying to get #2657684: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits into 8.2 at all, per #2657684-94: Refactor BigPipe internals to allow a contrib module to extend BigPipe with the ability to stream anonymous responses and prime Page Cache for subsequent visits. Therefore, closing this as a duplicate of #2795209: Allow streamed responses to be cached by Page Cache. Unless someone sees value in getting this into 8.3 before the rest of #2795209: Allow streamed responses to be cached by Page Cache is RTBC'd, in which case, please re-open it.
Comment #7
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedI want to get this in rather than the Streamed responsed cached by page cache.
Per Wim Logic (TM) a use case to be supported generically in core should have at least 2 provided use cases, but only BigPipe would ever be using the cache streamed responses, so znerol has rightly reservations about having page cache support that natively.
Instead it indeed makes sense for BigPipe to enhance + decorate the page cache - if both are enabled dynamically during a compile step.
Overall I think page cache is a generic enough service that it would be fine to have public fetch / store methods instead of just handle() - but that is another discussion entirely - which as a co-maintainer of page cache I should first have with znerol ;).
So this is a good task to get in for 8.3.x.
Comment #8
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedActually it is even RTBC.
Comment #9
alexpottCommitted 8cc3eea and pushed to 8.3.x. Thanks!
This looks like sensible re-factoring and there no functional change.