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.#2688545: Remove ampNodeViewController in favor of amp_entity_view_alter() adds custom cache context services for the 'amp' and 'warnfix' query arguments, because currently Drupal 8 does not provide cache context variations for query arguments with empty values. #2729439: QueryArgsCacheContext should return a special value for ?arg (without value) may fix this upstream in an upcoming 8.1.x release.
Once #2729439: QueryArgsCacheContext should return a special value for ?arg (without value) lands, we can remove the custom cache context services (which are marked as deprecated).
It is not possible at present to set the AMP module to depend upon a certain 8.1.x release, so we will have to decide if we remove the deprecated services once #2729439: QueryArgsCacheContext should return a special value for ?arg (without value) makes it into 8.1.x or if we wait until 8.2 when we could set 8.2 as the minimum core version.
The downside to not setting 8.2 as a dependency is that somebody could be using an older 8.1.x release without #2729439: QueryArgsCacheContext should return a special value for ?arg (without value), which would lead to caching problems where &warnfix would appear to have no effect. (Somebody could use &warnfix=1 to get around this.)
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2730295-1-remove-custom-cache-contexts.patch | 7.97 KB | RainbowArray |
Comments
Comment #2
mtiftPostponed until #2729439: QueryArgsCacheContext should return a special value for ?arg (without value) reaches a D8 point release
Comment #3
Wim Leers#2729439: QueryArgsCacheContext should return a special value for ?arg (without value) landed.
Comment #4
RainbowArrayThanks. Still not available in a point release yet though. Not going to require people use the dev version of D8 for AMP caching to work properly.
Comment #5
Wim LeersOf course not. In core, we usually then mark the issue active, to not forget about it again, and because it means it can already land in the next version.
Your project, your decision of course :)
Comment #6
Wim Leershttps://www.drupal.org/project/drupal/releases/8.1.2
Comment #7
Wim LeersComment #8
Wim LeersUpdated relevant core issues: #2313917-67: Core version key in module's .info.yml doesn't respect core semantic versioning and #2641658-22: Module version dependency in .info.yml is ineffective for patch releases.
Comment #9
RainbowArrayThis removes the custom cache context services, which are no longer necessary. It's certainly possible somebody could run into an issue if running an older version of 8.1, but that is something that should be avoided anyhow in order to stay up to date on security fixes in core. So I think we should be okay expecting the latest version of 8.1 in order for AMP to function without issue.
Comment #10
RainbowArrayComment #11
RainbowArrayTo test this, view an AMP-enabled page that has content in the body that needs to be converted to AMP with ?amp, without ?amp and with ?amp&warnfix. The first should have content converted to AMP, the second should not, and the third should have warnings at the bottom of the page. You should not need to do drush cr between page changes.
Comment #12
mtiftLooks good! Committed to 8.x-1.x.
Comment #13
Wim LeersYay! Less code!