Replacement for ExtensionDiscovery, following SOLID principles

I experimented, and came up with an architecture where a lot of things can be swapped out for testing, and individual components can be tested individually. Here is what such a test can look like:
https://github.com/donquixote/drupal/blob/8.2.x-extension-libraries-2016...
This is far from perfect, and the git history is messy. Don't waste too much time looking at it.
But it does what it is designed for. And even runs measurably faster for me (10ms instead of 13ms).
It is not meant to be used as-is, but as a quarry to take parts from.
Some of it is redundant, providing more than one solution to a problem. Again this is to compare and then decide what's best.

Lower discovery layer:
The result of this layer is a grouped list of extensions or file paths per searchdir, extension type, subdir name and extension name.
SearchdirToFilesGroupedInterface
SearchdirToRawExtensionsGroupedInterface

• Directory scanning happens with opendir() + readdir(), as it used to be in D7. The iterator makes everything more complex, and does not really add any value. And things are faster now.
• We scan per subdirectory, e.g. 'core/modules', not per search directory, e.g. 'core'. This way we don't need a "whitelist" in the scanning tool.
• Search directories are specified as "searchdir prefix", e.g. 'core/'. This way we don't have a special case for the root search directory, ''.
• The part with the static cache is split out as a component with a buffer wrapper. See https://github.com/donquixote/drupal/blob/8.2.x-extension-libraries-2016...
  Note that this caches one instance per root and "include_tests" setting.
  This is not really a singleton in the classical sense. Identical instances can be created independently for testing. The only reason for the static access is so that components not using the container can still benefit from the static cache.
  This way, this part is available to other components, and can be tested separately.
• An alternative to this exists, where we cache file paths instead of objects, to avoid modifications leaking back into the cache.
  SearchdirToFilesGrouped*
  This in fact does have a test: SearchdirToFilesGroupedTest.
• The actual scanning of a directory is also split out, so it can be separately tested, and swapped out for a fake scan during testing of components that depend on it.
  An alternative to this would be vfsStream, as in #2605654-48: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing. But maybe it is still nice to have this stuff separate.
• The logic to determine the type from an info yml file is swappable to make this more independent from the filesystem.
  This may no longer be needed if we do all the testing with vfsStream.

It is possible to just take this part, and use it to replace the scanning and static cache in ExtensionDiscovery.
https://github.com/donquixote/drupal/commit/e7694f3781d9a694b41720bfb566...
Note that this is following a series of smaller fixes on ExtensionDiscovery
We can, however, leave ExtensionDiscovery unchanged and instead only use the new components directly.

Upper discovery layer:
The result of this layer is an array of unprocssed extension objects by extension type and extension name.
RawExtensionsByTypeInterface

• Distinguish "raw extension" vs "extension" in class names and comments. Still the same Extension class, for BC reasons. But "Extension" has an ->info array added, whereas "raw extension" does not. Or more precisely, it is not guaranteed, because the same objects are passed around and modified.
• Declare new public properties on the Extension class, that were previously anonymous.
• The list of searchdir prefixes is swappable for testing.
  Maybe we no longer want this if we test with vfsStream.
  An alternative would be to optionally set an array
• The active profile name is swappable for testing.
  For some this may look like too much of interfaces and swappability. And maybe they are right. If we can instead inject the literal profile name as a string.
• The info parsing is swappable for testing, so we don't have to bother with the filesystem.
  Again, we could avoid this with vfsStream.
• No more setProfileDirectories(), which made other extension lists depend on the profile extension list. Instead, only the profile name is required. See RawExtensionsByType_FromRawExtensionsGrouped.
• The equivalent of this with file paths instead of extension objects: FilesByType_FromFilesGrouped.

Extension list layer:
The result of this is a list of extension objects with ->info array filled in, and a bunch of processing based on the extension type, which modules are enabled, etc.
To not nameclash with the planned ExtensionList classes, I named this one ExtensionsByType*.

• The steps to process extensions after construction are pluggable for testing.
  This is totally optional, we could instead just bake them into e.g. one class per extension type!
• Thanks to the swappability of scanning components, this can be nicely tested with virtual directory setup.

General considerations:

