Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname()

drupal_get_path() is used for much more than PHP files.
Mostly for CSS and JS.

I would agree with almoudoh. Its best to remove drupal_get_path()and replace with module_load_include

With most code now in classed objects, do we even need module_load_include() anymore? I don't think it has a purpose. (If you have enough procedural code to bother breaking into multiple files then most of it should be in classes anyway.)

Thats a nice thought, but it's post-beta and there are 53 usages of module_load_include in HEAD.

If we don't want to go around in loops with #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service, we might change the goal of this issue into striving to replace the other usages of drupal_get_path():

- js files: about 23 usages: replace with module://...?
- css files: about 37 usages: replace with module://...?
- info.yml: 2 usages (and 1 in a comment): replace with $this->moduleHandler()->getModule($moduleName)->getName()
- other .yml: 2 usages: replace with module://...?
- xml files: 4 usages: replace with module://...?
- php files not to be included but to be executed via an HTTP request in tests: 8 usages: can be replaced with module://

In total there are about 181 usages. It seems that many can be replaced with module:// (or theme:// or profile:// or the not yet existing library://) as their usage is in adding css/js resources to a page, and many are in test situations. However, to which degree, do we want to make our tests depend on other functionality like module:// where a small error in the module stream wrapper may lead to 1000's of test failures?

For not test situations: how is the order of loading? I saw some usages of drupal_get_path in the module handler, so that obviously can't be replaced with module://. But how early are the stream wrappers loaded, currently and after turning them into services?

I was not ware of that issue.

#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is in.

I created a change record and I deprecate the drupal_get_path() function.
Please review.
Thanks:).

fixed as per #21

Blocker commited #2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList

