Problem/Motivation

VIewsData uses 'views.settings':skip_cache in order to skip its caching. This is an additional setting, without the actual need for it otherwise on runtime.

Proposed resolution

Get rid of the usage and let people just use a different cache backend for '@cache.discovery'. In case they really care they could also inject the null backend
by using a custom services.yml file.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#47 2541074-47.patch988 byteslendude
#40 2541074-40.patch12.27 KBlendude
#40 interdiff-2541074-38-40.txt2.27 KBlendude
#38 2541074-38.patch11.62 KBlendude
#38 interdiff-2541074-36-38.txt2.2 KBlendude
#36 2541074-36.patch9.56 KBlendude
#36 interdiff-2541074-34-36.txt524 byteslendude
#34 2541074-34.patch8.95 KBlendude
#33 2541074-33.patch8.67 KBlendude
#7 interdiff-4-6.txt2.53 KBmartin107
#7 viewsData-2541074-6.patch8.87 KBmartin107
#4 viewsData-2541074-3.patch9.77 KBmartin107

Comments

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Performance

.

martin107’s picture

martin107 commented
Assigned: Unassigned » martin107

I am working on this now.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs change record

This probably needs a short doc how to bypass caching for views

martin107’s picture

martin107 commented
Status: Active » Needs review
StatusFileSize
new viewsData-2541074-3.patch9.77 KB

I really like the idea behind this issue... I think it will, in the end look much simpler.

This is not ready for review.... the changes are spread across too many files, 9 , and I have little confidence that I will make all the changes in the first draft.
I am looking to testbot to point out anything I have missed.

What can I say

1) Site install works.
2) ViewDataTest passes.
3) Views UI settings forms test passes.

dawehner’s picture

dawehner
Primary language German
commented

Really like the idea!

  1. +++ b/core/modules/views/src/ViewsData.php
@@ -32,9 +32,9 @@ class ViewsData {
+   * @var \Drupal\Core\Cache\CacheBackendInterface|NULL
    */
   protected $cacheBackend;

    No, this won't be NULL but rather the Null cache backend. its kinda a difference :) 

  2. +++ b/core/modules/views/src/ViewsData.php
@@ -95,22 +88,18 @@ class ViewsData {
+  public function __construct(CacheBackendInterface $cache_backend,  ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager) {

    Two whitespace before ModuleHandlerInterface :P 

  3. +++ b/core/modules/views/src/ViewsData.php
@@ -174,17 +163,17 @@ public function get($key = NULL) {
-    if ($this->skipCache) {
+    if (!$this->cacheBackend) {
       return FALSE;
     }

    There you can cleanup the code also a bit more

damiankloip’s picture

damiankloip commented

Hmm, I am not sure about this tbh. Instead of the config option, you need to:

- Override the service definition for views_data, and inject a Null/memory backend instead
- Use Null/memory backend for the cache_data bin

That's not quite as obvious for 99% of people? But on the other side, do we really need this setting to disable the cache? Can we live without it..

martin107’s picture

martin107 commented
StatusFileSize
new viewsData-2541074-6.patch8.87 KB
new interdiff-4-6.txt2.53 KB

1) Oh Good Grief Charlie Brown -PEBKAC - Ok well on the bright side I learn something new every day!

2) Fixed.

3) Yes, same applies to cacheSet(). If you are supplying a null cache backend its behaviour is well defined ..
Its seems an over-complication to mention that corner case in the methods documentation anymore so I have dropped any mention of it.

Same tests pass locally.

PEBKAC - The Problem exists between keyboard and chair.

dawehner’s picture

dawehner
Primary language German
commented

Write you need both the entry in the settings.php + services.yml file, but still this is quite an edge case, as you wrote.

damiankloip’s picture

damiankloip commented
Related issues: +#2539282: ViewsData__construct should do less
martin107’s picture

martin107 commented
Assigned: martin107 » Unassigned
martin107’s picture

martin107 commented

Can I ask what aspect of the change should be documented in a change record.

That the configuration element views.settings':skip_cache is going away ?

dawehner’s picture

dawehner
Primary language German
commented

Yeah document that the setting is removed and there these are the steps to opt out of that caching.

martin107’s picture

martin107 commented
Assigned: Unassigned » martin107

Thank you .. I am drafting the change record now.

martin107’s picture

martin107 commented
Assigned: martin107 » Unassigned

Here is the first draft of the cr.

dawehner’s picture

dawehner
Primary language German
commented

When defining a new service developers wishing to skip the cache now have the option of removing the usual cache.discovery service and injecting a null cache backend instead.

This is not entirely obvious, to be honest. I would vote for listing all the needed steps.

martin107’s picture

martin107 commented

Fair point.... I am out of time today .... tomorrow is another day

martin107’s picture

martin107 commented

@dawehner - I have a quick question

Are developers commonly going to alter the existing service or provide a parallel service?

SO

A) Should I provide an altering service guide, something very similar to https://www.drupal.org/node/2026959

OR

B) Describe how to provide a new service.

