Problem/Motivation

Let's introduce some public constants for extension types so we can reference them in our code instead of hard-coding 'theme', 'module', 'profile', 'theme_engine'.

Additional references:

  1. https://git.drupalcode.org/project/drupal/-/merge_requests/3432/diffs#no...

Steps to reproduce

Proposed resolution

  • Add Drupal\Core\Extension\ExtensionTypeInterface with public constants for MODULE, THEME, etc.
  • Use ExtensionTypeInterface::MODULE in code instead of hard-coding 'module'. Same for THEME, etc.

Remaining tasks

  1. Decide on how to best carve up the scope of the remaining porting.
  2. Open appropriate follow-ups.
  3. Final reviews / refinements.
  4. RTBC.
  5. Commit.
  6. Unblock #3352546: [pp-1] Port SDC to use ExtensionTypeInterface

User interface changes

None.

API changes

Addition of Drupal\Core\Extension\ExtensionTypeInterface with public constants instead of having to hard-code strings.

Data model changes

None.

Release notes snippet

N/A

Issue fork drupal-3350946

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

e0ipso created an issue. See original summary.

e0ipso’s picture

Issue summary: View changes
andypost’s picture

There's more extensions possible

$ find . -name '*.engine' -print
./core/themes/engines/twig/twig.engine
./core/modules/system/tests/themes/engines/nyan_cat/nyan_cat.engine
./core/modules/system/tests/fixtures/HtaccessTest/access_test.engine
larowlan’s picture

Category: Feature request » Task
andypost’s picture

Using PHP 8.1 feature https://www.php.net/manual/en/language.enumerations.backed.php this enums can give file finder pattern to extension.list claases

longwave’s picture

A sort of related thought I've had for a while: why are theme engines a separate extension type? Can't they just be modules that provide a theme engine service? Worth opening a separate issue to discuss this?

andypost’s picture

Re #7 - nice idea! but I bet we can't have more then one engine enabled same time

e0ipso’s picture

Issue summary: View changes
e0ipso’s picture

I have been looking at replacing usages of 'module', ... with ExtensionType::Module, using backed enumerations. They are not a drop-in replacement, like a constant would be, they are causing errors when used in string context (like array keys).

One potential solution is to use ExtensionType::Module->value in those cases. I am not sure how ergonomic that is. Perhaps we should create an ExtensionTypeInterface with public constants instead?

larowlan’s picture

One potential solution is to use ExtensionType::Module->value in those cases. I am not sure how ergonomic that is. Perhaps we should create an ExtensionTypeInterface with public constants instead?

Yeah feels nicer than ->value

e0ipso’s picture

Status: Active » Needs review

The MR implements a new interface that contains public constants, as suggested in #10 and agreed on #11.

larowlan’s picture

Looks good, there's a couple of place in DrupalKernel and ConfigImporter we should update too in my book

e0ipso’s picture

I replaced strings by constants in the commit above for DrupalKernel and ConfigImporter.

I noticed that the core folder has +5k uses of 'module' alone. Is the expectation to replace them all with the newly introduced constants? Or are we good with the current patch?

grep --recursive "'module'" ./web/core |wc --lines

5810

dww made their first commit to this issue’s fork.

dww’s picture

Wearing my Update Manager maintainer hat, I just tried to port that core module to use this. Mostly pretty simple. However, Update Manager has a project_type used internally that handles 'module' and 'theme' (for which this constant is nice), but it also has to deal with these options:

     'module-disabled' => t('Uninstalled modules'),
     'theme-disabled' => t('Uninstalled themes'),

Those obviously don't make sense for this new enum/constant. So we're left with code that has to do a mix of the const + hard coded strings, anyway, e.g.:

  $project_types = [
    'core' => t('Drupal core'),
    ExtensionTypeInterface::MODULE => t('Modules'),
    ExtensionTypeInterface::THEME => t('Themes'),
    'module-disabled' => t('Uninstalled modules'),
    'theme-disabled' => t('Uninstalled themes'),
  ];

Not the end of the world, but not totally beautiful, either. 😅

Maybe we don't want to bother, and we should revert the commit I just pushed. But it's a start at cutting into the remaining 5810 instances of 'module', in a core module that really cares about extension types...

larowlan’s picture

I think we should aim to convert things that deal with extension types, so config importer and the kernel make sense, as does update module.

But no, I don't think we need to convert all 5k of them

dww’s picture

Title: Introduce PHP enum for extension types » Introduce ExtensionTypeInterface with public constants for extension types

More accurate title now that we've moved on from a PHP enum for this.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Can it be documented the scope of this? As mentioned there are tons of instances of 'module', 'theme', and 'profile'

Should instances like

$config_importer->getExtensionChangelist('module', 'install');

be updated to use the constants?

There are enough instances think it would be good idea of limit to specific sections of the repo at first and maybe open follow ups for other sections?

dww’s picture

Yeah, that's a legitimate request. 😅

Good news from greplandia. Although we've got ~5500 remaining hits for 'module', 4794 of those are from fixtures (mostly for migrate tests), and we're definitely not going to change those. So there's really only ~730 real hits left to consider the scope of, in ~290 unique files. If you filter out tests (or at least filenames that have 'test' in them), we're down to only ~120 files to consider.

Very quick attempt to analyze the results. Some big buckets:

  1. Extension list scanning
  2. Adding dependencies
  3. Tests
  4. Doc comments
  5. Other

Does that seem like a somewhat reasonable scope for the follow-ups?

dww’s picture

There's really not *that* many places that are using 'module' or 'theme' for extension list scanning. Maybe better to port those here as part of this initial issue. Pushed a few more commits for that. If I have more time today I'll keep going.

dww’s picture

StatusFileSize
new346.98 KB

I basically did this:

for i in `cat 3350946-21.module-string-find-no-fixture-files-only.txt`
 perl -pi.bak -e "s/'module'/ExtensionTypeInterface::MODULE/g" $i

It hit a bunch of docs we probably don't want to change. It didn't insert the necessary use statements to the changed files, etc. This isn't going to work at all. 😅 But I'm posting here as a do-not-test patch (instead of pushing anything to the MR) if folks wanna see what happens if we basically do the proposed change to all of core.

IMHO, this isn't an obvious win in some subsystems, which is making me wonder if this is worth doing at all. Is 'module' vs. 'theme' in our code really a "problem" we need to solve? 😉

dww’s picture

Issue summary: View changes

Based on Slack between @larowlan, @mherchel, @e0ipso and myself, we agreed this shouldn't block SDC. I opened #3352546: [pp-1] Port SDC to use ExtensionTypeInterface as a child issue of this, so we can unblock SDC while we sort out the scope here. See #3340712-123: Add Single Directory Components as a new experimental module for more.

I suspect we're going to need a different Interface for things like config dependency types, etc, so there will probably be other child issues here.

geek-merlin’s picture

RE #10 Enum vs Interface::Constants (sorry for being late to the parts):
Apart from the syntax for string constants, the decision enums vs interface constants has another consequence:

- If we create interface constants, then extension type will always be a string. No leveraging of type safety and static analysis.
- If we create enums, we CAN not only replace string typed values with the new constants (by using 'ExtensionType::Module->value'), but ALSO leverage type safety and static analysis for NEW APIs:

  public function getInstances(ExtensionType $extensionType) {...}
  ...
  $instances = $service->getInstance(ExtensionType::Module);

A big point for enums imho.

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.

nicxvan’s picture

Do we still want to do this?

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.