Use cache collector for state

Looks wonderfully simple!

1. +++ b/core/core.services.yml
   @@ -397,7 +397,9 @@ services:
   +    arguments: ['@keyvalue', '@cache.bootstrap', '@lock']
   

   Is some of the State used even on non-HTML responses? If it is, then I think @cache.bootstrap is fine. Otherwise, I think introducing @cache.state may make sense?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/KernelDestructionSubscriber.php
   @@ -64,7 +64,7 @@ public function onKernelTerminate(PostResponseEvent $event) {
   -    $events[KernelEvents::TERMINATE][] = array('onKernelTerminate', 100);
   +    $events[KernelEvents::TERMINATE][] = array('onKernelTerminate', -100);
   

   Interesting. Let's document why.

3. +++ b/core/lib/Drupal/Core/State/State.php
   @@ -22,19 +25,17 @@ class State implements StateInterface {
   -   * Constructs a State object.
   +   * Constructs an State object.
   

   I think the original was correct.

I also found #2154461: Consider persistent caching and/or CacheCollector for state system again, where we thought about using the cache separately for each item. With bootstrap/fast chained, that might also be a valid option and less risky if state is mis-used. Thoughts?

So over there, this was said:

1. Add a core state storage implementation that uses the cache API to cache each individual query, and default to it.
2. Add an implementation that uses CacheCollector so that these end up as a single query.

The second is tempting, but it does have the potential disadvantage that rarely used state keys could pollute the cache for all pages - for example cron timings, deleted fields and other stuff that's rarely accessed but in the same system.

It seems like the potential for damage is relatively big. If even a single contrib/custom module creates big State entries (think hundreds of kilobytes or maybe even megabytes), then every page is slowed down massively. With the chained inconsistent consistent cache backend that uses APCu, it seems like it may be better to have multiple small I/Os instead.

+++ b/core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php
--- a/index.php
+++ b/index.php

+++ b/index.php
+++ b/index.php
@@ -8,6 +8,11 @@

@@ -8,6 +8,11 @@
  * See COPYRIGHT.txt and LICENSE.txt files in the "core" directory.
  */
 
+$uprofiler_path = '/var/www/html';
+include_once $uprofiler_path . '/uprofiler_lib/utils/uprofiler_lib.php';
+include_once $uprofiler_path . '/uprofiler_lib/utils/uprofiler_runs.php';
+uprofiler_enable(UPROFILER_FLAGS_NO_BUILTINS + UPROFILER_FLAGS_MEMORY);
+
 use Drupal\Core\DrupalKernel;
 use Symfony\Component\HttpFoundation\Request;
 
@@ -20,3 +25,9 @@

@@ -20,3 +25,9 @@
 $response->send();
 
 $kernel->terminate($request, $response);
+
+$uprofiler_data = uprofiler_disable();
+$uprofiler_runs = new uprofilerRuns_Default();
+$namespace = 'd8';
+$id = $uprofiler_runs->save_run($uprofiler_data, $namespace);
+print "<a href='http://localhost/uprofiler_html/?run=$id&sort=excl_wt&source=$namespace'>uprofiler</a>";
 

This is why it failed :)

Once that's fixed, this is ready.

Improved the IS.

Regarding profiling: see #6.

GET http://ec2-52-89-208-32.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes).

I asked testbot 3083 to be killed.

We're still using state in https://api.drupal.org/api/drupal/core!modules!field!field.purge.inc/fun...

I'd be happier if we moved that to k/v first, or at least made an explicit decision that deleted fields in the bootstrap bin is OK.

So, catch, I think you're saying that the rationale given in #10 by Berdir is insufficiently convincing?

We want to optimize for the 90% use case, where we assume that people will end up using really problematic modules since contrib modules should hopefully get fix and improved by someone if they do it wrong […]

Therefore, the decision is to use the cache collector approach with the bootstrap cache bin, […]

We will look into moving some parts away from state even, e.g. the non admin routes since that is now cached and not needed on every or even many requests, but that's a different issue.

i.e. you feel that the "deleted fields state" must be fixed before this can go in, because it could pollute things too much. But like Berdir said in #10 already, we already have the routing.non_admin_routes state entry, which we should also remove.

Deleting fields feels like a relative edge case, and can be refactored/improved independently of this issue. Furthermore, if I read https://api.drupal.org/api/drupal/core%21modules%21field%21field.purge.i... correctly, then it looks like this can only ever temporarily "polluate" state: while the field is being purged. So it's also self-recovering. Which — if correct — is another reason to do that in a separate issue that this issue does not need to be blocked on?

This inclines me to RTBC it again. Feel free to re-un-RTBC — I may be missing something :)

