Problem/Motivation

According to @quietone: "Took another look and everything about this doc block is wrong, including the @param. We need someone who knows views to sort this one out."

   * Determine expiration time in the cache table of the cache type
   * or CACHE_PERMANENT if item shouldn't be removed automatically from cache.
   *
   * Plugins must override this to implement expiration in the cache table.
   *
   * @param $type
   *   The cache type, either 'query', 'result'.

Proposed resolution

   * Determine cache expiration time.
   *
   * Plugins must override this to implement expiration in the cache table. The
   * default is CACHE_PERMANENT, indicating that the item will not be removed
   * automatically from cache.
   *
   * @param string $type
   *   The cache type.

Remaining tasks

Patch
Review
Commit

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#3 3250482-3.patch1.03 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daffie created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Title: The docblock of Drupal\views\Plugin\views\cache::cacheSetMaxAge() is wrong » The docblock of \Drupal\views\Plugin\views\cache\CachePluginBase::cacheSetMaxAge() is wrong
Issue summary: View changes

@daffie, thanks for making this issue.

Added details to the IS and updated the title.

Regarding the $type parameter, there are other places in the file that state 'The cache type, either 'query', 'result'.' But I found that it can be can be 'output', see \Drupal\views\Plugin\views\cache\Time::getDefaultCacheMaxAge. I found that \Drupal\views\ViewExecutable::execute sets the type to 'results'. I did not find an instance where 'query' was used.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
3250482-3.patch1.03 KB

And a patch implementing the proposed resolution.

rajveergangwar’s picture

rajveergangwar
Primary language English
Location Delhi
CreditAttribution: rajveergangwar as a volunteer and commented
Status: Needs review » Reviewed & tested by the community

Hi @Quietone,

Patch looks good to me. Marking it review and tested by community.

daffie’s picture

daffie CreditAttribution: daffie commented

+1 for RTBC.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

Starting tests

  • catch committed 266fe1a on 9.4.x 
    Issue #3250482 by quietone, daffie: The docblock of \Drupal\views\Plugin...

  • catch committed 4b233f8 on 9.3.x 
    Issue #3250482 by quietone, daffie: The docblock of \Drupal\views\Plugin...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

This probably never got updated since before cache tags and views were a thing.

Committed/pushed to 9.4.x and cherry-picked to 9.3.x, thanks!

rajveergangwar’s picture

rajveergangwar
Primary language English
Location Delhi
CreditAttribution: rajveergangwar as a volunteer and at TO THE NEW commented

i didn't recieved credit? is something i need to do it ?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

@rajveergang a 'looks good to me' isn't generally enough to receive credit, it would need to be more like 'this looks good to be because [reason] [reason]' referring to why the patch resolves the problem. However, generally it's hard to get credit for reviewing a trivial patch unless you find an issue with it, or can document research done.

rajveergangwar’s picture

rajveergangwar
Primary language English
Location Delhi
CreditAttribution: rajveergangwar as a volunteer and at TO THE NEW commented

@catch,

You are right. I will keep in mind next time :)

Status: Fixed » Closed (fixed)

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