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:
Steps to reproduce
Proposed resolution
- Add
Drupal\Core\Extension\ExtensionTypeInterfacewith public constants for MODULE, THEME, etc. - Use ExtensionTypeInterface::MODULE in code instead of hard-coding
'module'. Same for THEME, etc.
Remaining tasks
- Decide on how to best carve up the scope of the remaining porting.
- Open appropriate follow-ups.
- Final reviews / refinements.
- RTBC.
- Commit.
- 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
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 3350946-23.do-not-test.patch | 346.98 KB | dww |
Issue fork drupal-3350946
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
Comment #2
e0ipsoComment #3
andypostThere's more extensions possible
Comment #4
andypostComment #5
larowlanComment #6
andypostUsing 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
Comment #7
longwaveA 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?
Comment #8
andypostRe #7 - nice idea! but I bet we can't have more then one engine enabled same time
Comment #9
e0ipsoComment #10
e0ipsoI 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->valuein those cases. I am not sure how ergonomic that is. Perhaps we should create anExtensionTypeInterfacewith public constants instead?Comment #11
larowlanYeah feels nicer than ->value
Comment #13
e0ipsoThe MR implements a new interface that contains public constants, as suggested in #10 and agreed on #11.
Comment #14
larowlanLooks good, there's a couple of place in DrupalKernel and ConfigImporter we should update too in my book
Comment #15
e0ipsoI 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?Comment #17
dwwWearing 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_typeused internally that handles 'module' and 'theme' (for which this constant is nice), but it also has to deal with these options: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.:
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...
Comment #18
larowlanI 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
Comment #19
dwwMore accurate title now that we've moved on from a PHP enum for this.
Comment #20
smustgrave commentedCan 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?
Comment #21
dwwYeah, 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:
Does that seem like a somewhat reasonable scope for the follow-ups?
Comment #22
dwwThere'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.
Comment #23
dwwI basically did this:
It hit a bunch of docs we probably don't want to change. It didn't insert the necessary
usestatements 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? 😉Comment #24
dwwBased 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.
Comment #25
geek-merlinRE #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:
A big point for enums imho.
Comment #27
nicxvan commentedDo we still want to do this?