Problem/Motivation

When creating a cache.$bin_name service, documentation for the cache API implies (but does not assert) that the service arguments: should be set to [$bin_name].

However, nothing in the code enforces this, and it's entirely possible to configure a cache service with service name and bin name different e.g:

services:
  cache.foo:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin, default_backend: cache.backend.memory }
    factory: cache_factory:get
    arguments: [foo_bar]

This service will register fine, and appear to work fine, but will suffer from undefined behaviour; for example, its default_backend will not be respected.

The problem can be demonstrated with the following class:

namespace Drupal\cacheproblem;
class CacheProblem {
    public function __construct(CacheBackendInterface $cacheBackend) {
    $this->cacheBackend = $cacheBackend;
  }

  public function test() {
    print "Previously:\tupdate=" . $this->cacheBackend->get('update') . "\n";

    $string = date('c');
    $this->cacheBackend->set('update', $string);
    print "Cached update:\t$string\n";

    print "Now set to:\tupdate=" . $this->cacheBackend->get('update') . "\n";
  }
}

With the correct memory backend, this should not remember cached values between requests i.e. all calls should be the same as the first one.

With cache.foo and arguments: [foo]

This response reflects the correct behaviour:

$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously:	update=
Cached update:	2017-01-20T15:30:18+00:00
Now set to:	update=2017-01-20T15:30:18+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously:	update=
Cached update:	2017-01-20T15:30:23+00:00
Now set to:	update=2017-01-20T15:30:23+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously:	update=
Cached update:	2017-01-20T15:30:28+00:00
Now set to:	update=2017-01-20T15:30:28+00:00

The cache never "warms up", because it's correctly in memory.

With cache.foo and arguments: [foo_bar]

This response reflects incorrect/undefined behaviour, when the first argument does not match the service name:

$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously:	update=
Cached update:	2017-01-20T15:29:34+00:00
Now set to:	update=2017-01-20T15:29:34+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously:	update=2017-01-20T15:29:34+00:00
Cached update:	2017-01-20T15:29:38+00:00
Now set to:	update=2017-01-20T15:29:38+00:00
$ drush php-eval '(new Drupal\cacheproblem\CacheProblem(\Drupal::cache("foo")))->test();'
Previously:	update=2017-01-20T15:29:38+00:00
Cached update:	2017-01-20T15:29:54+00:00
Now set to:	update=2017-01-20T15:29:54+00:00

The cache has (incorrectly) warmed up between requests, because it is now storing the data in a cache_foo_bar SQL table, not in memory:

mysql> SELECT * FROM cache_foo_bar;
+--------+---------------------------+--------+----------------+------------+------+----------+
| cid    | data                      | expire | created        | serialized | tags | checksum |
+--------+---------------------------+--------+----------------+------------+------+----------+
| update | 2017-01-20T15:44:41+00:00 |     -1 | 1484927081.763 |          0 |      | 0        |
+--------+---------------------------+--------+----------------+------------+------+----------+
1 row in set (0.00 sec)

Proposed resolution

How this can be resolved isn't clear, but behaviour should be consistent and undefined behaviour warned about:

* Either the DI container needs to spot that the configuration is inconsistent (if that's even possible) and abort
* Or the cache & backend integration needs to be impervious to the DI service name.

Remaining tasks

1. Work out what the tasks are!

User interface changes

None.

API changes

None (this is undefined behaviour.)

Data model changes

None.

Comments

jp.stacey created an issue. See original summary.

jp.stacey’s picture

Title: When cache.X service is configured with arguments: [not_X] - behaviour is undefined » When cache.X service is configured with arguments: [X_Y] - behaviour is undefined
jp.stacey’s picture

Issue summary: View changes

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

jp.stacey’s picture

Issue tags: +SprintWeekend2017

Looking at this as part of the Sprint Weekend 2017.

jp.stacey’s picture

Here's some of the things I've found out:

1) when caches are rebuilt, ListCacheBinsPass::process() retrieves all services tagged name: cache.bin and uses the service ID (e.g. cache.X to determine the bin-to-backend correspondence. These compiled settings are then added as a parameter on the container.

2) when a cache service is retrieved, the factory method CacheFactory::get() is called with arguments: [Y], using the zeroth argument (e.g. Y) as the bin name. It then uses this bin name to try to look up the backend from the settings parameter compiled above.

This is where the inconsistency arises. What we do about it is another question! Respectively:

1) when caches are rebuilt, only the service ID and the "attributes" (the tags) are currently available. But maybe more of the configuration could be discovered for each service ID in turn, and the arguments checked against the bin name?
2) when a cache service is retrieved, the original service ID is never passed to the factory method. So we can't detect an inconsistency here.
3) entirely separately, the status report page could warn of inconsistent cache configuration.

jp.stacey’s picture

Status: Active » Needs review
StatusFileSize
new1.64 KB

Patch attached, making ListCacheBinsPass::process() test for consistency.

There's no existing test for this class that I can extend, but maybe it could be added as part of this issue if I've got time.

jp.stacey’s picture

StatusFileSize
new5.39 KB

Patch attached: the same changes in executed code; but with a new test.

The new test covers most of the existing ListCacheBinsPass::process() behaviour, then also at the end tests what happens when an inconsistent service definition is passed in.

I appreciate that my proposed resolution here in executed code—to strictly enforce consistency and raise an exception—might be up for discussion, but at least now it's easier to test the final behaviour by interdiffing from this patch.

Would appreciate any feedback!

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). 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.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.

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.

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.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.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.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.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now 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.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now 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.

catch’s picture

Title: When cache.X service is configured with arguments: [X_Y] - behaviour is undefined » Add validation for cache bin service arguments
Category: Bug report » Task
Priority: Major » Normal
Status: Needs review » Needs work

Some validation seems sensible, but throwing an exception would potentially break existing sites as soon as they update. Maybe an assert() so it only breaks on local development?

Also think this mostly a DX improvement and not a bug as such, so downgrading to a task.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.