Problem/Motivation

Ist still uses the old array structure. Nobody noticed, because nothing in core calls it, not even tests. Only if you add token.module, then it fails on e.g. form submissions.

Proposed resolution

Fix the cache tag, add unit tests.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#3 core-token-reset-info-2420025-3-interdiff.txt1.35 KBberdir
#3 core-token-reset-info-2420025-3.patch3.94 KBberdir
#1 core-token-reset-info-2420025-1-patch-only.patch1.38 KBberdir
#1 core-token-reset-info-2420025-1.patch3.8 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new core-token-reset-info-2420025-1.patch3.8 KB
new core-token-reset-info-2420025-1-patch-only.patch1.38 KB

Also changed it to inject the invalidator, makes unit testing easier, with minimal impact (hopefully).

The test only patch is a bit bogus, due to the refactoring.

Also, unit testing this is somewhat limited, as we could have already had one that expected the old structure and would have passed too. But you can always have wrong mocking in unit tests and I'm also not sure about adding test modules and stuff just to be able to test it in a web/kernel test.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Looks great! Just a few problems… one of which should cause the test to blow up :) 

  1. +++ b/core/core.services.yml
@@ -977,7 +977,7 @@ services:
+    arguments: ['@module_handler', '@cache.discovery', '@language_manager', 'cache_tags.invalidator']

    Missing leading '@'. 

  2. +++ b/core/lib/Drupal/Core/Utility/Token.php
@@ -106,10 +114,11 @@ class Token {
    * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
    *   The language manager.
    */
-  public function __construct(ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManagerInterface $language_manager) {
+  public function __construct(ModuleHandlerInterface $module_handler, CacheBackendInterface $cache, LanguageManagerInterface $language_manager, CacheTagsInvalidatorInterface $cache_tags_invalidator) {

    Docblock wasn't updated.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new core-token-reset-info-2420025-3.patch3.94 KB
new core-token-reset-info-2420025-3-interdiff.txt1.35 KB

The last submitted patch, 1: core-token-reset-info-2420025-1.patch, failed testing.

The last submitted patch, 1: core-token-reset-info-2420025-1-patch-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: core-token-reset-info-2420025-3.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
Segmentation fault
FATAL Drupal\migrate_drupal\Tests\d6\MigrateDrupal6Test: test runner returned a non-zero error code (139).

Ugh. Everything but not segfaults, please...

Berdir queued 3: core-token-reset-info-2420025-3.patch for re-testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Quickfix, +D8 cacheability
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed c9cc835 and pushed to 8.0.x. Thanks!

  • alexpott committed c9cc835 on 8.0.x 
    Issue #2420025 by Berdir: Token::resetInfo() uses invalid cache tag...

Status: Fixed » Closed (fixed)

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