🥳

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -820,7 +830,7 @@ public function getPrefixGroupedUserFunctions($prefixes = []) {
    
      /**
   -   * Wraps drupal_get_path().
   +   * Wraps ExtensionList::getPath().
       *
   

   We only needed to wrap the function because it was a function, to make unit tests happy. Now that it no longer is, we can just call $this->moduleList->getPath() directly.

   Not sure if we should keep the protected function getPath(), we might need to, to be extra careful with subclasses.

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -828,7 +838,10 @@ public function getPrefixGroupedUserFunctions($prefixes = []) {
       */
      protected function getPath($module) {
   -    return drupal_get_path('module', $module);
   +    if ($this->moduleList === NULL) {
   +      $this->moduleList = \Drupal::service('extension.list.module');
   +    }
   +    return $this->moduleList->getPath($module);
      }
   

   the common approach is to do the fallback directly in the constructor, possibly with a @trigger_error() if it's missing.

3. +++ b/core/tests/Drupal/Tests/Core/Field/BaseFieldDefinitionTestBase.php
   @@ -65,7 +65,7 @@ protected function setUp() {
      /**
       * Returns the module name and the module directory for the plugin.
       *
   -   * Function drupal_get_path() cannot be used here, because it is not available
   +   * ExtensionList::getPath() cannot be used here, because it is not available
       * in Drupal PHPUnit tests.
       *
   

   Same here, the the description doesn't make sense anymore

Thanks @Berdir

Missed trait

Fix #46 and few other places
I think deprecation of drupal_get_filename() needs separate issue because this patch is already big enough

Remaining usage (todo)
- module_load_include()
- \Drupal\Core\Config\ConfigInstaller::drupalGetPath()
- \Drupal\Core\Config\ConfigManager::uninstall()

@almaudoh Thanks for fixes, meanwhile would be great to check #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service
ATM I'm workin on followup for drupal_get_filename() cos it needs separate CR and will help to simplify the patch a bit

Filed #3017951: Deprecate drupal_get_filename() and replace usage with ExtensionList::getPathname()

Let's see how many fails with #48 interdiff

Updated files getPath instead of __DIR__

oups, status...

Removed __DIR__ for other places in locale module

Replaced with services for batches in update and node modules

status

I find strange that __DIR__ replaced with service call, it reverts #2351919: Replace uses of drupal_get_path() with __DIR__ where possible

Nice summary! Meanwhile ci shows 4 coding standards messages

Hi,
rerolled #67 to trigger again tests
hope it'll be green this time

corrected the increment error in the file name

+++ b/core/lib/Drupal/Core/Config/InstallStorage.php
@@ -18,6 +19,8 @@
+  use ExtensionListingTrait;

@@ -254,7 +257,7 @@ protected function getComponentFolder(Extension $extension) {
-    return drupal_get_path('core', 'core') . '/' . $this->getCollectionDirectory();
+    return $this->getCorePath() . '/' . $this->getCollectionDirectory();

+++ b/core/modules/ckeditor/src/CKEditorPluginBase.php
@@ -30,6 +31,8 @@
+  use ExtensionListingTrait;

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImage.php
@@ -22,7 +22,7 @@ class DrupalImage extends CKEditorPluginBase implements CKEditorPluginConfigurab
+    return $this->getModulePath('ckeditor') . '/js/plugins/drupalimage/plugin.js';

@@ -51,7 +51,7 @@ public function getButtons() {
+        'image' => $this->getModulePath('ckeditor') . '/js/plugins/drupalimage/icons/drupalimage.png',

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImageCaption.php
@@ -46,7 +49,7 @@ public function getLibraries(Editor $editor) {
+    return $this->getModulePath('ckeditor') . '/js/plugins/drupalimagecaption/plugin.js';

@@ -70,7 +73,7 @@ public function getConfig(Editor $editor) {
+      $this->getModulePath('ckeditor') . '/css/plugins/drupalimagecaption/ckeditor.drupalimagecaption.css',

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalLink.php
@@ -20,7 +20,7 @@ class DrupalLink extends CKEditorPluginBase {
+    return $this->getModulePath('ckeditor') . '/js/plugins/drupallink/plugin.js';

@@ -46,7 +46,7 @@ public function getConfig(Editor $editor) {
+    $path = $this->getModulePath('ckeditor') . '/js/plugins/drupallink';

+++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/Language.php
@@ -128,7 +128,7 @@ public function settingsForm(array $form, FormStateInterface $form_state, Editor
+      $this->getModulePath('ckeditor') . '/css/plugins/language/ckeditor.language.css',

+++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
@@ -14,6 +15,8 @@
+  use ExtensionListingTrait;

@@ -81,7 +84,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    $url = $this->getModulePath('ckeditor_test') . '/css/test.css';

+++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
@@ -16,6 +17,8 @@
+  use ExtensionListingTrait;

@@ -336,7 +339,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      'file' => $this->getModulePath('update') . '/update.manager.inc',

This looks the all of usage of the trait, so I think no reason to implement the trait.
Instead, helpers like ones added to testBase classes makes sense

This patch needed a reroll for a conflict in
core/modules/system/src/Form/ThemeSettingsForm.php

I want to try to address #71 after the reroll

oops. missing comma

The patch in #74 was missing the new Trait.

In the patch attached I remove that trait with methods in the CkeditorPluginBase. I also fix the code style issue reported by the CI.

I don't know what is happening with the CI...

Forgot the interdiff

Ok, It was not a good decision to make DrupalImageCaption extend CKEditorPluginBase. Reverting that.

I forgot one getModulePath

Here's the updated patch for 8.8.x with the updated deprecated message.

Looking great!

Just a few nitpicks...

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -235,13 +235,13 @@ public function setProfileDirectoriesFromSettings() {
   +        $this->profileDirectories[] = \Drupal::service('extension.list.profile')->getPath($testing_profile);
   

   Can we create a local variable and typehint?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -509,7 +509,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   +    $service_yaml_file = \Drupal::service('extension.list.module')->getPath($module) . "/$module.services.yml";
   

   Can we inject this?

3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -177,6 +194,11 @@ public function __construct($root, CacheBackendInterface $cache, LockBackendInte
   +      @trigger_error('The extension.list.module service is a required parameter to the Registry constructor in Drupal 9.0.0.', E_USER_DEPRECATED);
   

   Can we use the *almost* standard format? https://www.drupal.org/project/coding_standards/issues/3024461

4. +++ b/core/modules/system/tests/modules/common_test/src/Controller/CommonTestController.php
   @@ -67,9 +67,9 @@ public function jsAndCssQuerystring() {
   +          \Drupal::service('extension.list.module')->getPath('node') . '/css/node.admin.css' => [],
   ...
   +          '/' . \Drupal::service('extension.list.module')->getPath('node') . '/node-fake.css?arg1=value1&arg2=value2' => [],
   

   Can we create a local variable then do assert($var instanceof ModuleExtensionList)?

5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/RegistryTest.php
   @@ -111,7 +113,8 @@ public function testSuggestionPreprocessFunctions() {
   +    $extension_list = \Drupal::service('extension.list.module');
   

   assert($extension_list instanceof ModuleExtensionList)

6. +++ b/core/tests/Drupal/KernelTests/Core/Theme/RegistryTest.php
   @@ -199,7 +203,8 @@ public function testThemeTemplatesRegisteredByModules() {
   +    $extension_list = \Drupal::service('extension.list.module');
   

   Ditto

Fixes for my own feedback in #84 :-)

Updates services yaml.

Fix deprecation message.

rerolling the patch against 8.8.x, am not able to take the interdiff between 89 and 90 as the earlier patch was not applying cleanly.

Here's another attempt at a reroll.

I also updated some of the deprecation messages to the standard format, and added a trigger_error where it was missing.

Fixes deprecation test.

Looks great!
As I see it missing legacy tests for this functions but adds legacy test for constructor which looks not much useful
Also base test classes using duplicated methods - not sure it fits to trait but this is code duplication

1. +++ b/core/includes/bootstrap.inc
   @@ -255,6 +255,18 @@ function config_get_config_directory($type) {
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
   + *   Instead, you should use \Drupal\Core\Extension\ExtensionList::getPathname().
   + *   Where it is not possible to inject the service, you can use the
   + *   \Drupal::service('extension.list.module')->getPathName()
   + *   \Drupal::service('extension.list.profile')->getPathName()
   + *   \Drupal::service('extension.list.theme')->getPathName()
   + *   as appropriate.
   + *
   + * @see https://www.drupal.org/node/2940438
   + *
   + * @see \Drupal\Core\Extension\ExtensionList::getPathname()
     */
    function drupal_get_filename($type, $name, $filename = NULL) {
   

   Maybe this function also should trigger error (depending on $type pointing to valid service)?

2. +++ b/core/includes/bootstrap.inc
   @@ -304,8 +316,14 @@ function drupal_get_filename($type, $name, $filename = NULL) {
    function drupal_get_path($type, $name) {
   +  @trigger_error('drupal_get_path() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Extension\ExtensionList::getPath() instead. See https://www.drupal.org/node/2940438');
   

   otoh it will trigger twice if both functions will use it

3. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2079,4 +2079,20 @@ protected function setHttpResponseDebugCacheabilityHeaders($value = TRUE) {
   +  protected function getModulePath($module_name) {
   +    return $this->container->get('extension.list.module')->getPath($module_name);
   

   other tests also add getThemePath (trait?)

4. +++ b/core/tests/Drupal/KernelTests/Core/Theme/RegistryTest.php
   @@ -255,4 +264,16 @@ public function testThemeTemplatesRegisteredByModules() {
   +   * @group legacy
   +   * @expectedDeprecation Calling Registry::__construct() without the $module_list argument is deprecated in drupal:8.8.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/2940438
   +   */
   +  public function testDeprecatedConstructor() {
   +    $theme_handler = \Drupal::service('theme_handler');
   +    $registry_theme = new Registry($this->root, \Drupal::cache(), \Drupal::lock(), \Drupal::moduleHandler(), $theme_handler, \Drupal::service('theme.initialization'), 'test_theme');
   +    $this->assertNotNull($registry_theme);
   

   Not sure it needed

5. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -1107,4 +1107,36 @@ public static function assertEquals($expected, $actual, $message = '', $delta =
   +  protected function getModulePath($module_name) {
   +    return $this->container->get('extension.list.module')->getPath($module_name);
   ...
   +  protected function getThemePath($theme_name) {
   +    return $this->container->get('extension.list.theme')->getPath($theme_name);
   
   +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -743,4 +743,36 @@ protected function translatePostValues(array $values) {
   +  protected function getModulePath($module_name) {
   +    return $this->container->get('extension.list.module')->getPath($module_name);
   ...
   +  protected function getThemePath($theme_name) {
   +    return $this->container->get('extension.list.theme')->getPath($theme_name);
   

   Could be a trait

NW for message

+++ b/core/includes/bootstrap.inc
@@ -255,6 +255,18 @@ function config_get_config_directory($type) {
+ * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.

@@ -304,8 +316,14 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+ * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0.
...
+  @trigger_error('drupal_get_path() is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Extension\ExtensionList::getPath() instead. See https://www.drupal.org/node/2940438');

The message should use new format `drupal:8.8.0` and so on according to https://www.drupal.org/core/deprecation#how-function

#95:

1. Added the trigger_error()
2. Triggering 2 deprecations seems ok to me
3. Added a getThemePath()
4. Removed
5. Do we really need a trait for this? 🤔

#96 updated deprecation format.

I added the deprecation and realised there were quite a lot of usages that were still in core. I've made an attempt to replace them all, however, not sure what is going to break now.

+++ b/core/lib/Drupal/Core/Config/ConfigManager.php
@@ -238,7 +268,18 @@ public function uninstall($type, $name) {
+    switch ($type) {
+      case 'module':
+        $schema_dir = $this->moduleExtensionList->getPathname($name) . '/' . InstallStorage::CONFIG_SCHEMA_DIRECTORY;
+        break;
+
+      case 'theme':
+        $schema_dir = $this->themeExtensionList->getPathname($name) . '/' . InstallStorage::CONFIG_SCHEMA_DIRECTORY;
+        break;
+
+      default:
+        $schema_dir = NULL;
+    }

This makes me think we haven't got the api right yet!

Went through the patch a bit, not a full review...

1. +++ b/core/includes/bootstrap.inc
   @@ -255,8 +255,21 @@ function config_get_config_directory($type) {
   + * @deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Instead, you
   + *   should use \Drupal\Core\Extension\ExtensionList::getPathname().
   + *   Where it is not possible to inject the service, you can use the
   + *   \Drupal::service('extension.list.module')->getPathName()
   + *   \Drupal::service('extension.list.profile')->getPathName()
   + *   \Drupal::service('extension.list.theme')->getPathName()
   + *   as appropriate.
   

   I think we can still improve this a bit.

   You also need to know the corresponding service name if you do inject it, so maybe it should be something like "Use ExtensionList::getPathname() from the respective service (extension.list.module, extension.list.profile, extension.list.theme)" or so

2. +++ b/core/includes/install.core.inc
   @@ -471,10 +476,9 @@ function install_begin_request($class_loader, &$install_state) {
      // Before having installed the system module and being able to do a module
   -  // rebuild, prime the drupal_get_filename() static cache with the system
   +  // rebuild, prime the module list static cache with the system
      // module's location.
   -  // @todo Remove as part of https://www.drupal.org/node/2186491
   -  drupal_get_filename('module', 'system', 'core/modules/system/system.info.yml');
   +  $module_list->setPathname('system', 'core/modules/system/system.info.yml');
    
   

   doesn't feel like we've really resolved this @todo and the one for user below. We apparently still special-case system and user, I think that @todo doesn't mean to just replace it with the equivalent method/service but completely remove it. I'd just keep the @todo for now?

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -91,11 +121,12 @@ public function buildByExtension($extension) {
          if ($this->moduleHandler->moduleExists($extension)) {
            $extension_type = 'module';
   +        $path = $this->moduleList->getPath($extension);
          }
          else {
            $extension_type = 'theme';
   +        $path = $this->themeList->getPath($extension);
          }
   -      $path = $this->drupalGetPath($extension_type, $extension);
   

   apparently we still need $extension_type below, once..

4. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -400,13 +431,6 @@ protected function applyLibrariesOverride($libraries, $extension) {
   -  /**
   -   * Wraps drupal_get_path().
   -   */
   -  protected function drupalGetPath($type, $name) {
   -    return drupal_get_path($type, $name);
   -  }
   -
   

   we've kept such methods before in similar cases and deprecated them as well. kinda overkill, but might be the safer option?

5. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -667,7 +667,7 @@ protected function getDefaultConfigDirectory($type, $name) {
      /**
   -   * Wrapper for drupal_get_path().
   +   * Wrapper for ExtensionList::getPath().
       *
       * @param $type
   

   looks like this updates the docs but not the actual implementation? Possibly another one we want to deprecate and use an injected service?

6. +++ b/core/lib/Drupal/Core/Config/ConfigManager.php
   @@ -238,7 +268,18 @@ public function uninstall($type, $name) {
    
   -    $schema_dir = drupal_get_path($type, $name) . '/' . InstallStorage::CONFIG_SCHEMA_DIRECTORY;
   +    switch ($type) {
   +      case 'module':
   +        $schema_dir = $this->moduleExtensionList->getPathname($name) . '/' . InstallStorage::CONFIG_SCHEMA_DIRECTORY;
   +        break;
   +
   +      case 'theme':
   +        $schema_dir = $this->themeExtensionList->getPathname($name) . '/' . InstallStorage::CONFIG_SCHEMA_DIRECTORY;
   +        break;
   +
   +      default:
   +        $schema_dir = NULL;
   +    }
   

   can there be anything else?

   I mean we could do something like this, but not sure if that's really better:

   $schema_dir = $this->{$type . 'ExtensionList'}->getPathname().

   That's basically what drupal_get_path/filename does too with "\Drupal::service("extension.list.$type")", so that would fail hard if called with something non-existing as well.

7. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -461,7 +475,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
          // Clear the static cache of the "extension.list.module" service to pick
          // up the new module, since it merges the installation status of modules
          // into its statically cached list.
   -      \Drupal::service('extension.list.module')->reset();
   +      $this->moduleExtensionList->reset();
    
          // Clear plugin manager caches.
          \Drupal::getContainer()->get('plugin.cache_clearer')->clearCachedDefinitions();
   

   there might be a reason we didn't inject that yet, module installer is really funky as it does container rebuilds.

8. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -74,13 +82,16 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
        $this->languageManager = $language_manager;
        $this->renderer = $renderer;
   +    $this->moduleList = $module_list;
      }
   

   missing the deprecation thingy.

9. +++ b/core/tests/Drupal/KernelTests/Core/Bootstrap/GetFilenameTest.php
   @@ -28,6 +29,8 @@ public function register(ContainerBuilder $container) {
      /**
       * Tests that drupal_get_filename() works when the file is not in database.
   +   *
   +   * @expectedDeprecation drupal_get_filename() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Extension\ExtensionList::getPathName() instead. See https://www.drupal.org/node/2940438
       */
      public function testDrupalGetFilename() {
   

   Having a dejavu, but I think this test isn't just a legacy test, that still needs to work now for our installer? so maybe instead rename and use the extension list?

10. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
    @@ -512,3 +509,7 @@ protected function getPath($module) {
     }
    +
    +if (!defined('DRUPAL_MINIMUM_PHP')) {
    +  define('DRUPAL_MINIMUM_PHP', '5.5.9');
    +}
    

    looks like a merge leftover... this was removed.

> there might be a reason we didn't inject that yet, module installer is really funky as it does container rebuilds.

Sounds like we should open a side issue to add a comment about that there, so it doesn't get accidentally 'fixed' in future.

Thanks @Berdir

1. Fixed
2. Fixed
3. Yes? I left as is.
4. Fixed
5. Fixed
6. I was thinking factory? So you could do something like ExtensionListFactory::getExtensionList($type)
7. Reverted those changes.
8. Fixed
9. Changed the name and switched to using the ExtensionLists. I also cleaned up the missing extension test, as it was doing something funky with state??
10. Fixed. Although we needed to require_once bootstrap in order to get the DRUPAL_MINIMUM_PHP constant.

Re: #101.6 here's what I was thinking for an extension list factory. I think it makes things a lot cleaner.

Hmm. Need to special-case some logic for core in ConfigInstaller.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -431,6 +431,19 @@ protected function applyLibrariesOverride($libraries, $extension) {
   +  protected function drupalGetPath($type, $name) {
   +    @trigger_error('drupalGetPath() is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Extension\ExtensionList::getPathname() instead. See https://www.drupal.org/node/2940438', E_USER_DEPRECATED);
   

   maybe include the classname for those methods?

2. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -81,6 +81,7 @@ class RegistryTest extends UnitTestCase {
      protected function setUp() {
        parent::setUp();
   +    require_once $this->root . '/core/includes/bootstrap.inc';
    
   

   hm, I know we don't do that in unit tests because it pollutes all tests. If we need it then I assume we have to re-add the constant, just with the right value.

   Why do we need it now though?

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionListFactory.php
   @@ -0,0 +1,66 @@
   +
   +/**
   + * Factory for getting extension lists by type.
   + */
   +class ExtensionListFactory {
   

   Maybe, but not sure if factory is the right term here? It doesn't actually create them, just allows you to access them through a single dependency?

   Can't really think of a good name right now, ExtensionListRepository also doesn't seem that great?

2. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionListFactoryTest.php
   @@ -0,0 +1,37 @@
   +    $factory = $this->container->get('extension.list.factory');
   +    $this->assertNotNull($factory);
   

   maybe use an assertInstanceOf for these?

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -108,7 +108,12 @@ public function __construct(ConfigFactoryInterface $config_factory, StorageInter
   public function installDefaultConfig($type, $name) {
-    $extension_path = $this->extensionListFactory->get($type)->getPathname($name);
+    if ($type === 'core') {
+      $extension_path = 'core';
+    }
+    else {
+      $extension_path = $this->extensionListFactory->get($type)->getPathname($name);

I suppose that's exactly why we have the core logic in drupal_get_filename() :)

Maybe, but not sure if factory is the right term here? It doesn't actually create them, just allows you to access them through a single dependency?

Can't really think of a good name right now, ExtensionListRepository also doesn't seem that great?

We could:

1. Rename it to ExtensionPathResolver - always liked 'resolver' ;-)
2. Have a getPathname() (getPath()?) method
3. Encapsulate the 'core' special-case handling
4. internally look up the right extension list, and call getPathname() on it

That sounds interesting, I guess most cases that need to call the same method on these services dynamically is the path stuff?

Last weekend I used to dig Advanced help module and filed #3077074: [Meta] Change mentions from module to extension
Module using module & theme handlers to look for read.me(+ bit more) in active extensions
Then a lot of UI/render require to
- get path to use "finder"
- render extension name - that's where it using proxy ModuleHandler::getName()

Here's an attempt at the idea suggested in #106.

Fixed incorrect replacement of getPathname() instead of getPath().

Not a full review, but I spotted this:

+++ b/core/includes/install.core.inc
@@ -471,10 +476,9 @@ function install_begin_request($class_loader, &$install_state) {
-  // @todo Remove as part of https://www.drupal.org/node/2186491

This @todo shouldn't be getting removed.

Found another place that can use ExtensionPathResolver.

Re #113: restores todo

Fix LibraryDiscoveryParserTest fails.

I'm scratching my head over these fails...

1) Drupal\FunctionalTests\Installer\InstallerExistingConfigNoSystemSiteTest::testConfigSync
Exception: Notice: Undefined index: theme
Drupal\Core\Config\ConfigImporter->createExtensionChangelist()() (Line: 435)

In BrowserTestBaseUserAgentTest I comparing the values that provaided before and after changes. So
drupal_get_path('module', 'system') returns path to the directory, but $this->container->get('extension.list.module')->getPathname('system') returns path to the info file.
See the results:

core/modules/system
core/modules/system/system.info.yml

So the main problem is in that test getPathname method call have to be replaced with getPath method call

+++ b/core/includes/install.core.inc
@@ -454,12 +454,17 @@ function install_begin_request($class_loader, &$install_state) {
+  /** @var \Drupal\Core\Extension\ModuleExtensionList $module_list */
+  $module_list = \Drupal::service('extension.list.module');
+  /** @var \Drupal\Core\Extension\ProfileExtensionList $profile_list */
+  $profile_list = \Drupal::service('extension.list.module');

Also, I'm not sure that $profile_list have to be an instance of the extension.list.module service.

Thanks @voleger 🎉 They are definitely bugs, however I don't think that will fix the fails.

From @Berdir slack:

random guess on ..., could it also be due to depency injection?

config importer installs modules, and then the injected service doesn't know about the thing thing that it just installed

I think I need a picture to get my head around that! 🤯

At least we fix the UA related test.
There is more issue related to changes introduced by the patch:

+++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
@@ -79,21 +87,28 @@ class ConfigInstaller implements ConfigInstallerInterface {
-    $extension_path = $this->drupalGetPath($type, $name);
+    $extension_path = $this->extensionPathResolver->getDir($type, $name);

There is an incorrect method call introduced in the ConfigInstaller. getInfoPath method call has to be used instead of getDir method call.

There is an incorrect method call introduced in the ConfigInstaller. getInfoPath method call has to be used instead of getDir method call.

I don't think that's true? Looking at how $extension_path is used on the next line indicates it should be a directory.

if (is_dir($extension_path . '/' . InstallStorage::CONFIG_SCHEMA_DIRECTORY) || $type == 'theme') {
      $this->typedConfig->clearCachedDefinitions();
}

I debugging the code in the context of the InstallerBase tests, so I'm also confused about that. But the return values of the replaced code is different so that looks incorrect.
Also

1. +++ b/core/includes/bootstrap.inc
   @@ -255,8 +255,18 @@ function config_get_config_directory($type) {
   + * @see \Drupal\Core\Extension\ExtensionList::getPathnØame()
   

   Typo here

2. +++ b/core/lib/Drupal/Core/Config/ConfigInstaller.php
   @@ -663,7 +678,7 @@ protected function getProfileStorages($installing_name = '') {
   +    return $this->extensionPathResolver->getDir($type, $name) . '/' . InstallStorage::CONFIG_INSTALL_DIRECTORY;
   

   Checked the return values of the replaced code so in the context of the InstallerBase test here we can get the next values:
   $this->drupalGetPath($type, $name) returns core
$this->extensionPathResolver->getDir($type, $name) returns .
   But if we replace code with $this->extensionPathResolver->getInfoPath($type, $name) that will returns core

I have an assumption that at some phase we set the dirname path to the extension path property instead of the filename of the info file of that extension. Can be that a reason why we can get the correct value from getFilename method instead of getDir method?

Checked the return values of the replaced code so in the context of the InstallerBase test here we can get the next values:
$this->drupalGetPath($type, $name) returns core
$this->extensionPathResolver->getDir($type, $name) returns .
But if we replace code with $this->extensionPathResolver->getInfoPath($type, $name) that will returns core

Aha! That was the problem. I tried to remove that code in the ExtensionPathResolver, but it's required for the 'core' type.

Oh, that makes sense. And also fixes the installer related tests.
+1 for rtbc

+1 to rtbc but I think @Berdir should set it

1. +++ b/core/core.services.yml
   @@ -512,20 +512,32 @@ services:
   +  extension.path.resolver:
   +    class: Drupal\Core\Extension\ExtensionPathResolver
   +    tags:
   +      - { name: service_collector, tag: extension_list, call: addExtensionList }
   

   I like the path resolver, but I'm not 100% sure that we should use an extensible pattern to inject its depenndencies, that implies an extensibility that doesn't actually exist? There is no use case for contrib to add another etension list?

2. +++ b/core/includes/install.core.inc
   @@ -454,12 +454,17 @@ function install_begin_request($class_loader, &$install_state) {
      // Prime drupal_get_filename()'s static cache.
      foreach ($install_state['profiles'] as $name => $profile) {
   -    drupal_get_filename('profile', $name, $profile->getPathname());
   -    // drupal_get_filename() is called both with 'module' and 'profile', see
   +    // Profile path is set in both module and profile lists. See
        // \Drupal\Core\Config\ConfigInstaller::getProfileStorages for example.
   -    drupal_get_filename('module', $name, $profile->getPathname());
   +    $profile_list->setPathname($name, $profile->getPathname());
   +    $module_list->setPathname($name, $profile->getPathname());
      }
   

   I guess it's off-topic to fix that here, although we're actually specifically changing ConfigInstaller::getProfileStorages() here.

3. +++ b/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php
   @@ -0,0 +1,92 @@
   +  public function getInfoPath($type, $name) {
   ...
   +   */
   +  public function getDir($type, $name) {
   +    return dirname($this->getInfoPath($type, $name));
   

   Why different method names here than on the extension lists? I agree that getPath() vs getPathname() is a bit confusing, but IIRC, that was discussed and explicitly decided to be consistent with https://www.php.net/manual/de/splfileinfo.getpathname.php and https://www.php.net/manual/de/splfileinfo.getpath.php.

   The class name is also ExtensionPathResolver.

   Also, we don't shorten things like that, so should be getDirectory() if we stick with that.

4. +++ b/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php
   @@ -0,0 +1,92 @@
   +    // Type 'core' only exists to simplify application-level logic; it always
   +    // maps to the /core directory, whereas $name is ignored. It is only
   +    // requested via ::getDir(). /core/core.info.yml does not exist, but is
   +    // required since ::getDir() returns the dirname() of the returned pathname.
   +    if ($type === 'core') {
   +      return 'core/core.info.yml';
   +    }
   

   If it's only called through getDir() then why not implement it there?

5. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -80,7 +88,14 @@ protected function setUp() {
        $this->streamWrapperManager = $this->createMock(StreamWrapperManagerInterface::class);
   -    $this->libraryDiscoveryParser = new TestLibraryDiscoveryParser($this->root, $this->moduleHandler, $this->themeManager, $this->streamWrapperManager);
   +    $this->extensionPathResolver = $this->prophesize(ExtensionPathResolver::class);
   +    $this->libraryDiscoveryParser = new TestLibraryDiscoveryParser(
   

   I'm a bit confused about our two mocking libraries in core to be honest.. other places use createMock(), this uses prophecy, it's not about consistency either as the call above is createMock() ;)

6. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -620,3 +641,6 @@ public function setFileValidUri($source, $valid) {
    }
   +if (!defined('DRUPAL_MINIMUM_PHP')) {
   +  define('DRUPAL_MINIMUM_PHP', '5.5.9');
   

   still confused by this, as mentioned should at least use the same php version?

7. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -1,12 +1,8 @@
   -/**
   - * @file
   - * Contains \Drupal\Tests\Core\Theme\RegistryTest.
   - */
   -
   

   not really in scope ;)

8. +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -2079,4 +2079,36 @@ protected function setHttpResponseDebugCacheabilityHeaders($value = TRUE) {
   +   *   The drupal-root relative path to the module directory.
   +   *
   +   * @throws \Drupal\Core\Extension\Exception\UnknownExtensionException
   +   *   If the module does not exist.
   +   */
   +  protected function getModulePath($module_name) {
   +    return $this->container->get('extension.list.module')->getPath($module_name);
   +  }
   +
   +  /**
   +   * Gets the path for the specified theme.
   

   we certainly don't need these methods on WebTestBsae and I'm also not sure if we should if others need it. Maybe it should be getExtensionPath($type, $name) and use the extension path resolver?

   kernel tests have 18 calls to their getModulePath() (and just 2 for getThemePath()), most of them for hardcoded method names that often violate https://www.drupal.org/node/3034299, for example all the ckeditor calls.

   And most that aren't their own module is for simpletest files, so something that we need to move out of there anyway.

   browser tests do have a bit more calls, and whlie most are also a hardcoded module, most need to relative path for checking http requests/html content, so __DIR__ doesn't work there. Maybe we could just keep a getExtensionPath() method there?

   Just thinking about loud, happy to hear counter-arguments on most of these (not WebTestBase, though :))

On #133.6, I did run the test without that line with run-tests.sh and phpunit directly and it's not needed?

There is no use case for contrib to add another etension list?

I suppose it is, at least db drivers screams about it

Why extension system should be not extensible?

Thanks for the review @Berdir!

1. Fixed. I removed the service collector, and added a constructor.
2. Left this one alone.
3. Fixed. Renamed to getPathname() and getPath() as this was previously agreed to.
4. Fixed. Moved to getPath()
5. Fixed. Changed to createMock()
6. Fixed. Removed.
7. Reverted. However, since we've touched this file, it will fail code sniffer.
8. I removed getThemePath() as it wasn't used. I also replaced a few usages in \Drupal\aggregator\Tests\AggregatorTestBase although that is deprecated anyway.

> Why extension system should be not extensible?

Because it's not? ExtensionPathResolver has a hardcoded list of allowed types and validates against that.

+++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
@@ -339,7 +339,7 @@ public function getEmptyOpml() {
    */
   public function getRSS091Sample() {
-    return $GLOBALS['base_url'] . '/' . $this->getModulePath('aggregator') . '/tests/modules/aggregator_test/aggregator_test_rss091.xml';
+    return __DIR__ . '../../tests/modules/aggregator_test/aggregator_test_rss091.xml';
   }

yeah, these are exactly problematic examples. This isn't about a local path, it's about a URL that is then requested in the test.

WebTestBase is fully deprecated and we shouldn't have any calls to WebTestBase::getModulePath() in this patch anyway?

I've reverted changes to WebTestBase and sub-classes, removing getModulePath()

Found that #105.2 still without answer why we need this change.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php
@@ -53,7 +53,7 @@ public function testDrupalSettingsCachingRegression() {
-    $fake_library = 'fakeLibrary/fakeLibrary';
+    $fake_library = 'ajax_test/fakeLibrary';

Also wondering why we need this change also. Does that mean influence on the library definitions?

Thanks @voleger.

1. I've removed bootstrap and are now defining DRUPAL_MINIMUM_PHP in the test setup.
2. 🤷‍♂️ no idea. I've reverted.

#140.1 now it is better. Instead of including the whole bootstrap file just defining the required constant. And that is great, let's keep it for the issue related to moving those constant into namespaced class. See #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions.

#140.2 Does that mean that change introduced in the patch has influence on the caching?

Not sure.

I also re-removed the file docblock as this is now failing code sniffer as we've touched this file.

Reroll and one more fix

Set retest against the latest changes in core. If green +1 for RTBC

Needs reroll

Reroll + few fixes, also found few new mentions in comments

I think this looks good, I did several reviews and I really like the ExtensionPathResolver service that we added here, it allows us to keep a similar/simple API to get the path of any type of extension and made many of the replacements much easier as we previously needed to inject 3 different services to get the same result.

If #2908079: Move some of the bootstrap.inc PHP-related constants to \Drupal and deprecate the old versions lands first (and it is also RTBC), then we can drop the constant definition here.

1. Thanks for working on this

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php
   @@ -0,0 +1,94 @@
   +   * @var array
   

   @var \Drupal\Core\Extension\ExtensionList[]

3. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -148,6 +149,20 @@ class Registry implements DestructableInterface {
   +  /**
   +   * The theme initialization.
   +   *
   +   * @var \Drupal\Core\Theme\ThemeInitializationInterface
   +   */
   +  protected $themeInitialization;
   
   +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -2,20 +2,21 @@
   +use Drupal\Core\Config\ConfigFactoryInterface;
   +use Drupal\Core\Extension\ModuleHandlerInterface;
   +use Drupal\Core\Extension\ThemeExtensionList;
    use Drupal\Core\Extension\ThemeHandlerInterface;
    use Drupal\Core\File\Exception\FileException;
    use Drupal\Core\File\FileSystemInterface;
   +use Drupal\Core\Form\ConfigFormBase;
    use Drupal\Core\Form\FormStateInterface;
    use Drupal\Core\Render\Element;
    use Drupal\Core\StreamWrapper\PublicStream;
    use Drupal\Core\StreamWrapper\StreamWrapperManager;
   +use Drupal\Core\Theme\ThemeManagerInterface;
    use Symfony\Component\DependencyInjection\ContainerInterface;
    use Symfony\Component\HttpFoundation\File\MimeType\MimeTypeGuesserInterface;
    use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
   -use Drupal\Core\Config\ConfigFactoryInterface;
   -use Drupal\Core\Extension\ModuleHandlerInterface;
   -use Drupal\Core\Form\ConfigFormBase;
   -use Drupal\Core\Theme\ThemeManagerInterface;
   
   +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -1,12 +1,8 @@
   -/**
   - * @file
   - * Contains \Drupal\Tests\Core\Theme\RegistryTest.
   - */
   -
   

   This is a large change and some of this is not related or necessary for the change. Makes reviewing harder.

4. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -819,16 +841,4 @@ public function getPrefixGroupedUserFunctions($prefixes = []) {
   -  /**
   -   * Wraps drupal_get_path().
   -   *
   -   * @param string $module
   -   *   The name of the item for which the path is requested.
   -   *
   -   * @return string
   -   */
   -  protected function getPath($module) {
   -    return drupal_get_path('module', $module);
   -  }
   

   This should be deprecated - no?

5. +++ b/core/modules/action/action.install
   @@ -0,0 +1,13 @@
   +<?php
   +
   +/**
   + * @file
   + * Install, update and uninstall functions for the action module.
   + */
   +
   +/**
   + * Removes the action.settings configuration.
   + */
   +function action_update_8001() {
   +  \Drupal::configFactory()->getEditable('action.settings')->delete();
   +}
   

   Not sure this should be here.

6. +++ b/core/modules/update/src/Form/UpdateManagerUpdate.php
   @@ -38,10 +46,13 @@ class UpdateManagerUpdate extends FormBase {
   +   * @param \Drupal\Core\Extension\ModuleExtensionList $module_list
   +   *   The module list service.
       */
   -  public function __construct(ModuleHandlerInterface $module_handler, StateInterface $state) {
   +  public function __construct(ModuleHandlerInterface $module_handler, StateInterface $state, ModuleExtensionList $module_list) {
        $this->moduleHandler = $module_handler;
        $this->state = $state;
   +    $this->moduleList = $module_list;
      }
    
   

   Don't we need to dance the deprecation dance here?

7. +++ b/core/tests/Drupal/KernelTests/Core/Bootstrap/BootstrapDeprecationTest.php
   @@ -0,0 +1,35 @@
   +    $this->assertNotEmpty(drupal_get_filename('module', 'system'));
   ...
   +    $this->assertNotEmpty(drupal_get_path('module', 'system'));
   

   Can we not assert against the return value. Won't this be core/modules/system

8. +++ b/core/tests/Drupal/KernelTests/Core/Bootstrap/ExtensionListTest.php
   @@ -0,0 +1,106 @@
   + * Tests that drupal_get_filename() works correctly.
   

   This isn't testing drupal_get_filename()

Needed a re-roll after #2940189: Deprecate system_get_info().

Re #150

1. You're welcome!
2. Fixed
3. I reverted this and created a follow up #3086397: Registry does not declare a themeInitialization property. I realise this is a pain, but as we're modifying the list of imports, its a good opportunity to clean it up and sort it alphabetically. Happy to revert and file a follow up.
4. Fixed. Made it deprecated.
5. Not sure where this should go?
6. Did the deprecation dance 💃
7. Fixed
8. Fixed

Re: #150.5 I think this was accidentally included in a bad reroll. Removed.

+++ b/core/tests/Drupal/KernelTests/Core/Bootstrap/ExtensionListTest.php
@@ -0,0 +1,106 @@
+<?php
+
+namespace Drupal\KernelTests\Core\Bootstrap;
+
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\Core\Extension\Exception\UnknownExtensionException;
+use Drupal\KernelTests\KernelTestBase;
+
+/**
+ * Tests for ExtensionList and sub-classes.
+ *
+ * @group Bootstrap
+ */
+class ExtensionListTest extends KernelTestBase {
...
+  /**
+   * Tests that drupal_get_filename() works when the file is not in database.
+   */
+  public function testDrupalGetFilename() {
...
+  /**
+   * Tests missing extensions.
+   */
+  public function testMissingExtensions() {

Discussed with @berdir. The first test is no longer testing anything without a database. And we already have good unit test coverage of both ExtensionList and ExtensionDiscovery.

The second test is also covered by the existing unit test coverage.

Therefore let's not add this test here but still completely remove GetFilenameTest.

Therefore let's not add this test here but still completely remove GetFilenameTest.

I took this to mean remove the entire test class.

Yes, exactly that. I always feel bad when I RTBC something and miss things like #150.5 and some of the others, hopefully this one will be better :)

Updated the title and CR to mention we are also deprecating drupal_get_filename().

Ubernit: found an extra character in getPathnØame() in this comment.

+++ b/core/includes/bootstrap.inc
@@ -256,8 +256,18 @@ function config_get_config_directory($type) {
+ * @see \Drupal\Core\Extension\ExtensionList::getPathnØame()

This is great work here folks, and really highlights some hidden dependencies we were masking with procedural code. Below is a few actual things that might need fixing, and a lot of random ramblings about the coupling.

1. +++ b/core/includes/bootstrap.inc
   @@ -256,8 +256,18 @@ function config_get_config_directory($type) {
   + * @see \Drupal\Core\Extension\ExtensionList::getPathnØame()
   

   stray character here that can be fixed on commit

2. +++ b/core/lib/Drupal/Core/Config/ExtensionInstallStorage.php
   @@ -104,10 +104,12 @@ protected function getAllFolders() {
   +          /** @var \Drupal\Core\Extension\ProfileExtensionList $profile_extension_list */
   +          $profile_extension_list = \Drupal::service('extension.list.profile');
   +          $profile_extension_list->setPathname($this->installProfile, $profile_list[$this->installProfile]->getPathname());
   

   ick, circular dependency? realise it was already there, but just hidden

3. +++ b/core/lib/Drupal/Core/Config/InstallStorage.php
   @@ -162,7 +162,9 @@ protected function getAllFolders() {
   +          /** @var \Drupal\Core\Extension\ProfileExtensionList $profile_extension_list */
   +          $profile_extension_list = \Drupal::service('extension.list.profile');
   +          $profile_extension_list->setPathname($profile, $profile_list[$profile]->getPathname());
   

   and here too 😿

4. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -229,7 +229,7 @@ public function scan($type, $include_tests = NULL) {
      public function setProfileDirectoriesFromSettings() {
   ...
        if ($profile = \Drupal::installProfile()) {
   -      $this->profileDirectories[] = drupal_get_path('profile', $profile);
   +      $this->profileDirectories[] = \Drupal::service('extension.list.profile')->getPath($profile);
   

   It would be good to get a follow up to try and untangle this a bit (and the above).

   install storage calls scan on profiles, which calls setProfileDirectoriesFromSettings which gets the profile directories, but then install storage calls set profile? its super-coupled and the static access on both of them indicates something is out right?

   Something where we can map out a plan of how we fix this interdependency

5. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
   @@ -531,7 +531,7 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
   -    $service_yaml_file = drupal_get_path('module', $module) . "/$module.services.yml";
   +    $service_yaml_file = \Drupal::service('extension.list.module')->getPath($module) . "/$module.services.yml";
   

   wow this really highlights all the places we were kidding ourselves about having modern code 😂

   the module installer and the extension list are tightly coupled aren't they (sorry, nothing useful in my review so far, I might be getting delirious)

6. +++ b/core/modules/ckeditor/src/Plugin/CKEditorPlugin/DrupalImageCaption.php
   @@ -46,7 +46,7 @@ public function getLibraries(Editor $editor) {
   +    return \Drupal::service('extension.list.module')->getPath('ckeditor') . '/js/plugins/drupalimagecaption/plugin.js';
   
   @@ -70,7 +70,7 @@ public function getConfig(Editor $editor) {
   +      \Drupal::service('extension.list.module')->getPath('ckeditor') . '/css/plugins/drupalimagecaption/ckeditor.drupalimagecaption.css',
   

   any reason these don't use ->getModulePath like the ones above and below?

7. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -1108,4 +1108,36 @@ public static function assertEquals($expected, $actual, $message = '', $delta =
   +  protected function getModulePath($module_name) {
   ...
   +  protected function getThemePath($theme_name) {
   
   +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
   @@ -753,4 +753,36 @@ protected function translatePostValues(array $values) {
   +  protected function getModulePath($module_name) {
   ...
   +  protected function getThemePath($theme_name) {
   

   could these be in a trait so a) we can share the code with these two base classes but b) DTT can also use it?

Re: #159

1. Fixed

I left 2-5 for @Berdir to respond to.

6. Created follow up #3087327: [PP-1] DrupalImageCaption should extend CKEditorPluginBase.
7. Moved to a trait.

Also fixed method name in deprecation message.

Epic work!

1. +++ b/core/modules/ckeditor/src/CKEditorPluginBase.php
   @@ -30,6 +30,44 @@
    abstract class CKEditorPluginBase extends PluginBase implements CKEditorPluginInterface, CKEditorPluginButtonsInterface {
    
   +  /**
   +   * The module list service.
   +   *
   +   * @var \Drupal\Core\Extension\ModuleExtensionList
   +   */
   +  protected $moduleList;
   +
   +  /**
   

   🤔 Shouldn't this get a constructor with an optional argument, with the optionality being deprecated in 9.0.0?

   ✅ Hm … upon further investigation, it looks like we almost never do this in plugin base classes.

2. +++ b/core/modules/ckeditor/src/CKEditorPluginBase.php
   @@ -30,6 +30,44 @@
   +   * Gets the drupal-root relative installation directory of a module.
   

   🤓 Übernit: "drupal" → "Drupal"

3. +++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
   @@ -74,13 +82,20 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
   +      @trigger_error('Calling CKEditor::__construct() without the $module_list argument is deprecated in drupal:8.0.0 and is required in drupal:9.0.0. See https://www.drupal.org/node/2940438', E_USER_DEPRECATED);
   

   🤓 "8.0.0" → "8.8.0"

4. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionPathResolverTest.php
   @@ -0,0 +1,64 @@
   + * Tests the extension list factory.
   

   🤓 "list factory" → "path resolver". Nope, \Drupal\Core\Extension\ExtensionPathResolver is documented as Factory for getting extension lists by type. so leaving unchanged. Oddly named though.

5. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionPathResolverTest.php
   @@ -0,0 +1,64 @@
   +   * @covers ::getPathname
   ...
   +  public function testGetPath() {
   ...
   +   * @covers ::getPath
   ...
   +  public function testGetDir() {
   

   🤓 Mismatches.

6. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ExtensionPathResolverTest.php
   @@ -0,0 +1,64 @@
   +    $path_resolver->getPathname('foo', 'bar');
   ...
   +    $path_resolver->getPathname('foo', 'bar');
   

   👎 The first one is correct, the second one should be calling ::getPath().

7. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -565,18 +628,8 @@ public function providerTestCssAssert() {
   -  protected function drupalGetPath($type, $name) {
   
   +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -486,22 +495,12 @@ public function providerTestPostProcessExtension() {
   -class TestRegistry extends Registry {
   -
   -  protected function getPath($module) {
   -    if ($module == 'theme_test') {
   -      return 'core/modules/system/tests/modules/theme_test';
   -    }
   -  }
   -
   -}
   

   🥳

8. +++ b/core/tests/Drupal/Tests/ExtensionListTestTrait.php
   @@ -0,0 +1,42 @@
   +   *   The drupal-root relative path to the module directory.
   ...
   +   *   The drupal-root relative path to the theme directory.
   

   🤓 "drupal" → "Drupal"

Fixed all those nits, and back to RTBC ah no, I can't, @Berdir still needs to respond to point 2–5. Darn!