@Wim I think it's convincing for contrib, but only if core sets a good example as to what's in state vs. not.

We could remove the caching of non-admin routes and just get it from this cache instead.

I have a way to resolve this issue in a BC-compatible way with clear expecations set for contrib:

- Create a decorator, which decorates the state service (instead of changing it)
- Call this decorator state.cached
- Use state.cached instead of state for core services that only store small things

Profit!

Nice work! :)

This got committed by mistake here:

commit 714799324429f301188a1ff9113907728d4c1e18
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Mon Dec 19 09:27:43 2016 +0000

    Issue #2834580 by Berdir: Move locale.translation_status state key to a separate key/value collection

But as far as I can tell, the State cache item never gets set. Moving to critical bug for a revert.

Patch doesn't reverse apply any more, here's a revert patch based on #30.

One line wasn't fully reverted.

Better when you create the patch properly too.

This patch is causing bugs like #3007827: The state cache is not cleared when setting a state, here's a revert patch created manually.

The revert works fine, let's get it in so we can continue working on the original patch.

Committed/pushed the revert to 8.7.x and 8.6.x.

Back to CNW.

The revert has broken PHP 5 testing :(

core/modules/system/tests/src/Functional/System/ShutdownFunctionsTest.php now errors with

1) Drupal\Tests\system\Functional\System\ShutdownFunctionsTest::testShutdownFunctions
null does not match expected type "array".

This fixes it. It might be a separate issue that this revert has exposed. Before the revert there are two callbacks before the callbacks start being called. _system_test_first_shutdown_function and the lock release callback. After the revert there's only _system_test_first_shutdown_function - in PHP 5 seems to be a bug with a foreach where the array being looped over changes. See https://3v4l.org/le78R

The fix is to use while and next for a more reliable process - see https://3v4l.org/clIom

It also needs to prevent serialization