• Naming: Currently classes are named with a pattern that is unusal for Drupal but is what I got used to. We should figure this out later in the process, but I think it is ok for experimenting.
• Every member variable is private, not protected. All the supposed benefits of protected member variables vanish, if your classes are sufficiently small. For the few remaining cases of inheritance, member variables of the parent class are intentionally concealed from the child class.
• I think it totally matches all the letters of "SOLID".
  S (Single responsibility) - Classes and interfaces are really small. Some may say too small, and too many. We can always "collapse" some of them.
  O (Open/closed) - Small enough components that if we ever want to change something, we likely don't have to change any implementation, but only their composition.
  L (Liskov substitution) - This is part of the language, if we work with interfaces..
  I (Interface segregation) - Most of the interfaces have only one method. Not sure how the reset() method fits in, but that's ok.
  D (Dependency inversion) - "Depend upon abstractions". Yep. All the components depend on interfaces, not implementations. They do not create their own helper objects, but instead have stuff injected.
  

Fixes:

• Info files in the searchdir itself, such as sites/default/foo.info.yml are no longer found. I think this was a misbehavior anyway.

Issues remaining or newly introduced:

• For some this may seem like too many layers with explicit interfaces. Maybe some of it can be squashed, and maybe some of it can be made more generic and reusable. File streams may make some of the swapping out obsolete for testing. I just prefer to start with too many than too few interfaces.
• Class names do not conform with usual practice in
• Variable names are messed up, lowerCamel vs snake_case. This is not the time to discuss this!
  Overall this is not a time for code review, but architectural planning!
• The "file cache" is somehow lost in the modified ExtensionDiscovery. I am sure it could be re-added as a decorator somewhere.
• The ->reset() mechanic is incomplete. I'm not really sure how much of it is needed, and whether it should be part of the interfaces.

For BC reasons, the following was *not* changed:

• Extension objects are mutable. This sucks. But a lot of components depend on it. So it would be hard to change this.
• We statically cache mutable objects. All modifications on the extensions leak into the static cache.
  The demo code still contains a static cache for objects, see SearchdirToRawExtensionsGroupedSingleton. This can be used to replace the static cache in ExtensionDiscovery, while replicating its behavior.
  But it also contains an alternative tool which caches file paths instead of objects.

Btw I'm a bit undecided or confused about the semi-mutability of cached / buffered data providers.
But I think we can live with it.

This is a hell lot of a long post. I don't have yet the time to read it all.
I'm sorry, but my brain cannot hold 30 files in my head when scanning files.

The trick is to look at one component at a time, and stop trying to think too much about the context where it is used.
Also it helps to see the code in an IDE where you can jump around.

But as said further down, maybe we can squash some of the interfaces and classes. I find it a bit too much myself, especially if we need additional layers to ease the construction and composition of components.

The above list is a quarry. We can pick and choose.
But I think it can be useful to have a proof-of-concept implementation, instead of doing it all in theory.

Can you describe why your system is faster? I would guess there should be a way to add those kind of optimizations to the existing system. ... Need to run

I think it is faster by using good-old opendir() + readdir() instead of RecursiveDirectoryIterator. I was sceptical when I first saw the DirectoryIterator in ExtensionDiscovery. And I don't really think it helps readability, or anything else. I don't understand why this was introduced. Maybe it looks "modern".

The opendir() + readdir(), or even another method, can be applied to the existing ExtensionDiscovery system. But doing this on the existing system would mean to move all the logic from RecursiveExtensionFilterIterator into ExtensionDiscovery, making this class even more scary.

I would prefer if we have a standalone and separately testable directory scanning tool, which we can then optimize. E.g. I recently made some experiments where glob() was even faster than scandir() + readdir(). But this depends on the way it is used, and it is not clear whether the same optimization can be applied here.

-----

Otherwise, performance should be mostly the same in terms of performance, because it does the same steps.
The additional interfaces and indirection should be fairly cheap in performance terms, because they make one call per array of files (e.g. per searchdir, per extension type, etc), instead of a separate call per file.

So I think there is no structural reason why one system should be significantly faster than the other, if both are optimized in the best possible way. But this optimization would be easier in a new system, where the relevant components are standalone and can be individually tested.

We came up with a highly optimized version in #2188661: Extension System, Part II: ExtensionDiscovery

The optimization here was that we have dedicated logic for extension discovery, instead of a one-size-fits-all file_scan_directory().
I don't find any comment that talks about the difference between RecursiveDirectoryIterator vs opendir() + readdir() vs scandir() vs glob().
The numbers show that whatever we did with SystemListing[Info] was slower than what we now do with ExtensionDiscovery. They do not show that whatever we are doing now is the best possible.
Also I am a bit suspicious about the numbers. They are so far off, that maybe the SystemListing[Info] includes parsing the info file. This is obviously slower, but does not tell us much about the fastest way to discover the files.

Btw, in this issue my goal is not really performance. I just want to preemptively counter any concerns that a cleaned-up system might be slower.

