Problem/Motivation

Discovered in #3302800: Core tests need to filter out deprecated themes when looping over all themes we have config in help_topics module that references Seven:

core/modules/help_topics/config/optional/block.block.seven_help_search.yml:theme: seven

This needs to move to Seven itself so we can deprecate and remove Seven from core.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3303787-14.patch3.32 KBandypost
#14 interdiff.txt807 bytesandypost
#11 interdiff_6-11.txt1014 bytesravi.shankar
#11 3303787-11.patch3.3 KBravi.shankar
#6 3303787-6.patch3.09 KBandypost
#6 interdiff.txt509 bytesandypost
#5 3303787-5.patch2.73 KBandypost
#5 interdiff.txt2.39 KBandypost
#2 3303787-2.patch345 byteslongwave

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3303787-2.patch345 bytes

Tested on 9.5.x with

vendor/bin/drush si
vendor/bin/drush then seven
vendor/bin/drush en help_topics

Logged in as admin and switched the admin theme to Seven. The "Search help" block is correctly placed on /help.

Status: Needs review » Needs work

The last submitted patch, 2: 3303787-2.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented

Nice

1) Drupal\KernelTests\Config\DefaultConfigTest::testThemeConfig with data set "seven" ('seven')
Drupal\Core\Extension\Exception\UnknownExtensionException: The module seven does not exist or is not installed.

/var/www/html/core/lib/Drupal/Core/Extension/ExtensionList.php:346
/var/www/html/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php:235
/var/www/html/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php:136
/var/www/html/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.ph
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt2.39 KB
new 3303787-5.patch2.73 KB

it needs changes in test, this test still fail with 

Testing /var/www/html/web/core/tests/Drupal/KernelTests/Config

block.block.seven_help_search provided by seven does not exist after installing all dependencies
 /var/www/html/web/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php:236
 /var/www/html/web/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php:133
 /var/www/html/web/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php:61
 /var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:726
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt509 bytes
new 3303787-6.patch3.09 KB

kinda it

The last submitted patch, 5: 3303787-5.patch, failed testing. View results

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Needs work

Looks good IMHO except these two comments and a question :)

  1. +++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
@@ -127,10 +127,10 @@ protected function assertExtensionConfig(string $name, string $type): void {
     // Test configuration in the module's config/install directory.
...
     // Test configuration in the module's config/optional directory.

    module => extension 

  2. +++ b/core/tests/Drupal/KernelTests/Config/DefaultConfigTest.php
@@ -229,9 +231,9 @@ protected function doTestsOnConfigStorage(StorageInterface $default_config_stora
+        $info = $this->container->get("extension.list.$type")->getExtensionInfo($extension);

    Is this the best API we have for getting an extension list of $type?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Re #8.2 I think the only other way would be to pass in the full service name instead of just the type. There is extension.path.resolver but that can only give us the filename and not the parsed contents.

#3272093: Cache bin names should be set from service tags, not the service name is kinda similar for cache bins. Maybe we need an extension.list service collector that can provide the relevant tagged service? (out of scope here, but maybe worth a followup)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Yeah out of scope here. Let's get the code comments fixed and get this in then IMHO :)

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 3303787-11.patch3.3 KB
new interdiff_6-11.txt1014 bytes

Made changes as per comment #8.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Re #8 there's change record https://www.drupal.org/node/2709919 so all core using this pattern - using substitution for extension type

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Looks good now :) Thanks a lot!

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new interdiff.txt807 bytes
new 3303787-14.patch3.32 KB

The only nit is to add type-hint and BC for new argument if anything using it in contrib/custom

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Those are great points, thanks!

  • catch committed 4cc760e on 10.0.x 
    Issue #3303787 by andypost, ravi.shankar, longwave, Gábor Hojtsy: Move...
  • catch committed 2a0cfe5 on 10.1.x
    Issue #3303787 by andypost, ravi.shankar, longwave, Gábor Hojtsy: Move...
  • catch committed bdf69e7 on 9.5.x
    Issue #3303787 by andypost, ravi.shankar, longwave, Gábor Hojtsy: Move...
catch’s picture

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

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

Status: Fixed » Closed (fixed)

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