Add Module, Theme, Profile, and Extension value objects

tagging

how about sth like that? i ll start adding some usage in core in a bit

lets

hmm, two fixes..

This problem looks ready-made for Symfony's ParameterBag. Not sure if it's verboten in D8.

But here's a patch anyway.

I kept all the interfaces, but just made Module, Profile, and Theme into subclasses of ExtensionBase, itself a subclass of ParameterBag. This makes it (almost) trivially easy to manage the parameter arrays.

With tests.

+use Symfony\Component\HttpFoundation\ParameterBag;

This feels wrong, just saying.

There is no profile interface, any reason for that?

Additional the constructors on the different Extensions needs docs.

The namespace in the test and the Contain statement disagree.

The use statements in the test is not needed.

ArrayCollection looks better, with iterators and map() and filter().

RE: ProfileInterface... I think I left it out because I wasn't sure how it was currently implemented, and how different it would be from a module. Plus it's not in the issue title.

If the whole point of this exercise is to make an abstract container class and special-case containers for modules, profiles, and themes so some manager class can deal with them, then yes. That's what we'll do, with ArrayCollections.

Also, just linking to what you've obviously seen: #2024083: [META] Improve the extension system (modules, profiles, themes)

doh. :-)

OK, here we go. Full hierarchy of interfaces, and built with ArrayCollection, and probably too many tests. :-)

There's one question though... Maybe arrays of dependencies should be other ExtensionInterface objects, or maybe not. I'm not quite sure how this is going to be generated or consumed, so for now it assumes that dependencies will be arrays of strings, as you'd get when you parse a YAML .info file.

I really really like this issue. I wanted to do the exact same thing after working on #2021959: Refactor module handling responsibilities out of DrupalKernel.

This ModuleHandlerFactory issue isn't really dependent on this though, so committing it could possibly wait until we have usage, but it certainly wouldnt hurt to do this preemptively if it makes that patch easier.