I still think we can improve things, but why we cannot do an incremental improvement is just not obvious for me

Imo, to fix ExtensionDiscovery, or to make it suitable as a service, we have to change its public API and behavior, because this is part of the problem.

There are two ways to do this.

• First option:
  (all in one patch/commit):
  1. Modify the class ExtensionDiscovery itself, in a BC-breaking way.
     Update all code that uses it, all instances at once.
     Possibly break contrib.
• Second option:
  (in separate patches/commits):
  1. Leave ExtensionDiscovery as it is, with some minor non-BC-breaking fixes.
  2. Create an alternative class or mechanism that is more reliable, easier to test, suitable as a service, etc, overall just better. So far, no risk of breaking stuff, because we are not using it anywhere.
  3. Write tests to verify that the new thing produces the same result as the old thing, in those cases where we intend to use it.
  4. Slowly replace usages of ExtensionDiscovery with the new thing, and verify that nothing breaks.

The second option is a lot safer and BC-friendly.
The initial patch will be quite big, but it will be risk-free.
The risky patches can be done

Btw, this is exactly how I understand the "O" in "SOLID" (after a long time of confusion): Stop modifying the behavior of existing classes / components. Once a component is finished, with clearly defined specs, leave it alone ("close" it). Then write a new component for your changed requirements.
(Maybe I am still getting it wrong, and the "O" means something else. But at least this meaning makes sense to me and reflects the way I think and work.)

So far, this does not automatically imply a "total rewrite".
We could instead just copy ExtensionDiscovery to ExtensionDiscovery2, and apply gradual modifications. But this just means spending more time with fragile code, instead of focusing on something beautiful.

-------------=========-----------

Besides all of this:

I find the incremental approach, depending how it is executed, depressing. It kills creativity, it makes us blind for the "big picture", it prevents real architecture, and is possibly one of the reasons why some problems take so long to fix. (This can be a long discussion, where this issue is the wrong place.)

I need at least a vision and possibly a proof of concept how I want things to look like. Then I can go back to baby steps, but with an inspiring direction in mind.

I understand that we need to stay "shippable" and avoid BC breaks. But as illustrated above, I do not see a contradiction here. In fact, a rewrite can sometimes be less risky than changing the behavior of existing components.

We (@dawehner, @donquixote) agreed in IRC that we continue to follow different angles. This means, whatever we do here, we still continue to work on the existing ExtensionDiscovery, e.g. #2605654: Modify Drupal\Core\Extension\ExtensionDiscovery to allow for scanning multiple file systems, enabling vfsStream testing

Opinion

You need to reduce the long term maintenance cost on midterm and longterm which means you cannot just drop existing knowledge about components completely

I think we have different ideas what rewrite means.
Think of a city with a river, and a bridge connecting the two sides.
One day new requirements appear, and the existing bridge is no longer sufficient.
We can now refit the existing bridge, causing traffic jams or worse during construction time.
Or build a new bridge at a different place, and leave the old bridge in place. No traffic jams. We can leave the old bridge around as long as we want.

We do not "drop existing knowledge" this way.

Well, I strongly disagree with the idea that incremental improvements doesn't help. Its the ONLY way forward in an opensource project to stay sane.

How has this worked out for us so far? Where are we now, where will we be 2 years from now?
I'm afraid that a lot of the problems we have today will still be there in 2 years.

Proposal

But seeing that we disagree about this, here is what I propose:

We regard this code as "proof of concept", and we keep this issue for experimenting.
I might post a few patches based on the code, to test if this works for the installer and other places.
(spoiler: I already tried some of it, and it was ok so far)

At the same time, we continue working on the incremental improvements.
I have some ideas how we can do this without breaking BC of ExtensionDiscovery. Basically we turn this class into a transformer, which behaves awkward by default, but can turn into something sane with alternative constructor setup. We add static factory methods that give us the sane behavior by default.

Once this works, maybe the difference to what I am proposing here is not as big anymore.

EDIT: In the metaphor above, this means we build a demo bridge, then we refit the old bridge based on the demo bridge. But we never open the demo bridge for the public (= we never commit it).

Other example:
Imagine PHP that the behavior of array_multisort() is awkward.
So they have 3 options:
1) Change the existing behavior.
2) Add an additional parameter combo which now changes the behavior.
3) Provide a new function which does the job better.

I am changing the title to "replacement". Maybe it sounds less threatening :)

(The direction I proposed in #10 still applies nonwithstanding.)

Nothing shall happen here before we have better test coverage.
#2729711: Improve test coverage for ExtensionDiscovery