Problem/Motivation

The \Drupal\views\Plugin\views\cache\Time plugin using to inject Request service instead of using - $this->view->getRequest()

It was added in #2477157: rest_export Views display plugin does not set necessary cache metadata

Proposed resolution

Do not inject request

Remaining tasks

- deprecate argument for 10.0 (no usage found)
- decide on better way to use request in view
- review/commit

User interface changes

no

API changes

\Drupal\views\Plugin\views\cache\Time no longer require request as argument

Data model changes

no

Release notes snippet

CommentFileSizeAuthor
#14 3109110-14.patch4.04 KBandypost
#14 interdiff.txt3.39 KBandypost
#13 3109110-13.patch4.04 KBkapilv
#10 3109110-10.patch4.04 KBandypost
#10 interdiff.txt3.33 KBandypost
#8 3109110-08.patch1.88 KBandypost
#8 interdiff.txt1.5 KBandypost
#6 3109110-6.patch2.35 KBandypost
#6 interdiff.txt1.43 KBandypost
#5 3109110-5.patch2.01 KBandypost
#5 interdiff.txt594 bytesandypost
#3 3109110-3.patch1.69 KBandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
Parent issue: #3109109: AccountForm should read pass-reset-token only from query string » #2477157: rest_export Views display plugin does not set necessary cache metadata
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
StatusFileSize
new 3109110-3.patch1.69 KB

Kind of itm this never used

Status: Needs review » Needs work

The last submitted patch, 3: 3109110-3.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt594 bytes
new 3109110-5.patch2.01 KB

So only 5 times plugin created within tests

Now test cleaner patch

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt1.43 KB
new 3109110-6.patch2.35 KB

The only place where requests is needed

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue summary: View changes
StatusFileSize
new interdiff.txt1.5 KB
new 3109110-08.patch1.88 KB

Reroll patch for 9.1 - picked from #3109109-8: AccountForm should read pass-reset-token only from query string (needs to give a credit to @kkalashnikov)

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work
Issue tags: -Needs change record +Needs tests

Filed CR https://www.drupal.org/node/3154016

Needs work for deprecation test

+++ b/core/modules/views/src/Plugin/views/cache/Time.php
@@ -156,7 +161,7 @@ protected function getLifespan($type) {
-      $cutoff = REQUEST_TIME - $lifespan;
+      $cutoff = $this->view->getRequest()->server->get('REQUEST_TIME') - $lifespan;

this change should be done in context of #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new interdiff.txt3.33 KB
new 3109110-10.patch4.04 KB

Added tests for constructor and BC fshim for request property (views still has properties public)

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

NW for 9.2

kapilv’s picture

kapilv commented
StatusFileSize
new 3109110-13.patch4.04 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.39 KB
new 3109110-14.patch4.04 KB

Re-roll for 9.2 and fix deprecation test

andypost’s picture

andypost
he/him
Primary language Russian
commented

Green! I bet the Request_Time global could be replaced by the view's request

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

Looks good, the service is never used, so sounds good to take it out. We have a follow up to deal with REQUEST_TIME in #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service, and a CR.

  • catch committed 9a3bb08 on 9.2.x 
    Issue #3109110 by andypost, KapilV, Lendude: Deprecate request argument...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x, thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone commented

Publish the change record