+++ b/core/lib/Drupal/Core/Extension/ExtensionBase.php
@@ -0,0 +1,47 @@
+use Doctrine\Common\Collections\ArrayCollection;
...
+abstract class ExtensionBase extends ArrayCollection implements ExtensionInterface {

This basically bounds our module system to doctrine ... are we sure we want to go into that direction?

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionInterface.php
   @@ -0,0 +1,51 @@
   +   * extensions's .info.yml file and does not represent the entire dependency
   

   docs nit: "extension's", not "extensions's".

2. +++ b/core/lib/Drupal/Core/Extension/ThemeInterface.php
   @@ -0,0 +1,79 @@
   +  public function getScreenshot();
   

   i would have guessed this gets the image data of the screenshot itself, but it seems it actually gets the path TO the screenshot. right?

This basically bounds our module system to doctrine ... are we sure we want to go into that direction?

i also think this is fine; we depend on libraries, that's why we bring them in. plus, ArrayCollection is really a pretty generic datastructure, so it's no big thing.

what is a little awkward here, though, is that these objects are straddling data that is read-only in a php context (information extracted from info yamls) and rw data (e.g., installation/active state). it's not that big of a deal, but it's worth noting.

Good point. There will be a time when installation and active state info will be unknown.

The installation status is also read-only. The only way to change it will be to use ModuleInstaller which will cause a recompilation of the container and only subsequently change that data. We are looking at a pure, read-only object (which should or should not be enforced, I don't know...).

yeah, that's a good point - the source may be different, but the data should be equally inviolate. OK so, the requirements really do dictate that the object should be read-only, which suggests state is set and frozen in the constructor. in that case, ArrayCollection is not necessarily the best thing to subclass from; we need the subset of methods that are read-only.

unfortunately, that raises the bar on what objects need to be present in order to correctly construct one of these. kinda a weird chicken or egg thing, since ModuleHandler is supposed to be the canonical point of record on what modules are enabled...right?

ArrayCollection should just be removed for now. I like the idea of it, but it's clearly a distraction here.

So: What should ExtensionInterface encapsulate?

If we truly need to support contrib-added parameters to .info.yml, then yeah, a get() method seems appropriate. I'd just make sure we need it for each extension type before adding it. For example, if it's just a theme thing, let's only do it there.

Updated issue summary.

Just a rerole + one usecase. Should this patch also patch all places like the entity info one?

30: extension-2024043.patch queued for re-testing.

Back to green.

Plain reroll

At that point in time we kinda dislike interfaces for those kind of value objects. Ideally we would rather have just the value objects to indicate that those objects.

#39 very much in favour of that. Value objects are exactly what we want here. We don't want alternate implementation of Modules in contrib or custom. The extension system is complicated enough as it is. #3015812: Introduce new Theme extension object and properly deprecate REGIONS_VISIBLE and REGIONS_ALL begins to implement this for themes.

Agreed

Re-roll, removal of interfaces, making classes final.
Looks ExtensionBase no longer needed because there's Extension object which has mostly all methods.
Diggin extension discovery...

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionInterface.php
   @@ -0,0 +1,46 @@
   +interface ExtensionInterface {
   

   I thought comments above indicated we were dropping the interface?

2. +++ b/core/lib/Drupal/Core/Extension/Module.php
   @@ -0,0 +1,22 @@
   +final class Module extends ExtensionBase {
   
   +++ b/core/lib/Drupal/Core/Extension/Profile.php
   @@ -0,0 +1,22 @@
   +final class Profile extends ExtensionBase {
   

   Module also has requires/required_by and sort values - see \Drupal\Core\Extension\ModuleHandler::buildModuleDependencies

Perhaps we should pursue #2972143: Create \Drupal\Component\Utility\ArrayObject instead of using the doctrine array collection?

Fixes for #52. Also some linting issues.

Re: #53 do we really want our own implementation for this? Seems super low-level.

Just spent some time working around theses questions without finding this issue initially.

As mentioned in #52, module Extension instances carry these undeclared properties:

• info
• required
• required_by
• sort

This issue avoids declaring them on Extension because theme and theme_engine extensions don't have them, but now that [#2937955] is in, the required[_by] fields can also exist on themes, and be empty on theme_engines, while profiles are modules in everything but name and load order.

AIUI the proposed patch changes the API, so it needs to be moved beyond 9.1, but could we just start by adding these four fields on the existing Extension class, as this doesn't break any API and adds a minimum of documentation and discoverability to these fields ?

1. +++ b/core/lib/Drupal/Core/Extension/ExtensionBase.php
   @@ -0,0 +1,58 @@
   + * Things which are extensions: Modules, themes, and profiles.
   

   now database drivers are extensions

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionBase.php
   @@ -0,0 +1,58 @@
   +    return $this->get('name');
   ...
   +    return $this->get('version');
   ...
   +    return $this->get('description');
   ...
   +    return $this->get('dependencies') ?: [];
   

   it will notice on PHP 8.2

Running test on PHP 8.2 brings lots of deprecations because properties are missing

  1448x: Creation of dynamic property Drupal\Core\Extension\Extension::$subpath is deprecated
    724x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    724x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  1448x: Creation of dynamic property Drupal\Core\Extension\Extension::$origin is deprecated
    724x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    724x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  1182x: Creation of dynamic property Drupal\Core\Extension\Extension::$info is deprecated
    591x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    591x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  1150x: Creation of dynamic property Drupal\Core\Extension\Extension::$status is deprecated
    575x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    575x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  1150x: Creation of dynamic property Drupal\Core\Extension\Extension::$required_by is deprecated
    575x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    575x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  1150x: Creation of dynamic property Drupal\Core\Extension\Extension::$requires is deprecated
    575x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    575x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  1150x: Creation of dynamic property Drupal\Core\Extension\Extension::$sort is deprecated
    575x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    575x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  994x: Creation of dynamic property Drupal\Core\Extension\Extension::$weight is deprecated
    497x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    497x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

  994x: Creation of dynamic property Drupal\Core\Extension\Extension::$schema_version is deprecated
    497x in TrackerNodeAccessTest::testTrackerNodeAccessIndexing from Drupal\Tests\tracker\Functional
    497x in TrackerNodeAccessTest::testTrackerNodeAccess from Drupal\Tests\tracker\Functional

As per #57 I propose a new issue to create those methods on the extension class first, and postpone this on that issue.

Update: I think we should continue working on this issue.

Created #3292759: Create getters and setters for dynamic Extension properties

I've combined this with our own ArrayObject implementation in #2972143: Create \Drupal\Component\Utility\ArrayObject and added fixes for #62

Re-testing #66, because I cannot reproduce those failures.

Patch #66 applied cleanly for version 10.0.x.

Queued last patch for PHP 8.2-rc5 & MySQL 8

and here's a patch test the sate

I assume these are random test fails in HEAD, as these are new objects and not getting used anywhere yet.

They are not random - 2794 failed https://www.drupal.org/pift-ci-job/2510630

re-queued patch #66 again

Ah yes, the php 8.2 fails are real.

1. +++ b/core/lib/Drupal/Component/Utility/ArrayObject.php
   @@ -0,0 +1,100 @@
   + * Provides a utility object for arrays.
   ...
   +   * Creates a new ArrayObject from the array.
   

   Having the same name as SPL class is confusing and DX-- on reading code, please rename

   Ref https://www.php.net/manual/en/class.arrayobject.php

2. +++ b/core/lib/Drupal/Component/Utility/ArrayObject.php
   @@ -0,0 +1,100 @@
   +class ArrayObject implements \Countable, \IteratorAggregate, \ArrayAccess {
   
   +++ b/core/tests/Drupal/Tests/Component/Utility/ArrayObjectTest.php
   @@ -0,0 +1,83 @@
   + * @coversDefaultClass \Drupal\Component\Utility\ArrayObject
   + * @group Utility
   ...
   +class ArrayObjectTest extends TestCase {
   
   +++ b/core/tests/Drupal/Tests/Component/Utility/DummyArrayObject.php
   @@ -0,0 +1,22 @@
   +class DummyArrayObject extends ArrayObject {
   

   I think it needs to be split because nearly the same is happening in #3275858: View's ResultRow uses deprecated dynamic properties which is much simple then new extension value objects for extensions

   Moreover it could be applied to plugins - views has a lot dynamic properties so would be great to reuse same approach

3. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,107 @@
   +  public function getStylesheets() {
   +    return $this->get('stylesheets') ?: [];
   ...
   +  public function getScripts() {
   +    return $this->get('scripts') ?: [];
   

   wondered why there's no libraries

4. +++ b/core/lib/Drupal/Core/Extension/Theme.php
   @@ -0,0 +1,107 @@
   +  public function getFeatures() {
   +    return $this->get('regions') ?: [];
   

   copy/paste

Just to be clear, these value objects aren't meant to replace \Drupal\Core\Extension\Extension. That class is much more to do with loading extensions with ExtensionDiscovery. It might make sense to rename it to ExtensionFile (or similar)?

These value objects are for 'Extension Info' which is currently just an array that gets passed around for modules, themes, engines, and profiles.

I don't think we need our own ArrayObject after all. We can just use SPL \ArrayObject for the abstract base class.

Also since we are representing extension info, I renamed everything to include 'Info', eg. \Drupal\Core\Extension\ThemeInfo

I provided a usage example for ThemeInfo for feedback. I didn't include an interdiff since it was pretty basic and I re-wrote most of it.

Fix spelling error.

Something is slightly broken Undefined array key "dependencies" and others all over https://dispatcher.drupalci.org/job/drupal_patches/154004/

Yeah, this is because we removed the "dependencies" array key in favour of using a property and getDependencies().

However, we need to have a BC layer, so I guess that means supporting both but triggering a deprecation error if array access is used. We can use Drupal\Component\Utility\DeprecatedArray for this.

When I do, I get hundreds of instances where this is getting called. Not sure how to break this down into more bite-sized chunks.

I think we can use DeprecatedArray and silence deprecations as contrib also can have tons of usages

Added patch against #77 in 10.1.x

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.

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.

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.