Make CachePluginBase::generateResultsKey() to pass valid data to CacheContextsManager::convertTokensToKeys

Created on 20 June 2015, about 9 years ago
Updated 30 January 2024, 5 months ago

Problem/Motivation

https://qa.drupal.org/pifr/test/1076308

According the PHPDocs in the CacheContextsManager the calling method is REQUIRED to send an array of strings. Currently it is not doing so, and the assert statement reveals this. Whether or not this is an underlying cause of a greater issue is unknown.

Steps to reproduce

NA

Proposed resolution

- Assert that cache context keys and tokens are strings in CacheContextManager::convertTokensToKeys and ContextCacheKeys::__construct()
- In QueryArgsCacheContext::getContext(), when the value is an array, return a query string with the query argument and all values separated by "&", by using the existing http_build_query().

Remaining tasks

Review MR
Commit

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Cache 

Last updated about 17 hours ago

Created by

🇺🇸United States Aki Tendo

Live updates comments and jobs are added and updated live.
  • VDC

    Related to the Views in Drupal Core initiative.

  • Runtime assertion

    It deals with the addition of an assert() statement(s) to the code, and/or contains a test patch where one is failing indicating a need to change code or the documentation the statement was based on.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇳🇿New Zealand quietone New Zealand

    This was a bugsmash daily target. I ran the test that is in the patch and it fails, so I assumed this still needed to be done. larowlan confirmed that in slack.

    This also needs an issue summary update, see Write an issue summary for an existing issue for guidance.

  • 🇳🇿New Zealand quietone New Zealand

    Trying to improve the title

  • Status changed to Needs review over 1 year ago
  • 🇮🇳India bhaveshithape

    Adding Rerole for d10, As previous patch failed to apply on d10 and also most of the files already had the changes so there is minor change in the patch.

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    Regarding #46

    also most of the files already had the changes

    How so? I'm checking D10.1 and I don't see the changes for a few of those files. Also this lost test coverage so adding that tag back

    Was previously tagged for issue summary which still needs to happen as well.

  • First commit to issue fork.
  • Status changed to Needs review 6 months ago
  • 🇬🇷Greece dimitriskr

    I've altered the existing test to support the new format brought by $cache_context->getContext()

  • Status changed to Needs work 6 months ago
  • 🇺🇸United States smustgrave

    Issue summary section for solution is missing.

    Tagging for submaintainer to make sure they agree on the approach

  • Status changed to Needs review 6 months ago
  • Status changed to Needs work 6 months ago
  • 🇨🇭Switzerland Berdir Switzerland

    Those asserts are not doing anything, see MR comment.

  • Status changed to Needs review 5 months ago
  • Status changed to Needs work 5 months ago
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    I feel like #27.1 was never addressed. My assumption is also that if CacheableMetadata::merge() has $result = clone $this;, then it seems kind of moot to assert($result instanceof CacheableMetadata);.

    Comment #28 also touched on the ampersand but not sure if we really need that.

    From a cache submaintainer point of view, I see nothing wrong here. Assertions are harmless, casting casche context return values to strings is too because worst case scenario that leads to a few cache entries no longer being reachable, at which point they get recalculated and stored again. Only been subsystem maintainer for a few months now, so will ask @catch before removing tag.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    This looks fine conceptually, still NW per my latest comment, though.

Production build 0.69.0 2024