+++ b/core/lib/Drupal/Core/State/State.php
@@ -19,18 +16,20 @@ class State extends CacheCollector implements StateInterface {
+  protected $cache = [];

it brings state here, so needs __sleep()/__wakeup() to prevent static cache from serialization

Baffled as to how the recent commit exposed this, but since HEAD is busted, I just went ahead and committed #55. Leaving this open for #56 and for followup investigation or a proper fix since the patch in #55 changes code that hasn't been touched in a year. Maybe it should be opened as a separate issue?

@xjm yes it it baffling. I think it's another thing we broke when we converted code for the each() deprecation. But the break was hidden because other changes including this one meant that we had more than just the _system_test_first_shutdown_function in $callbacks. This meant when _system_test_first_shutdown_function adds yet another callback we weren't at the end of the loop and the test didn't break. When this issue was reverted it became the only callback and therefore it broke as per https://3v4l.org/le78R

I ponder given the random set of things that has happened to this issue if we should close it and start with a fresh issue.

Since we're now back to where we were before this got erroneously committed as part of another issue (see #39) setting the issue metadata back to where it is.

I've opened #3008140: Improve shutdown function testing to add test coverage so there's additional test coverage so we don't get caught out by #55 again.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/KernelDestructionSubscriber.php
   @@ -60,7 +60,10 @@ public function onKernelTerminate(TerminateEvent $event) {
   -    $events[KernelEvents::TERMINATE][] = ['onKernelTerminate', 100];
   +    // Run this subscriber after others as those might use services that need
   +    // to be terminated as well or run code that needs to run before
   +    // termination.
   +    $events[KernelEvents::TERMINATE][] = ['onKernelTerminate', -100];
   

   This change feels quite impactful. Is there not an argument for adding another tag - something like needs late destruction

   I worry that something is contrib is expecting to come after this. Like will this break supercache? http://grep.xnddx.ru/search?text=onKernelTerminate&filename=

2. +++ b/core/lib/Drupal/Core/State/State.php
   @@ -16,20 +19,26 @@ class State implements StateInterface {
   +      @trigger_error('The cache.bootstrap service must be passed to State::__construct(), it is required before drupal:10.0.0.', E_USER_DEPRECATED);
   ...
   +      @trigger_error('The lock service must be passed to State::__construct(), it is required before drupal:10.0.0.', E_USER_DEPRECATED);
   

   The format for these is @trigger_error('Calling ' . __METHOD__ . '() without the $image_factory argument is deprecated in drupal:9.1.0 and is required in drupal:10.0.0. See https://www.drupal.org/node/3173719', E_USER_DEPRECATED);

I think changing automated cron to 101 is a good idea. And opening the follow-up to discuss late destruction / changing the priority.

Patch addresses #80, #79 and #77.

Created the change record https://www.drupal.org/node/3177901 and updated issue summary.

One alternative idea I just had was to skip caching large entries automatically, we'd just ignore them and don't call persist() on them, just pass them through. Basically a strlen() > 10/100kb or so? I think the overhead of that would be fairly minimal.

Seems like a good idea. Maybe this could be baked into the cache collector pattern? Like isn't this true for all use-cases?

@Berdir said:

One alternative idea I just had was to skip caching large entries automatically, we'd just ignore them and don't call persist() on them, just pass them through. Basically a strlen() > 10/100kb or so? I think the overhead of that would be fairly minimal.

@alexpott responded:

Seems like a good idea. Maybe this could be baked into the cache collector pattern? Like isn't this true for all use-cases?

+++ b/core/core.services.yml
@@ -462,7 +462,9 @@ services:
+    arguments: ['@keyvalue', '@cache.default', '@lock']

+++ b/core/lib/Drupal/Core/State/State.php
@@ -16,20 +24,26 @@ class State implements StateInterface {
+      $cache = \Drupal::cache('discovery');

Isn't discover and default two separate bins?

Thanks, a couple of more observations:

• Do we need BC tests here? I reckon we don't need them.
• Do we want to collect some before and after stats here?

Not sure if there is a faster way to get the size of an array or object than serializing it. This means an extra serialize call which is not so great. Only on a set/cache miss, but still.

This bit feels really tricky. The most likely stuff to fall into this size restriction is the entity type info... here's the rows greater than 10000 on a real site I'm working on which has plenty of custom entity types:

| entity.definitions.installed                         | contact_message.field_storage_definitions                              |          8609 |
| entity.definitions.installed                         | path_alias.field_storage_definitions                                   |          9576 |
| entity.definitions.installed                         | site__referrer.field_storage_definitions                               |         11044 |
| entity.definitions.installed                         | site__request.field_storage_definitions                                |         11469 |
| entity.definitions.bundle_field_map                  | easy_email                                                             |         11526 |
| entity.definitions.installed                         | site__offer_group.field_storage_definitions                            |         11847 |
| entity.definitions.installed                         | codes_pool_collection.field_storage_definitions                        |         12234 |
| entity.definitions.installed                         | file.field_storage_definitions                                         |         12357 |
| entity.definitions.installed                         | site__region.field_storage_definitions                                 |         12797 |
| entity.definitions.installed                         | message.field_storage_definitions                                      |         14694 |
| entity.definitions.installed                         | site__store.field_storage_definitions                                  |         18551 |
| entity.definitions.installed                         | site__referee.field_storage_definitions                                |         18919 |
| entity.definitions.installed                         | block_content.field_storage_definitions                                |         20156 |
| entity.definitions.installed                         | taxonomy_term.field_storage_definitions                                |         21079 |
| entity.definitions.installed                         | site__file_upload_type.field_storage_definitions                       |         22231 |
| entity.definitions.installed                         | association_channel_map.field_storage_definitions                      |         22562 |
| entity.definitions.installed                         | site__leads.field_storage_definitions                                  |         22725 |
| entity.definitions.installed                         | comment.field_storage_definitions                                      |         23232 |
| entity.definitions.installed                         | site__banner.field_storage_definitions                                 |         25463 |
| entity.definitions.installed                         | site__offer.field_storage_definitions                                  |         27384 |
| entity.definitions.installed                         | user.field_storage_definitions                                         |         27716 |
| entity.definitions.installed                         | menu_link_content.field_storage_definitions                            |         29164 |
| entity.definitions.installed                         | site__form_submission.field_storage_definitions                        |         30170 |
| entity.definitions.installed                         | site__sub_offer.field_storage_definitions                              |         34275 |
| entity.definitions.installed                         | site__custom_form.field_storage_definitions                            |         36089 |
| entity.definitions.installed                         | site__channel.field_storage_definitions                                |         41726 |
| entity.definitions.installed                         | easy_email.field_storage_definitions                                   |         42517 |
| entity.definitions.installed                         | site__customer.field_storage_definitions                               |         46881 |
| entity.definitions.installed                         | node.field_storage_definitions                                         |         50498 |
| entity.definitions.installed                         | site__association.field_storage_definitions                            |         60462 |
| state                                                | routing.non_admin_routes                                               |        191093 |

/me wonders how often routing.non_admin_routes is used...

"Only on a set/cache miss, but still." means that if you have something above the defined size, it will be a cache miss every time it is requested.

Could we cache the size that we calculate in the cache collector, with a special format or whatever so it's distinguishable from actual data? Then rather than recalculating each time we'd just know we have to look it up. Then it really would only be done on cache misses.

#3120301: RoutePreloader: prevent preloading of routes generated by JSON:API fixes the massive route preload entry for me. So now this issue will add a double cache.

Found this because this is an issue that has already been committed but still open. In fact I see two commits.

Maybe we should do a new issue as this is indeed pretty confusing with the reverts and bugfixes being committed there.

I agree. (From #65)

Not that I recommend continuing in this issue, but if it makes sense to do so, can the Issue Summary be brought up to date?

Try to fix cs errors.

version of the patch from #106 without the tests, to have a patch that applies on D9. test changes are excluded to avoid rerolls for D9

sorry about the empty patch, here is again patch 106 with no test

Based on your last comment this is postponed #2363351: Cached services can't be used in service providers/modifiers

Deprecation messages need updating. Same for CR please

All I did was to update two trigger_error messages.

Should we be moving some keys out of state to a separate collection? I was thinking about the recent development settings page since that's only accessed on container rebuilds. Not sure what that collection would be called though, like state-2...

We're comparing this to Drupal 7 variables which loaded the entire table of state + config on every request so we're already quite a bit more efficient, but yeah it does seems a waste.

So this issue doesn't stall should this be NW for #137?

@smustgrave if we did #136 it would be needs work for that, but if we do #137 it would not, instead we'd open a spin-off and go ahead here.

To keep the issue from stalling going to mark it then. If we need a follow up for #136 can do that for everyone here.

I am reviewing issues that I have bookmarked that were committed and then reverted.

As I read #136 and #137 there is not yet agreement on the approach so setting this to needs review for a decision on that point.

There are failing tests and this should be converted to an MR. I will do that now.

There was a conflict when applying the patch so here is the interdiff.

I am also tagging no-needs-review-bot to emphasize that a decision should be made before working on the MR.

Question who can make the decision though?

@berdir or @catch could you make the calls the remaining tasks? Not sure who can.

@berdir's idea is to opt-in individual keys for caching.

My idea is two have two services/collections - one for cached state things, one for infrequent.

I think two collection is more robust - no chance of something being requested both cached and uncached, but we need to name it, and we also need to decide whether the new or old collection is the one that gets the caching.

Once possibility would be state vs. state_cached (or state.cached) and move the frequent stuff over to it.

Consider moving some examples in core to a different key value collection to have fewer bad examples, coming back to the BC concerns though. I don't think we need to provide BC for example for the twig development mode setting though?

This sounds good and I also don't think we need to provide BC for that one (hopefully none of the ones we move, but definitely not that one).

The $settings opt-in also sounds like a good plan.

#149 I could go for either of those approaches, not sure which is best, but at least it'll be temporary.

From https://git.drupalcode.org/project/drupal/-/merge_requests/5636/pipeline... looks like we're saving approx 10 database queries on nearly every page request, which is great and what I'd expect based on how many state checks we do usually.

Also there's a possibility it's a fluke since occasionally we get runs this fast already, but the full test run was 7m53 seconds with all functional tests finishing under 4 minutes - it may be enough of a change that it's making test runs faster overall. Shouldn't get too excited until that's happened a couple more times though.

Consider moving some examples in core to a different key value collection to have fewer bad examples, coming back to the BC concerns though. I don't think we need to provide BC for example for the twig development mode setting though?

Yeah I think that's fine - we can add a change record and the specifics of whether and when keys are set aren't part of the public API, obviously if contrib is actually relying on something a lot we'd need to think about it more carefully, but we can do that case-by-case.

Checking on two separate production databases that I work on:

> select name, length(value) from key_value where collection='state' and length(value) > 10000;
+--------------------------------------+---------------+
| name                                 | length(value) |
+--------------------------------------+---------------+
| drupal_css_cache_files               |         30824 |
| routing.non_admin_routes             |         16115 |
| simple_sitemap.queue_stashed_results |       8136706 |
| system.js_cache_files                |         70931 |
+--------------------------------------+---------------+
4 rows in set (0.005 sec)

> select name, length(value) from key_value where collection='state' and length(value) > 10000;
+--------------------------------------+---------------+
| name                                 | length(value) |
+--------------------------------------+---------------+
| drupal_css_cache_files               |         10519 |
| simple_sitemap.queue_stashed_results |       6331055 |
| system.js_cache_files                |         11710 |
+--------------------------------------+---------------+
3 rows in set (0.001 sec)

Looks like simple_sitemap should be storing whatever that data is somewhere else.

Reproducing that manually in blackfire with a user without any permissions in umami gives me _very_ different numbers and what I expect. -4 state queries, +1 cache_default query, 33/3 in total. And not 17/18.

One thing to note is that cache requests in the performance tests include those from chained fast, so if blackfire is ignoring those, it'll be a huge difference.

I think now that it's opt in, it would make more sense to move this to cache_discovery so we use the fast-chained backend for it

Seems sensible to me. isn't it using bootstrap already? If so that's already using chained fast.

RefreshVariablesTrait calls $state->resetCache() which in the MR calls clear() which resets both the persistent and static caches, it needs to be ::reset() instead. Pushed a commit. Test should fail in the right direction now.

#156 makes the change in the performance test look right to me, however we're seeing other test failures in runs which aren't consistent. I think the issue is that any test using state to track.. well.. state between requests, now needs to either switch to WaitTerminateTestTrait or to use the key value service directly without the state wrapper - because the cache will be updated in terminate. Pushed a commit for LanguageNegotiationContentEntityTest which was failing locally for me.

Before I found this, I thought I found a race condition in the state implementation - the cache entries were being invalidated before key value is written to. That's not the cause of the test failures after all, but I still think it's a valid change.

Got to a green test run, but had to do some horrible things in LanguageNegotiationContentEntityTest and some of the test failures are random so could still be a couple of false positives.

I think the proper approach to those is going to be switching from state to raw key value in the test modules/test classes, if we do that we can probably undo TerminateWaitTestTrait. However this at least confirms that all of the test failures are due to test and tested site going out of sync when explicitly using the state API to track/alter things in the test/test modules themselves, and not real bugs caused by the change at all.

Any recommendation for reviewing this one?

I think the main remaining things are:

1. We should open issues for taking things out of state and into key/value directly which are accessed extremely rarely. The new development settings stuff that's only on container rebuild / form save are the obvious ones, there are probably more though.

Also an issue against simple_sitemap would make sense too.

2. Decide whether the changes in core/modules/language/tests/src/Functional/LanguageNegotiationContentEntityTest.php are acceptable here with a follow-up to move the test modules to use key/value, or whether we should refactor those test modules to use key/value in this issue. This also goes for the various test modules used in the tests that are getting WaitTerminateTestTrait added. I could go either way here - refactoring the test modules adds scope, but it'll be less churn because we then wouldn't be adding WaitTerminateTestTrait and removing it again.

Tagging needs follow-up since we need that for #1 even if we need to make a decision on #2.

I'll take care of the performance stuff real quick

Went green on my local (the performance stuff)

@berdir see #3414373: Route preloading doesn't run on dynamic page cache hits.

There are some cold/lukewarm cache tests in the Umami performance tests which don't do query assertions yet, I've been hesitant to add assertions to them while we had so many random test failures, but we should probably try that now. This isn't the same as no dynamic page cache at all though so we still might want that too.

I would assume the extra get is coming from the collector going to the cache once to get all of the state stuff, whereas before the state stuff were non-cache-related queries, which showed up in the list of individual queries we're tracking.

Quick guess: The cache get that is saved in RoutePreloader shows up in those test cases where the cache get count does not go up by one. The ones where it does go up, RoutePreloader doesn't save a cache get and state introduces one extra get. Just a guess, though.

Edit: Cross-posted with @catch and my guess seems to match what he's saying.

Fixed tests after #3113971: Replace REQUEST_TIME in services

Last fail is Drupal\Tests\media\FunctionalJavascript\MediaSourceOEmbedVideoTest::testMediaOEmbedVideoSource, I'm not getting any wiser from the artifacts. I'm not seeing anything related to media in the changes either.

Worth running the job again to see if it's an existing random. I don't remember that one specifically but there's been a lot of random media test failures.

The test reported already but only once in #2829040-188: [meta] Known intermittent, random, and environment-specific test failures and error is different, so maybe it's real issue with redirect?

Behat\Mink\Exception\ExpectationException: Current page is
    "/media/add/test_media_oembed_type", but "/admin/content/media" expected.

it's because of error The provided URL does not represent a valid oEmbed resource. for https://vimeo.com/7073899

traced down the test locally (reproducible) til \Drupal\media\OEmbed\ResourceFetcher::fetchResource()

cURL error 28: Operation timed out after 5003 milliseconds with 0 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://172.30.1.2/media_test_oembed/resource?url=https://vimeo.com/7073899&altered=1

Why can't I do a fast-forward pull on this branch? Happened twice now already 🤔

Re #172, are we sending a GET request to Vimeo every time we run our tests? I'm not sure how happy Vimeo would be with us doing so :)

@kristiaanvandeneynde no, there's mapping in state to return local file and somehow this request is failed.

I used to add debug print and I see that request failed before the controller returns result(

Very interesting work. Xhprof lead me to this issue, as I am tracking down a performance issues in a D10.2.x —site.

select name, length(value) from key_value where collection='state' and length(value) > 10000;

 custom_fs_connector.basic_data          | 808712
 custom_fs_connector.info_type_name_list |  27342
 custom_room_info                            | 304317
 custom_fs_connector.course_code_list    | 101678

This is information that not often change, tough by reading through the issue and try to understand what is going on, I would say, this data should not be stored here but the site custom module. From my understanding is this data that not often change.

Ah yes, now that you mention it: ResourceController::setResourceUrl() uses state.

Then the failure makes slightly more sense, we're changing how state works and now a test using state is failing.

Rebased, I expect the performance tests to fail as I couldn't figure out what the new expected counts were supposed to be from the conflicts.

Also reverted the AutomatedCron event subscriber priority back to 100 and the docs change in core.api.php, I think now that needs_destruction is handled after all terminate events (following #3412556: Allow needs_destruction services to run on page cache hits) that all terminate event subscribers are safe to use state.

@kristiaanvandeneynde fast forward pulls do not work when the branch has been rebased and force pushed, most MR contributors tend to use the rebase method instead of merging in 11.x as merge commits are sometimes tricky to work with and it makes the set of commits in the MR cleaner.

Maybe a can of worms but is this another use for a feature flag module over settings.php? We could enable the feature flag module on new installs but existing sites can opt in over time, and eventually we can deprecate/remove it?

Changed my mind, this is more low level and belongs in settings.php.

MediaSourceOEmbedVideoTest sets state during the test and then expects the site under test to read that, but now we can't (easily) invalidate the site's state cache so the test doesn't have the effect it thinks it does. To solve this I switched the test ResourceController to use key value instead of state.

Also updated the performance test counts, and updated the change record a bit.

ExtensionList::getPathNames() appears to wrap cache around state, we can perhaps drop the cache wrapper in a followup.

In general this looks really good! I'm wondering why we're adding the WaitTerminateTestTrait without calling its method, though? I couldn't find anything in core that automatically calls that method if it's present either.

Removed WaitTerminateTestTrait seemingly with no harm done.

Removed the cache wrapper from ExtensionList, hoping this will remove another cache get.

edit: it did not, but it should still save some cache accesses.

Just went over all the changes again and they look good to me. I would just like to see a follow-up so we don't forget to change the state_cache setting in D11.

Also wondering if we need a deprecation notice for dropping the $cache argument from RoutePreloader, although I couldn't find any other example of that in core or the documentation. The docs describe what to do when A, B, C becomes A, C; but not what to do if it becomes A, B. I would err on the side of adding a trigger_error still, using func_get_args().

I have no other objections. Good job on figuring out MediaSourceOEmbedVideoTest, by the way! Setting to NW for the 2 points above but feel free to ping me when those are addressed and I'll gladly set this to RTBC.

Thanks for reviewing, opened #3436954: Remove state_cache setting in Drupal 11.

#160 asked for some other followups, so leaving the tag in place for now.

Will add a deprecation for the RoutePreloader argument, and also change LanguageNegotiationContentEntityTest to use keyvalue in this issue, given we have precedent in the ResourceController for the OEmbed test.

Also opened #3436960: simple_sitemap.queue_stashed_results should be in keyvalue instead of state, unsure if @catch wants to open any more followups or not - leaving the tag in place for now.

Added a release note given we are adding a new setting here.

The tests failed a couple of times but I think I was just unlucky with random fails - they just passed again so hopefully this is all good now.

Tagging as a release highlight, because along with other changes in Drupal 10.3 we're dramatically decreasing the database queries on all page requests, can go in a general 'performance improvements' paragraph.

I opened #3436971: Move the TestWaitTerminate middleware to be added conditionally which I'd been thinking about for a while, but seeing the query in the diff reminded me of it.

Now that tests using state have been updated to k/v here, I think that's everything I was worried about in #160 so removing the tag.

RTBC for me, should fix the 2 drupal:10.0.0 references to drupal:10.3.0, though. Holding off on those grounds.

Fixed the deprecation version, don't know what I was thinking there.

In a followup I think we should add a note to https://api.drupal.org/api/drupal/core%21core.api.php/group/info_types/10 and perhaps https://www.drupal.org/docs/develop/drupal-apis/state-api/state-api-over... mentioning that the state store is meant for frequently used and infrequently changed small pieces of data, and the key-value store should be used for larger or more frequently changing data.

Agreed. Code looks good to me now, so RTBC.

One more follow-up, I opened #3437162: Move twig_debug and other development toggles into raw key/value.

Really nice to be able to commit this one, we've come a long way since variable_get().

I had to resolve a merge conflict on 10.3.x, it was trivial, let's see whether I did I right or not...

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

This feels like a good heuristic:

Sites with more than 100 entries and/or a total value size/length of 100'000

Wonder if we should add a hook_requirements status check for this to help people before Drupal 11 enables this for good?

Question: we have a sub-class of State in ECA, how should we declare the constructor and the service, if we want to support both Drupal 10.2 and 10.3? This seems impossible with this change, or am I missing something?

I found https://git.drupalcode.org/project/eca/-/commit/3bb22ea5820fd81f72e46c44...

Since this is a subclass with its own service definition, why pass the extra services into your own constructor for both 10.2 and 10.3 - they won't be used in 10.2 but that seems OK?

Unfortunately not. We have an extra service to inject and our service declaration looks like this:

  eca.state:
    class: Drupal\eca\EcaState
    parent: state
    arguments: ['@datetime.time']

So, for 10.3 the constructor needs to look like this:

public function __construct(KeyValueFactoryInterface $key_value_factory, CacheBackendInterface $cache, LockBackendInterface $lock, TimeInterface $time) {

but for 10.2 it needs to look like that:

public function __construct(KeyValueFactoryInterface $key_value_factory, TimeInterface $time) {