Memory backend defined as a service in core.services.yml

Let's try this.

Ermm, sorry. Maybe this.

4: 2113169-4.patch queued for re-testing.

What a wonderful patch.

+++ b/core/core.services.yml
@@ -7,8 +7,6 @@ services:
-  cache.backend.memory:
-    class: Drupal\Core\Cache\MemoryBackendFactory

If we remove this, then it is not possible to do something like this:

$settings['cache']['views_results'] = 'cache.backend.memory';

in your local.settings.php during development, right?

Yes, true. However, we don't want this memory backend in the container all the time when we basically use it for testing.

Oh nice, thanks!

Oh, wasn't aware that you can do this, this is the relevant snippet:

    // Add site specific or test YAMLs.
    if (!empty($GLOBALS['conf']['container_yamls'])) {
      $this->serviceYamls = array_merge($this->serviceYamls, $GLOBALS['conf']['container_yamls']);
    }

Which is a bit weird, because what you need to do there is specify a .yml file, that then contains the definitions. With the full path.

There also appears to be 0 documentation about this? Certainly not in default.settings.php. And searching on google for container_yamls only finds references to the code. Not a problem with this issue, of course.

However, this does mean that the use case in #8, which I think is very useful, now means that you need two separate lines in settings.php and a .yml file somewhere and you need to figure out how to name it and so on. Not sure that's worth saving a single definition in there, what's the cost of that?

Ah, the second thing that I forgot to mention, i'm not sure if it's correct to just replace the cache factory with the memory factory, as they have no common interface. It works, because the provide the same method, but it seems a bit fragile...?

RE #12, I think this should be the job of devel or something to add the memory backend if it wants to. I don't think we should be leaving stuff in the container for debugging purposes, even if it is just one service. This IMO kind of opens up other thing potentially being left in with "It's just one service" ?

i'm not sure if it's correct to just replace the cache factory with the memory factory, as they have no common interface. It works, because the provide the same method, but it seems a bit fragile...?

Whats the point in having different factories if we can't swap them out? If this is deemed too fragile (which you're right on), we should add a generic factory interface or something?

Created #2143011: Add a CacheFactoryInterface. How about that?

Because it's a different kind of factory, you're swapping the factory that asks the backend specific factory for the backend specific factory. They happen to have the same method and arguments, but there's no specific contract that enforces that. Maybe the cache factory will get additional methods at some point that the other one doesn't provide.

If everyone thinks it's not a problem, then I'm fine with it, I just wanted to point that out.

The KeyValueFactory looks very similar to CacheFactory and the memory backend is registered in install.core.inc like this:

$container->register('keyvalue.memory', 'Drupal\Core\KeyValueStore\KeyValueMemoryFactory');
...
$settings['keyvalue_default'] = 'keyvalue.memory';

Do you think we should do the same with cache.backend.memory?

OK guys, this does make sense, as CacheFactory is not the same as Memory and Database factories, so shouldn't really be swapped out like that I guess.

How about we do this instead, and just go with what olli is suggesting and just add the MemoryBackendFactory, and set that as the default?

I also updated related patch in #15 based on this.

I'm fine with doing #18 as a follow up.

+++ b/core/lib/Drupal/Core/DependencyInjection/UpdateServiceProvider.php
@@ -35,7 +35,9 @@ public function register(ContainerBuilder $container) {
+      ->register("cache_factory", 'Drupal\Core\Cache\CacheFactory');

This needs the container and settings.

I think we should just do this now, in this issue. As most of the patch had to change anyway :)

Good spot with the settings reference arg.

Need the cache bins to go with the proper cache factory.

Related: #2156265: KeyValueFactory is swappable, add an interface and fix type-hints needing #4 for the keyvalue.

Rerolled.

The issue referenced in #24 removed the lines in #17 and key value memory factory is now registered like in #4. Back to that approach?

I think what that issue has done is not correct actually. See comments #13 - #16. #2156265: KeyValueFactory is swappable, add an interface and fix type-hints Looks like it has replaced the factory of factories with one of the factories, which seems wrong. It's replacing something that the factory should call with that directly. As berdir pointed out in the comments I mentioned above, this is coincidental that the name/signatures are the same.

@olli, actually, yes, I think you're right - we should/could change that back to how it was originally. We will want to wait for #2143011: Add a CacheFactoryInterface ideally though now.

So we should do this now? Change the implementations for update and DUTB.

It's been a little while since I looked at this :) The CacheFactoryInterface issue is in though.

Silly mistake. Need to use the actual factory!

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -752,6 +752,7 @@ protected function setUp() {
+    $conf['container_service_providers']['WebTestServiceProvider'] = 'Drupal\simpletest\WebTestServiceProvider';
…
+ * Contains \Drupal\simpletest\WebTestServiceProvider.
…
+class WebTestServiceProvider implements ServiceProviderInterface {

I don't think this is possible, because the Simpletest module is not necessarily installed by a web test.

While the memory cache backend override makes sense for DUTB tests, I do not think we want to manipulate the environment of regular web tests in any way?

I don't think this is possible, because the Simpletest module is not necessarily installed by a web test.

This could be very true, I will move to a core namespace.

I do not think we want to manipulate the environment of regular web tests in any way?

We already modify settings to change the default cache backend. Are you saying revert back to that approach? I don't see how switching out the factory really hurts us here? Either way, we're going to be using the memory cache backend.

31: 2113169-31.patch queued for re-testing.

For web/integration tests, swapping out backends with in-memory implementations has the opposite of the intended effect: Data is no longer shared across requests (as it is cached in memory only and thus rebuilt on every single HTTP request).

I do not think we want to manipulate the environment of regular web/integration tests in any way. Doing so invalidates the one and only point of performing web/integration tests in the first place.

If there are any other existing manipulations in HEAD already (aside from test database/environment re-routing), then those should be removed/reverted.

Ok, had a look into this, the modification to set the default cache backend to memory for web tests doesn't even work. So let's just remove it al together, yeah?

Thanks! That's a nice + seemingly long overdue clean-up!

FWIW, it was a strange surprise for me to see a crazy amount of new environment/Settings hacks in the setUp() methods of DUTB + WebTestBase when I dived back into D8 core very recently. The original goal was to remove as many of those as possible and not to introduce tons of new ones.

Given the amount of overrides that are currently happening in our tests, I'm tempted to say that our tests became borderline pointless today — they're testing an artificial environment that doesn't exist (at all) in practice, in particular (and that's worst), WebTestBase::setUp(), which seemingly isn't afraid of hacking and injecting tons of data into the test environment, before and after the installer.

In light of that (ab)use, I think it was a really big mistake to allow Settings to be replaceable in any way. Just grepping for "new Settings" and looking at the amount of explicit and implicit re-instantiations, the fundamental idea of allowing hard-coded Settings to be replaced with new ones will be borderline impossible to revert and the unexpected side-effects of that will keep us busy for the next couple of years.

Anyway, this patch contributes to getting rid of a few unnecessary Settings overrides (which were unnecessary in the first place), so this is definitely good to go! :)

Rerolled, don't need quotes change in UpdateServiceProvider anymore. That's already been changed.

The change in WebTestBase will cause another conflict for #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead, but I have to re-roll that anyway after the spin-off issues have landed.

Re-roll. Patch no longer applied.

Just a reroll. This can go back to rtbc.

Re-roll.