- last update
over 1 year ago Custom Commands Failed - 🇩🇪Germany donquixote
I don't like the mixing of array access and regular object methods.
If we need to support custom properties, I would rather want to see$info->get($key)
than$info[$key]
, if$info
is not an array.Is there any good reason for it?
BC support where we previously used arrays?
It would not really work because we already break BC by not providing the basic keys like version etc.I think for issues like this we should have two PRs or two branches:
- One with the actual implementation we want to be merged in this issue.
- One "proof of concept" that shows how this would be used to improve DX elsewhere in the codebase. This could be done in a separate issue (subtask), if we want to keep the main issue clean.
In the end we only merge the first MR, and split out the work from the second branch into follow-up issues.
- 🇩🇪Germany donquixote
Also, the issue summary and title are out of date.
The original issue asked to introduce extension objects.
Now we have them.So the remaining problem seems to be that we are not happy with the current Extension class.
Which is fine, but it seems this should be addressed in a new issue where we clearly summarize what is wrong with the current Extension class, and what are the main use cases where we want to improve DX.Or at least update the summary of this one.
----
On the topic:
I have the impression that different functions/methods/classes actually need different types of extension objects:
- Some just need the machine name of each module.
- Others need machine name and label.
- Others need machine name and path, to look for yml files.
- Others need various info from the *.info.yml file.
- Some need only enabled modules, others need all available modules, possibly with a status for enabled/disabled.
- Some need raw data from a filesystem scan and from the *.info.yml files, others want processed data that already ran through hook_system_info_alter().
At different points in the application and the bootstrap process we actually have different parts of this info available.
And depending where and when you get your Extension object from, some of its properties might be filled at that time, or not.
E.g.:
- ModuleHandler->getModuleList() gives us a list of Extension objects, but these don't have any of the special properties filled.
- ExtensionDiscovery->scan() gives Extension objects with no info filled in, but later these same objects get updated in ModuleExtensionList. This "pollutes" the static cache in ExtensionDiscovery::$files. Yes, at some point, ExtensionDiscovery::$files[...]['core'][0]['module']['.../system.info.yml']->info will be filled in.
- ModuleExtensionList->getList() gives us distinct Extension objects with don't have their info filled in, and never will - afaik.
This maybe works ok for now, but it makes for messy dependency chain, and prevents proper refactoring.
It would be cleaner to use different classes, or at least diference instances, in different places of the application.
These classes can be immutable, or at least be treated as such most of the time, and only contain the properties that are needed and available in the context in which they are used and provided:- One DiscoveredExtension class returned from and cached by ExtensionDiscovery. This does not allow dynamic properties.
- The "feature-complete" AvailableExtension class to be returned from ExtensionList, which includes all the info. This could have subclasses for theme, module, profile etc.
- Another InstalledExtension class (named differently I guess) to be returned from ModuleHandler->getModuleList(), without the extra info. For my taste, this also should not contain the app root and the ->load() method.
This would allow to refactor the respective classes, and make a clean chain of dependencies, where the dependent layers don't "pollute" the objects that they obtain from a source.
There are different ways to do this in a BC-friendly way. E.g. leave the ModuleHandlerInterface unchanged, let it return the current Extension objects, but provide an alternative that returns the "clean" Extension objects.
This is all still quite rough and might be difficult to do.
But if we are already considering a redesign of this, we should do it in a clean way. - 🇩🇪Germany donquixote
Re #84 (myself)
The original issue asked to introduce extension objects.
Now we have them.So I think the idea is:
Currently we have one Extension class for everything.
Instead, we want dedicated Extension classes for different types of extension.I'd say this makes sense, and it is fine to keep the existing issue for this.
So
should be addressed in a new issue
Not needed :)
Still, re #85 (myself):
If you ask me, we should only use these new extension classes in the places that really need them.
So yes for ExtensionList, but:
- Not in ExtensionDiscovery.
- Not in ModuleHandler.Both of these can use more slim objects, or perhaps just plain arrays.
Or ModuleHandler can use the old Extension class for BC.