My knowledge of the various use cases is not good enough so say definitively.

dawehner’s picture

dawehner
Primary language German
commented

IMHO we should at a cache.null cache bin in the development.services.yml and provide a commented out example in there which uses this new cache bin

dawehner’s picture

dawehner
Primary language German
commented

And the CR basically document that and point to development.services.yml

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 7: viewsData-2541074-6.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.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.

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

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Version: 9.5.x-dev » 11.x-dev
Category: Bug report » Task
Status: Needs work » Needs review
Issue tags: -Needs change record
StatusFileSize
new 2541074-33.patch8.67 KB

Not a bug I'd say, and not a useful setting, as described, there are other ways to temporarily disable this cache if you are working at ViewsData development, and there is really no other use for this that I see.

Reroll, let's see what I got wrong!

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 2541074-34.patch8.95 KB

CS clean up

Status: Needs review » Needs work

The last submitted patch, 34: 2541074-34.patch, failed testing. View results

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2541074-34-36.txt524 bytes
new 2541074-36.patch9.56 KB

Added update to clear the setting from config, not adding a test for it since the fails in #34 show we have coverage for this.

Status: Needs review » Needs work

The last submitted patch, 36: 2541074-36.patch, failed testing. View results

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2541074-36-38.txt2.2 KB
new 2541074-38.patch11.62 KB

Update tests to match the new flow.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Needs work
Issue tags: +Needs change record 
--- a/core/modules/views/src/ViewsData.php
+++ b/core/modules/views/src/ViewsData.php
-  public function __construct(CacheBackendInterface $cache_backend, ConfigFactoryInterface $config, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager) {
+  public function __construct(CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager) {

I think we need to do the deprecation-dance here (as specified in https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...)?

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2541074-38-40.txt2.27 KB
new 2541074-40.patch12.27 KB

Too right, thanks @Spokje

Something like this I guess.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Needs work

Well, since I'm already being annoying, we kinda need a CR and the deprecation notice needs to link to that.

(Also CRs are being watched by people working on Rector/PHPStan Drupal-related magic)

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review

It has a CR and it is linking to that :)

The NIDs are just eerily similar https://www.drupal.org/node/2541974

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

It has a CR and it is linking to that :)

Meh!

The NIDs are just eerily similar

Yes, and also in the 2 million-range, so I never expected a CR so old...

I've looked at the code and all seems well enough for an RTBC.

  • catch committed 3f6c5710 on 11.x 
    Issue #2541074 by Lendude, martin107, dawehner, Spokje: Get rid of using...
catch’s picture

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

Committed/pushed to 11.x, thanks!

markdorison’s picture

markdorison
he/him
commented

The associated change record notes this is being introduced in 10.2.0, but the metadata and commit here seem to indicate 11.x. Is the change record incorrect?

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
StatusFileSize
new 2541074-47.patch988 bytes

Good question, since it has an update and removed something, I'm gonna assume it will probably not be backported to 10.2, so quick follow up to adjust the deprecation message to mention the correct versions

catch’s picture

catch
he/him
Primary language English
commented

Just to clarify, 10.2.x will be branched off 11.x around when we get to alpha. https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... has the details.

Status: Fixed » Closed (fixed)

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