Add Module, Theme, Profile, and Extension value objects

Created on 20 June 2013, over 11 years ago
Updated 12 July 2023, over 1 year ago

Parent issue

📌 [META] Improve the extension system (modules, profiles, themes) Active

Problem/Motivation

The DX of ModuleHandler or any other place that reads or works with module/theme/profile data is horrible. We need some kind of container through which we can easily access information about extensions, namely themes, modules and possibly profiles too.

Proposed resolution

Introduce classed objects for modules, themes and profiles to increase DX and maintainability of our code. We should be able to create a shared ExtensionInterface which would expect basic methods like ->getVersion(), ->getDescription(), etc.

I believe that this would make module/theme-related code much more readable and less error prone. Also, we would get autocompletion in our IDEs for reading the relevant .info.yml properties.

Detailed concept TBD.

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Extension 

Last updated 3 days ago

No maintainer
Created by

🇦🇹Austria fubhy

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 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.

Production build 0.71.5 2024