Problem/Motivation
Part of #2186491: [meta] D8 Extension System: Discovery/Listing/Info to untangle the Drupal 8 extension (module, profile, theme, theme_engine) system.
Proposed resolution
-
Provide a common way to list extensions (enabled and not enabled ones)
→ Central data source for
ExtensionList
(ModuleExtensionList, ThemeExtensionList, ...) -
→ Provide common methods for e.g. resolving dependencies
→ Use it directly for profiles and theme engines.
This issue deals with the listing of available profiles/modules only, theme and theme_engine will be dealt with in #2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList.
Review instructions
Note: There is a change in DrupalKernel around sessions, read more about on #2208429-98: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
Remaining tasks
Final reviews
Commit
User interface changes
None
API changes
New ExtensionList
, ModuleExtensionList
and ProfileExtensionList
classes. New extension.list.module
and extension.list.profile
services to access information on modules and profiles.
Note: This change is still BC compatible.
Data model changes
Module info is now stored in the default cache and no longer in state.
Previous Report
- The "list" of extensions defines the list of enabled/installed extensions.
- There is a list for each extension type: profile, module, theme, theme_engine.
The installation profile
- is defined as
$settings['install_profile']
in settings.php (but only its name) - is added to the list of enabled modules with a weight of 1000 in
drupal_rebuild_module_data()
- is contained in the
%container.modules%
parameter on the service container
There is a second "parent" installation profile when a test is executed, which
- is defined as
$config['simpletest.settings']['parent_profile']
in config/settings.php (only its name) - is NOT added to the list of enabled modules
- is NOT contained in the
%container.modules%
parameter of the service container - is only used by
ExtensionDiscovery
to additionally find extensions in the parent installation profile, from which tests were executed.
The list of enabled modules
- is maintained by
ModuleHandler::install()
andModuleHandler::uninstall()
- is stored in the
core.extension
configuration object - is contained in the
%container.modules%
parameter on the service container - can be manipulated at runtime (without touching configuration) via
ModuleHandler::setModuleList()
- is consumed by
system_rebuild_module_data()
to enhance module info with a$status
property reflecting the installation status
Aside from that and the system_rebuild_module_data()
aspect, management and maintenance of the module list is pretty clean already.
The list of enabled themes
- is maintained by
ThemeHandler::install()
andThemeHandler::uninstall()
- is stored in the
core.extension
configuration object - is consumed, enhanced with theme info, and cached by
system_list()
- is consumed by
system_rebuild_theme_data()
to enhance theme info with a$status
property reflecting the status (whereas there is no installation status yet)
Except of the core.extension:theme
configuration, this list factually does not really exist on its own right now — it is deeply intertwined with theme info currently.
The list is also not stored in an efficient way like the %container.modules%
parameter on the service container; i.e., the system always has to consult the configuration system to retrieve it.
The list of theme engines
- does not exist at all.
ThemeHandler::rebuildThemeData()
blatantly performs an ExtensionDiscovery
to simply retrieve all available engines.
Proposed solution
-
Provide a common way to list extensions (enabled and not enabled ones)
→ Central data source for
ExtensionList
(ModuleExtensionList, ThemeExtensionList, ...) -
→ Provide common methods for e.g. resolving dependencies
→ Use it directly for profiles and theme engines.
-
Try to untangle at least some of the theme list/data/info mess.
This issue deals with the listing of available profiles/modules only, theme and theme_engine will be dealt with in #2659940: Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList.
FAQ for reviewers
The module installer doesn't have services injected, see #2208429-313: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
Comment | File | Size | Author |
---|---|---|---|
#341 | Screen Shot 2018-01-26 at 10.29.41 PM.png | 67.02 KB | almaudoh |
#334 | interdiff-2208429.txt | 2.96 KB | dawehner |
#333 | 2208429-333.patch | 70.34 KB | dawehner |
#330 | interdiff-2208429.txt | 1.07 KB | dawehner |
#330 | 2208429-330.patch | 70.31 KB | dawehner |
Comments
Comment #1
sunComment #2
sunComment #3
sunComment #4
catchIf we're doing this we should also separate out install/uninstall etc. from list retrieval and hook invocation. Most of the ModuleHandler dependencies are undeclared, I'm not sure it'd be possible to actually declare them without circular references, and even if it is they make more sense split up.
Comment #5
sunYeah, splitting out the install/enable/disable/uninstall functionality into a dedicated
ExtensionInstaller
class would be a good idea.With that goal in mind, I already "prepared"
ThemeHandler
in #1067408: Themes do not have an installation status by copy/pasting the code and logic fromModuleHandler
(even though the code contains some nonsensical logic, which felt silly to copy as-is, but did so nevertheless for the sake of making them uniform).Comment #6
donquixote CreditAttribution: donquixote commentedI was trying to get an idea how ModuleHandler works, and there was this comment sending me here.. so here are some thoughts.
A good idea seems to be to split "available extensions" (with discovery) from "enabled extensions". The former can be rather low-level, not depending on enabled modules. The "available extensions" would be without any hook_system_info_alter() applied. Instead, there would be an additional level on top of available modules and enabled modules, "altered available module info". This may limit what can be done with hook_system_info_alter(), but also make it more predictable.
I would suggest to split up ModuleHandler even more than that.
Currently what I imagine is to split it into really small pieces like ModuleImplements, ModuleImplementsDiscovery, ModuleImplementsAlter, HookSystem, etc, but I might change my mind about that.
One obvious benefit of small pieces is that each piece will be individually unit-testable, and that pieces can be replaced by mock objects. E.g. you could create a HookSystem with a fake (mock) ModuleImplementsDiscovery, or with a fake module list, so you can test it without setting up a Drupal installation, or without actually creating example implementations in test modules.
Slicing it into pieces could also make it easier to solve the circular dependencies.
Another solution for the circular dependency can be passing stuff to the actual method that does the business of an object, instead of giving the object a permanent reference in the constructor.
E.g.
instead of
We could also do some observer stuff, so that ModuleImplements automatically gets a reset whenever ModuleList is reset, if these end up to be in two different classes.
Comment #7
sun@donquixote: Discovery of available extensions has been separated into
ExtensionDiscovery
already.The primary objective of this issue to "consolidate" the base concepts of
ModuleHandler
+ThemeHandler
into a genericExtensionHandler
, since their code is fairly similar.This change MAY also involve to split the install/enable/disable/uninstall functionality into a separate logical piece of an
ExtensionInstaller
or similar.However, changing internals of the module hook system in
ModuleHandler
is not within the scope of this issue and off-topic here, as that is too specific to modules. — Some level of theme interaction is currently mixed into that, but #2228093: Modernize theme initialization has a concrete plan and very good reasons to remove themes altogether from it.In other words, this issue here architecturally operates on a macro/meta level, having the goal of introducing a formal notion of a "list of active/enabled extensions" (per extension type), since that notion/logic only exists in the form of inconsistent one-off implementations currently (and does not exist at all for half of the known extension types).
PS: Regarding module hooks, have a look at #2237831: Allow module services to specify hooks
Comment #8
jibranSo what's next here?
Comment #9
dawehnerAdding a related issue
Comment #10
dawehnerAlright, here is a first version of a extension listing service, using the module handler as example.
Once we have that we could also get rid of the other custom code in ThemeHandler
Comment #12
dawehnerLet's fix failures!
Comment #13
dawehnerMh, that seem to get stuck in the bot, but --all works kinda locally. Anyone has a concrete suggestion?
Comment #14
dawehnerLet's do a bit less, and fix the tests first.
Comment #17
dawehnerLet's see whether this is enough so far.
Comment #19
dawehnerWow, that was tricky to figure out, but as @berdir wrote in IRC, these tests find pretty bugs.
Comment #20
stefan.r CreditAttribution: stefan.r commentedHaving worked with this in the other issue, this does look nicer already! :)
Should this really have n/a as a fallback? What if the extension has accidently been removed and we can't find the info file (we do seem to account for this in drupal_get_filename), shouldn't we at least show the machine name?
This should probably set
$this->extensions
?This method name could be clearer as it actually rebuilds the extension list. Also just out of interest, we do a bunch of other stuff in
system_rebuild_module_data()
with this data (add weight, status, schema version, add dependencies, cache file names), are there any plans of moving that out?Comment #21
dawehnerThank you for your review.
What about throwing an exception in those cases?
Good point!
Well yeah, my strategy was to get the first bit converted and passing and then tackle the next one.
Let's see whether this works already.
Comment #23
dawehnerUps, that was horrible.
Comment #25
dawehnerJust fixes.
Comment #28
dawehnerUpdates the IS a bit.
Comment #29
jibranI think it's good to go but we need a change notice and beta evaluation for this.
Comment #30
stefan.r CreditAttribution: stefan.r commentedreroll
Looks like this gets rid of the circular call @effulgentsia mentioned here? http://www.drupal.org/node/2281989#comment-9840429
Comment #31
Wim LeersLooks good. But either the IS is outdated, or this still needs to be updated to handle themes/profiles/theme engines as well.
s/module/'module'/
s/theme/'theme'/
What about install profiles and theme engines, like the IS says?
s/entries/extensions/
Can be done now?
80 cols.
Comment #32
dawehnerWell yeah, I was thinking about just doing it for modules for now and update the usages in themes and install profiles later ... feel free to disagree with that process,
but it seemed to be the most straightforward one.
Comment #33
dawehnerWe (alexpott, xjm, stefan_r, dawehner) tried hard to discuss, whether this issue can be done in 8.0.x.
It seems to be that solving that in a sane way is 8.1.x stuff, though.
@alexpott @xjm
If you have changed your mind, please comment.
Comment #34
dawehnerI'd like to keep the amount of changes as small as possible.
Comment #38
dawehnerJust tagging
Comment #39
amitgoyal CreditAttribution: amitgoyal commentedReroll of #34.
Comment #41
amitgoyal CreditAttribution: amitgoyal commentedAs per #39, invalid PHP syntax in core/modules/system/system.module fixed.
Comment #45
anavarreComment #46
dawehnerWell, we will not work on that before 8.1 and then need a proper strategy.
Comment #47
alvar0hurtad0Here's the reroll
Comment #48
alvar0hurtad0I forgot the status (again).
:D
Comment #49
jonhattanI guess it is isset(), without
!
.you want to actually *set* cache here.
Comment #50
dawehnerYeah that that point we need to address with the big complain that this is adding another cache laying without getting better with the full crazy situation.
We need to come up with a full plan for 8.1 and do small steps towards that full plan.
Comment #51
dawehnerDid some reroll and a general different approach::
a) We now use cache on write
b) But still rebuild the cache info if accessed directly.
Comment #54
almaudoh CreditAttribution: almaudoh commentedThe last patch was failing because of a recursive function call from
ExtensionList::recalculateInfo()
andExtensionList::recalculateFilenames()
.So my understanding of this is that at the end of the refactoring the following should be the mapping of old procedural functions to new
ExtensionList
methodsExtensionList
methodsystem_get_info()
::getInfo()
_system_rebuild_module_data()
::reset()
::listExtensions()
system_rebuild_module_data()
::reset()
::listExtensions()
drupal_get_filename()
ExtensionList::getFilenames()
Here's a fix that does that...
theme_listing
has not been implemented yet even though it is used insystem_get_info()
so expecting some test failures.Comment #56
almaudoh CreditAttribution: almaudoh commentedLet's just focus on modules for now...
Comment #57
dawehnerNice work @almaudoh
Ah, so this reverts the theme handling bit?
Comment #58
almaudoh CreditAttribution: almaudoh commentedYeah. The theme handling part is a bit messy, so I wanted to get the module handling part passing tests first before digging into the theme bit.
Comment #60
almaudoh CreditAttribution: almaudoh commentedNow
drupal_get_path()
is limited in theModuleInstaller
, so we replace it with themodule_listing
service. Also, having problems with\Drupal::state()
saving to the database, so commented that out just to confirm that the thing works first.This is far from complete, but sending to testbot still...
Comment #62
Mile23I think it's great to be normalizing all this stuff so it's only in one place, and meets a bunch of different needs.
Some stuff I see...
If it needs to be the state service, then inject it in the constructor.
But it should be in the cache, instead. I don't think we have a way to clear state from the UI, and having a filesystem problem and a non-clearable cache is bad news for users.
All available or all enabled? Maybe rename the method to
listEnabledExtensions()
if the latter.Priming this cache doesn't help you if you're normalizing on
ExtensionDiscovery
, only wastes time and eats up memory in a static.Comment #63
almaudoh CreditAttribution: almaudoh commentedSo
drupal_get_path()
/drupal_get_filename()
are no more effective. Let's see how many test fails this fixes.#62:
1. Totally agree it should be in cache, not state, but I guess the main consideration was maintaining BC, since we don't know how many contrib modules out there are using
\Drupal::state()
directly to get module info.2. All available. There's a boolean
ModuleExtension::status
to know if an extension is enabled.3. Yeah, will be removed in the next patch.
Comment #65
BerdirThis could result in notices if $name is missing I guess? What about adding getFilename($name)?
I guess we can't officially deprecate this function as it's still needed for themes, but maybe we can add documentation on what you should call instead for modules?
getName($name) reads a bit strange. We have two different names apparently. Not sure if we can do something about it, but possibly worth thinking about. Maybe start using $machine_name or so?
Is that todo from the old code?
We can't do that before 9.x, so maybe just get rid of it?
I think we should stop with that "if empty, return all" stuff. Add two methods, one that returns all, one with a required $name argument.
missing docs.
still true, is this a method now?
Returns, missing description below.
why commented out?
this calls back in here again I think, so we should update that and call this->something.
the method reads like a converted function name. methods usually start with a verb, maybe we can just leave out the moduleData prefix?
it's just a static method, but it's still kind of a dependency, while ModuleHandler also depends on this. Maybe move the static method here, or make it a trait?
As mentioned before, we should inject this.
yeah, this can't be a replacement. as I said, we should add getFilename($name), and looking at this, we should also add getPath($name)
If this is a replacement for _system_rebuild_module_data() then we should remove the old call?
ẁe should remove the drupal_static_reset() calls for this function
is this meant to be done in this issue? if so, then we can deprecate this and a lot of other functions for real.
this is prefixed with _, but I'm not sure if we can really safely remove that?
same here for the static reset thing.
keeping $modules_cache is pretty useless, just return it.
yeah, like those, we should remove the old calls.
Comment #66
BerdirAlso, looking at module.inc for other functions to be replaced by this service (I think we want to remove this and other files like this completely in 9.x). Likely in follow-ups:
* system_list() is the part that would move in a ThemeExtensionList service I guess?
* module_load_include() has an interesting todo
* drupal_required_modules()
* module_config_sort() could be a static method somewhere.
* Not sure where module_set_weight could live, module installer maybe?
Comment #67
dawehnerThanks a lot for that review @berdir!
Comment #68
almaudoh CreditAttribution: almaudoh commentedLots of changes in this patch. I reverted the hack in #63 and properly addressed the underlying issues. Theme still not yet included in this though.
profile_listing
serviceProfileExtensionList
to decouple profiles from the special behavior of modules listing.ExtensionDiscovery
an instance property to allowModuleExtensionList
set the profile directories before scanning for modulesExtensionList::getFilename()
andExtensionList::getPath()
to directly replacedrupal_get_filename()
anddrupal_get_path()
drupal_get_filename()
now calls out toExtensionList::getFilename()
for modules and profiles (themes will be done later)#65: Thanks for the review @berdir
ExtensionList::getFilename($name)
. Also see point 3) & 5) above.$machine_name
. Can still be improved.getAllInfo()
method.ensureRequiredDependencies
ModuleExtensionList::ensureRequiredDependencies
Comment #70
almaudoh CreditAttribution: almaudoh commentedAs a matter of fact, we cannot use State to store extension lists and info because State depends on the system module, and the system module needs module lists for installation -> a circular dependency. So we have no choice but to use cache.
Fixed a couple of bugs and test fails and reverted some changes missed out in previous patch.
Comment #72
Berdir12. That's a good question. I'm not sure myself. We usually say that the constructor isn't part of the API, so we are allowed to change it in minor releases. But especially on base classes and commonly subclassed classes it is indeed very problematic. There's a documentation page somewhere that tries to specify this.
This seems to be missing an argument?
I think we usually document this as string[] as we know the inner type.
Comment #73
almaudoh CreditAttribution: almaudoh commentedMore fixes...
Comment #74
dawehnernice work!
How do we deal with the faster provider optimization here?
This method could be
$this->getExtension($name)->info['name'] directly
Note: We no longer use FormattableMarkup for exception messages. Let's use
"The {$this->type} $machine_name does not exist."
Should that kind of documentation in general just talk about extensions?
Do we want to throw an exception as well, if the extension don't exist?
It would be nice to document the usecases of setting the filename. In general I think we want to document that its not encouraged to call it.
It would be nice to document why we have this additional property on top of the
fileNames
onesame as before
IMHO we should document that this method can also throw an exception.
Nice!
I think it would be fine to have a dedicated follow up to deal with the theme aspect. Do you think we should create one?
Comment #75
dawehner@almaudoh I'll work on some unit tests at the meantime.
Comment #76
dawehnerThis is wrong, it should be if
(isset($extensions))
insteadComment #78
dawehnerHere is a unit test.
Comment #80
almaudoh CreditAttribution: almaudoh commentedThanks for the reviews.
#72
#74
ModuleHandler
first?Thanks for the unit test @dawehner
Comment #82
dawehnerIt would be great to talk with catch about it, whether he believes we still need that optimization.
Comment #84
almaudoh CreditAttribution: almaudoh commentedWoah!! is DrupalCI down???
Comment #86
almaudoh CreditAttribution: almaudoh commentedLooks like CI is choking on the unit test. Here's a patch without the unit test just to confirm...
Comment #88
catchThat's OK to change in a minor version. Module info happening to be in state isn't part of the public API, per https://www.drupal.org/core/d8-bc-policy
#80/#82 - can't really answer that question without profiling. However I'd hope that #2351919: Replace uses of drupal_get_path() with __DIR__ where possible means the answer is 'no'.
Comment #89
BerdirYes, this will most certainly require profiling anyway.
The failing tests indicate that we're missing some cache clearing, something updated them before and doesn't anymore, it seems.
Comment #90
dawehnerI absolutely agree, this change is quite lowlevel so we really should be better safe than sorry.
The @group as simply missing.
Comment #93
almaudoh CreditAttribution: almaudoh commentedFixed two test fails...
Comment #95
dawehnerWhat about something like this?
Comment #96
stefan.r CreditAttribution: stefan.r commentedComment #97
almaudoh CreditAttribution: almaudoh commenteddawehner++. So the test fail is due to unintended initialization of accountproxy. I've spent 3 days tracking down that test fail without any success :).
Comment #98
dawehnerThis was a classical instance where a fresh mind totally helps.
I'm honestly not sure how this actually works in HEAD.
\Drupal::getContainer()
is pointing to some old container, and by that contains some old, not reloaded configuration information.Comment #99
almaudoh CreditAttribution: almaudoh commentedSo the next question is who maintains the list of enabled / installed modules?
ModuleHandler
,ModuleInstaller
orModuleList
. I would say it should be theModuleList
mainly to avoid circular dependencies. I've seen a similar symmetrical case withThemeHandler
andThemeInstaller
.Comment #100
almaudoh CreditAttribution: almaudoh commentedDid some (not so) quick profiling with ab. TL;DR No signifaicant difference in performance between HEAD and patch, even though there seems to be a slight improvement on the front page with the page. Expect slight performance improvements on pages where
drupal_get_(filename|path)
is called multiple times.Five different scenarios were compared for HEAD and the patch in #12
ab -c1 -n1000 http://drupal.d8
ab -c10 -n1000 http://drupal.d8
ab -c1 -n10000 http://drupal.d8
ab -c1 -n500 http://drupal.d8/admin/people/permissions
ab -c1 -n500 http://drupal.d8/admin/modules
Results
ab -c1 -n1000 http://drupal.d8
ab -c10 -n1000 http://drupal.d8
ab -c1 -n10000 http://drupal.d8
ab -c1 -n500 http://drupal.d8/admin/people/permissions
ab -c1 -n500 http://drupal.d8/admin/modules
Comment #101
catch@almaudoh thanks for the load testing with ab. It'd be better to have profiling with xhprof so we can see the difference in function calls - easier to see if something unexpected was happening, and usually individual changes aren't enough for ab to give reliable data.
Comment #102
almaudoh CreditAttribution: almaudoh commentedComment #103
Mile23#95 needed a reroll, so here it is.
Comment #104
dawehnerHere is a total unscientific profiling (frontpage, without page_cache/dynamic_page_cache, no content)
The relative numbers are pretty 0, there seems to be no measurable difference.
The two runs are attached
Comment #105
dawehnerSame scenario, this time with some more stats, powered by xhprof-kit:
Comment #106
dawehnerInteresting enough the profiling in #104 had an accidental apc cache set involved.
This is a screenshot of another run, without that: https://www.drupal.org/files/issues/Screen%20Shot%202016-02-29%20at%2015...
Comment #107
almaudoh CreditAttribution: almaudoh commentedSoo...now that we're in beta, is there any chance this can go in? We still have some refactoring to do after this. And it will help us progress #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname(), fix #2442383: Add the option to cache drupal_get_filename() and some others...
Comment #108
almaudoh CreditAttribution: almaudoh commentedComment #109
dawehnerNo idea. I asked catch, well maybe I'll ping him again. Maybe berdir would be also a great person for more reviews.
Comment #111
dawehnerRerolled + some minor fixes
Comment #112
alexpottWe should deprecate ModuleHandler::getName(). ModuleHandler is far to low level to be handling that.
Why's there a reset in here?
]
Comment #113
dawehnerSo before,
_system_rebuild_module_data
used to be the function which didn't had any kind of caching. As we now have encapsulated the behaviour in its own service, and still keeping the exact same behaviour, resetting helps us to keep the behaviour.Comment #114
dawehnerRerolled against 8.2.x coding standard changes.
Comment #115
alexpottI think given the interface rule we recently we agreed we should just add the method to AccountProxyInterface and include this in the CR. Less complexity and weirdness. Unless we reckon there are lots of alternate implementations.
Should be the service name not the class name since services can be swapped out.
Comment #116
dawehnerThank you alex!
A cool
Comment #117
alexpottSo we need a CR ;)
Comment #118
dawehnerAdded one ...
Comment #119
andypostwhy this change?
"initialized" is internal state and implementation details of service
\Drupal\Core\Session\AccountProxy::getAccount()
Comment #120
dawehnerLet's add the explanation to the issue summary.
Thank you for your review @andypost!
Comment #121
alexpottHmmm @dawehner I think @andypost has a point. Whilst the fix in #98 fixes the test fail - is it the best fix?
Comment #122
andypost@dawehner I'm sure that Kernel should not know new interface method to be able to run!
I'm sure that API addition makes no sense in scope of the issue
Very probably this points to some error in clean-up or execution logic as #95 shows
Maybe changing this order caused account service state change
Comment #123
dawehnerWell,its needed to fix the bug in #95, its not just we do it for fun. It already makes sense.
Comment #124
alexpottIf we do need this - shouldn't it be called isAccountInitialized()
Comment #125
alexpottOr maybe even just hasAccount()?
Comment #126
dawehnerThis is just internal caches, so that doesn't rebuild the container, so it doesn't matter here.
@alexpott
I fear you are actually right :)
Comment #127
XanoThis patch improves documentation, centralizes cache key definitions, marks
AccountProxyInterface
as internal, implements the feedback from #125, and renames the services to have a consistent prefix so they're easier to find (similar to what we do with plugin managers). I did not find anything conceptually wrong with this patch.Comment #129
XanoComment #130
almaudoh CreditAttribution: almaudoh commentedLike the new names
We should make this match the general pattern as well:
'core.extension.list.' . $this->type
We can use the new cache ID helper function here too.
Comment #131
dawehner+2 to all of those changes. Nice improvements!
Totally agreed!
Comment #132
donquixote CreditAttribution: donquixote as a volunteer commentedArchitecture issues:
I think we should be using composition, instead of squeezing it all into one inheritance hierarchy.
There could be one "layer" that just returns an array of extension objects, and nothing more.
A cache layer in between, using the decorator pattern.
And then another layer on top of it which provides all the other methods like extensionExists().
And another layer on top of it with the getInfo() and getAllInfo().
In the patch, we are treating the Extension objects as mutable. That is, we are writing information on them after construction, in doListExtensions(). This seems undesirable to me.
Especially, the mutable object is stored in the static cache in ExtensionDiscovery::$files. So all the modifications have side effects.
I think instead we should have one layer that reads and caches and buffers the raw discovered extensions from the filesystem, with their *.info.yml files.
And then another layer that adds dynamic information, e.g. with hook_system_info_alter(). But in a way that does not pollute the original objects. E.g. there could be a RawExtensionObject representing the raw discovered extension as it lives in the filesystem. This can be statically cached, because it does not depend on any dynamic information. And then an Extension object referencing the RawExtensionObject. The Extension object would not be cached statically, because it depends on e.g. hook_system_info_alter().
---------------
Implementation details of current approach:
There should be an interface, e.g. ExtensionListInterface. Or multiple interfaces, if we do split up the responsibilities.
This way, consumers are not forced to depend on a concrete implementation.
The abstract base class should be named ExtensionListBase. Or are we no longer doing this?
We are confusing human name and machine name.
E.g. getName($machine_name) gives us a human name, but takes the machine name as the parameter.
On the other hand, in extensionExists($name), "name" means machine name. This is inconsistent.
I am not a friend of methods that sound like getters, but take a parameter. For me, $object->getFilename() means get the one filename of $object. I would personally use method names like extensionGetFilename($extension_name), if it takes a parameter, to indicate that the returned filename is relative to the parameter. Especially on a service like the extension list, this allows to distinguish between methods that apply to all extensions in the list, from those that target a specific extension (by name).
Maybe this is just not the way we do things in Drupal core - then so be it.
Maybe I will come up with a patch. I am still looking and thinking.
Comment #133
donquixote CreditAttribution: donquixote as a volunteer commentedBtw, ExtensionDiscovery already suffers from the same problems:
- Lack of interface.
- Lack of separation / composition.
- Static cache of mutable objects. First modification already happens in scan(), after they are already added to the static cache.
Comment #134
donquixote CreditAttribution: donquixote as a volunteer commented#129
On ExtensionList::getInfo():
"an empty array will be returned" should be "an exception will be thrown".
(Btw it would help if we could tell by the method name whether it returns NULL, empty array, or throws an exception, if nothing is found This is not currently the way we are doing things, but it would be a good thing.)
Comment #135
donquixote CreditAttribution: donquixote as a volunteer commented#129
"Returns a list of extension folder names" should be "Returns a list of extension file names".
Comment #136
donquixote CreditAttribution: donquixote as a volunteer commented#192, ExtensionList::doListExtensions()
The $extension->info is an anonymous object property.
Should we make it an explicit public property on class Extension? Like this:
Comment #137
donquixote CreditAttribution: donquixote as a volunteer commentedModuleExtensionList::doListExtensions():
We should avoid modifying objects long after they are created. It's a bad habit that often causes problems.
$this->extensionDiscovery is created in the constructor of ExtensionDiscovery. Modifications to the discovery object should happen there, not be delayed until doListExtensions().
Of course even better would be to inject the ExtensionDiscovery, instead of creating it in the constructor.
If this is somehow not possible due to early bootstrap limitations, then the state change should be avoided altogether, and instead only happen in local scope.
In ModuleExtensionList::doScanExtensions():
Some of this profile logic is repeated further down in doListExtensions(). Maybe this should happen only once?
Comment #138
donquixote CreditAttribution: donquixote as a volunteer commentedModuleExtensionList::doListExtensions():
It would clarify things if $profile was named $profile_name. Otherwise one would think that $profile is an array value of $profiles - so it would be an extension object, not a machine name.
And is scanning the profile directory really specific to modules? Should this really be in ModuleExtensionList? Or rather in ExtensionListBase?
Do we even need it at all? ExtensionDiscovery::scan() already calls ExtensionDiscovery::setProfileDirectoriesFromSettings(), which adds the profile directory for scanning. The only difference seem to be special cases for tests and during install.
Comment #139
dawehner@donquixote
Thank you for your feedback.
The goal of this issue is to NOT reinvent parts of the extension system, but rather provide a layer to list information and by that being able to get rid of a big dependency to system module. In order to do that, we don't want to change for example the mutability of the extension object, as the alter hook should not change its API layer.
Given that we also decided to not solve every problem at once, which means that we aren't done after this issue. Given that introducing an interface would be kind of a lie for me. A interface communicates that this is the final design. For me there could be for example a Module/Theme object that wraps/decorates an existing Extension object with the additional information coming from info files.
Good point, let's switch to
$machine_name
, see issue summary tasksSo you would suggest something like
getExtensionFileName
?See my explanation in this comment. This is also out of scope of this issue.
In general I hope we don't try to solve every problem we have in this issue but rather continue with a clear path where we can iterate further in the future.
PS: The reason extension discovery doesn't have an interface is that this layer is not meant to be swappable. It is required when you rebuild the container, there is really limited amount of swappability available at that point. Another point is that there a lot of assumptions about the way how extension discovery works (for example that /modules contain the modules), that swapping it out, is not easy/actual sane.
Comment #140
donquixote CreditAttribution: donquixote as a volunteer commentedVery true.
I wanted to restrict my comments to the new classes we are introducing: ExtensionList with its subclasses. I thought it should be easy to split it up.
Then I noticed that the mutability of the extension object, and the design of ExtensionDiscovery, is an obstacle when trying to split up ExtensionList.
But yes, if we do anything about ExtensionDiscovery or the Extension class, it should be in a separate issue.
While mutability in itself is already somewhat undesirable, the real problem here is something else.
It is the "mutations" leaking into lower layers, causing side effects on other consumers of these lower layers.
If ExtensionList finds a cached list of extensions, then ExtensionDiscovery will give other components a "raw" list of extensions, without the ->info or alterations.
On the other hand, on a cache miss in ExtensionDiscovery, the added ->info and alterations will leak into the ExtensionDiscovery::$files static cache. So other components using the discovery will get the prepared and altered extension objects.
To fix this, we don't have to give up on mutability in the "upper" layer. We could simply clone the objects coming from ExtensionDiscovery, to avoid leaking of alterations. If this happens in ExtensionList, it could be well within the scope of this issue. But if we want to open a separate issue for it, fine.
I personally use interfaces all the time, even for "internal" stuff, or stuff that I plan to change.
Interfaces allow to clean up the dependencies and communication between classes / objects. And classes depending on interfaces instead of implementations are generally better for testing. I prefer to have these qualities for internal objects as much as for external / publically visible objects. I rather have a bunch of interfaces that could possibly change, then a big and noodly class hierarchy. Maybe the respective interface could be marked as "@internal"?
(To not derail this issue too much, we can continue this point in IRC or elsewhere)
(So far I am only talking about interfaces for ExtensionList, ExtensionDiscovery, and possibly some in between. Interfaces for Extension objects are also desirable, but a different subject)
My first idea would have been ->extensionGetInfo(), ->extensionGetFilename() etc. But then we would also have extensionGetExtension(), which is weird, because now "extension" means both the object and the machine name. I think it is better if "extension" means just the object.
So the next idea is ->nameGetInfo(), ->nameGetFilename(), ->nameGetLabel() or ->nameGetHumanName(), ->nameGetExtension().
This may sound a bit awkward, but I think it is very clear. If the list object has a good variable name, then it will be
$extension_list->nameGetInfo($machine_name)
, which I think immediately explains what is happening.A question would be if we keep extensionExists(), or if this needs to be renamed nameExists() or nameIsAvailableExtension(). I think we can keep it as extensionExists(), and live with the slight inconsistence.
Comment #141
donquixote CreditAttribution: donquixote as a volunteer commentedI think the default value of this should be an empty array, not NULL.
Fyi: I am working on a patch that splits ExtensionList into smaller components, but (hopefully) is still within the issue scope. This also fixes details like this one.
Comment #142
donquixote CreditAttribution: donquixote as a volunteer commentedThe purpose of setFilename() is to make some extensions available even before the extension scanning process is available.
But in the new ExtensionList class, any attempt to access these "special" extensions will trigger a re-discovery, or cache lookup.
This was not the case with drupal_get_filename(), where a re-discovery or cache lookup only happened if the file was not previously registered.
I think this can be fixed by moving the $this->getFilenames() call further down, below the
if (isset($this->addedFileNames[$extension_name]))
.Comment #143
dawehner@donquixote
Its great to see that you agree with not fixing everything at once.
Totally, well we need to be able to distinct between the case where there is no result, and it wasn't scanned yet.
Good catch
Well as said, I think we should introduce interfaces when we are convinced that this is it.
Well I would argue that you are already in the context of extensions, given that you will have the extension (module/theme/profile) list initialized. Given that extension again would be sort of a duplicated information.
Comment #144
donquixote CreditAttribution: donquixote as a volunteer commentedAttempt to break up ExtensionList into smaller components.
This is the kind of architecture I usually work with. Or in fact I tend to slice it up even more.. but it is good enough for now.
With the break-up, I also did some renaming, which can be debated.
This means what was ExtensionList in previous patches is now ExtensionHub. I found this more reasonable than inventing another name for ExtensionListInterface. The list really is just a list, whereas a "hub" has a variety of methods for different purposes.
Note that the service names are not renamed.
Known issues:
/** @noinspection ... */
docs. We should remove these later, but for now they make it easier to review the code with an IDE (ok, probably only works with PhpStorm).*.list
, not*.hub
. This is up for discussion.DiscoveryExtensionListBase
andModuleDiscoveryExtensionList
can be debated.For now I just made the existing unit test to work with the changed architecture.
Comment #145
dawehnerSorry for this negative comment. I don't want to sound that negative.
There is IMHO no good reason to add a ExtensionDiscoveryInterface as part of this issue. Please don't try to make it harder that needed.
Its also out of scope of this issue to add info to the extension object. This kind of code existed exactly like that before.
Can you please elaborate why you think its helpful to add interface for every small bit, even if stuff will never be replaced in its implementation? Its harder to follow through multiple layers than one, as long each individual method is still simple. I mean no question for something which is meant to be flexible I totally get that kind of architecture, but this is more even like a
Its totally weird that we hardcode the type of extensions to 3 different kinds.
PS: There are gazillion of CS problems in your patch :(
Comment #146
donquixote CreditAttribution: donquixote as a volunteer commentedYou are right.
I am sorry. I meant to keep these changes out of this patch, and then missed to remove them.
Sorry if this is the case. I will review this later.
Do we? Not any more than previous patches, right? We already have ModuleExtensionList and ProfileExtensionList in the previous patches. The only thing I see being added here are the static factories like ExtensionHub::createForModules(). But I don't think this can be considered any more or less "hardcoded" than previous patches.
Also could it be that this is the wrong code snippet?
-----
The rest of your comment deserves a longer reply, which goes into a separate, follow-up comment.
(EDIT: After some IRC talk)
Comment #147
dawehnerI think I understand now why my brain has problems with this new direction: Instead of knowing 2/1 concept (ExtensionList/ModuleList) this patch now requires people for the full understanding and debugging on runtime to actually have
ExtensionFilenameList
,ExtensionFilenameListBuffer
ExtensionFilenameListCache
,ExtensionHub
ExtensionInfoListBuffer
ExtensionInfoListCache
DiscoveryExtensionListBase
ExtensionListBuffer
ExtensionListCache
Comment #148
almaudoh CreditAttribution: almaudoh commentedI agree with @dawehner that the last patch has introduced too many classes and interfaces that makes it even more difficult for a developer to understand the extension system. Additionally, the introduction of so many new interfaces bring along the risk of not being able to refactor in future (if people begin to use those interfaces - which may happen even when they are
@internal
) when the architecture has not been fully completed.I suggest we still stick to the narrow scope of this issue, which is the first step of decoupling the extension system from the
system.module
and also making it easier to understand. Further refactoring, based on actual use cases can be done in follow ups.There is the potential that this issue could be delayed further, while it is blocking some other cleanup issues e.g. #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname(), #2442383: Add the option to cache drupal_get_filename(), #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service.
Comment #149
donquixote CreditAttribution: donquixote as a volunteer commentedI think there are two types of people: Cake people, and cookie people.
The cake person says: I prefer the cake, because the cookies are too many.
The cookie person says: I prefer the cookies, because the cake is too big.
(assuming the same total weight or volume for both)
The cake person measures complexity as the number of "things".
The cookie person measures complexity as the size of one "thing".
The trick of the cookie person is to only look at one cookie at a time, and not be bothered with the other cookies.
One cookie fits nicely on a small plate. One cake does not.
On the other hand, the one cake sits more stable on the big plate, than a heap of cookies would.
This is not just a tradeoff, but a different mindset.
The cookie person will put one cookie on a small plate at a time, and be happy with it. The cake person will attempt to put all cookies on a big plate, and correctly observe that it sucks.
For me, whatever does not fit into one "page" of my brain, is "too big".
Looking at the average class size in Drupal, and the responses in this thread, I have to conclude that I am pretty much alone with this view. I am surrounded by "cake people", one could say.
Or, to be fair: Most of you probably agree that separation of concerns is a good thing. We just disagree how far we want to go. And we have different ideas about the use of interfaces.
I will therefore not further derail this issue, and move further thoughts into a blog post (which is still "under construction" while I post this).
Comment #150
donquixote CreditAttribution: donquixote as a volunteer commentedBack to the original approach (#139, unless explicitly stated otherwise):
@dawehner (#78 interdiff):
Do you remember the reason for this change?
Also @almaudoh in #68
And myself in #137
I think this is currently a bit weird, and we can do better.
(EDIT: This part is "fixed" in #152)
---------------
This part is also weird. First we don't care if $extensions[$profile] is set. And further down we do care?
In the original _system_rebuild_module_data() this was not an issue, because it was within
foreach ($modules as $key => $module)
. So it was guaranteed that $modules[$key] is set.------
It looks as if this is overwritten later.
Duh.
This may have been broken before already, dunno.
-----------------
This means that profiles are processed, by adding ->info and running them through hook_system_info_alter().
In previous behavior, this hook was only used for modules, *after* the profile had been added to the module list.
This means the processed profile is added into a list of unprocessed modules.
This is the old behavior, where the unprocessed profile, coming directly from ExtensionDiscovery, was added to the list of unprocessed modules.
------------
I am going to attempt to fix some of this, maybe not all.
Comment #151
donquixote CreditAttribution: donquixote as a volunteer commentedNeither old nor new code check if the $modules[$dependency_name] exists.
They just blindly write to a possibly non-existing object.
Or what am I missing?
EDIT: If this is intended, it needs to be documented.
Comment #152
donquixote CreditAttribution: donquixote as a volunteer commentedThe patches fixes some of the issues previously mentioned, but not all of them.
https://github.com/donquixote/drupal/compare/8.2.x...8.2.x-2208429-152-E...
->addedFileNames = []
on reset and initialization, so it is never NULL.->extensionDiscovery
, and instead call->getExtensionDiscovery()
directly.Comment #153
donquixote CreditAttribution: donquixote as a volunteer commentedThere is also some confusion about available vs installed / enabled extensions.
The doc comment here is not true.
And neither is this one.
Both these methods will return info arrays for available extensions, no matter if they are enabled.
At least this is what I conclude from looking at
ExtensionList::recalculateInfo()
.Comment #154
donquixote CreditAttribution: donquixote as a volunteer commentedThe assertion methods in PhpUnit are static.
My IDE (PhpStorm) tells me I should replace this call with
static::assertEquals(..);
.Do we follow this advice?
Comment #155
dawehner+1 for doing things lazy!
why is
$active_profile = $this->getActiveProfile()
not enough? A profile with an empty string in its name is certainly not supported.Good catch. Yeah, so to keep the core behaviour we currently have its indeed just the installed ones, see
\system_get_info
Comment #156
donquixote CreditAttribution: donquixote as a volunteer commented$active_profile is either an object or NULL. It never is a string.
Yes,
if ($active_profile = ..)
would also work.By doing the
if (NULL !== $active_profile = ..)
, we- avoid an implicit cast to bool.
- make visible which types of values we expect from the variable.
I picked this habit up somewhere, don't really remember where exactly. But it does make a lot of sense to me since then.
It forces or encourages a style where variables or return values have a clearly defined range of types / values.
Implicit casting is also slower than ===, even though this only matters for operations that are repeated a lot.
Comment #157
donquixote CreditAttribution: donquixote as a volunteer commentedBtw a lot of the problems I pointed out in previous comments still apply.
Why do we even have an "extension.list.profile" service? Where would this be used?
Comment #158
dawehnerAs written on IRC, profiles are also an extension type, so there should be a listing for them. Also, at some point we might support parents there, so a listing would be even more helpful.
Comment #159
dawehnerHere is some small updates to some of the points mentioned by @donquixote. Thank you!
Comment #160
XanoI removed an unused method parameter, made variable names more consistent, and made some small improvements to docblocks and inline code comments. No functional changes.
Comment #161
dawehner@xano
The derived method is using it!
Comment #162
XanoAh! I couldn't find any usages in the method I removed the method from, nor in the original code this patch replaces. Thanks for pointing this out.
Since this is not used by anything except one of the subclasses, should we keep (and document) it there only? There's no need for the base class or any other child to have this functionality, apparently. Since the original calling code did this filtering itself, I'm not even sure we should do this in
ExtensionList
now.Comment #163
dawehnerAdded a small follow up in #2719315: Try to install system module like any other module
Thank you @xano! As said on IRC we want this parameter as its part of the child methods.
Comment #164
donquixote CreditAttribution: donquixote as a volunteer commentedTo me this implies that ::getAllInfo() is a static method.
Maybe the solution is to write
Access this property's value through ->getAllInfo()
in the comment text, but use the@see static::getAllInfo()
in the @see tag?My IDE (PhpStorm) only understands the spelled-out
@see \Drupal\Core\Extension\ExtensionList::getAllInfo()
, or simply@see getAllInfo()
, but nothing withself::
orstatic::
. Do we have any convention for this? I don't see this specific case mentioned here. I think the fully spelled-out version would be the safe option.static::
is not meaningful in a@see
tag, imo.Imo this is a violation of the LSP. Semantically, at least. And confusing, definitely.
In the base class, the
$installed
parameter has no meaning at all. In the subclasses it suddenly has a meaning.The solution (*) is to make this two separate methods. Different meaning -> different methods.
Option a) Have one public method
getAllInfo($installed = TRUE)
, which is abstract in the base class.Then another method
getAvailableExtensionsInfo()
, which is implemented in the base class and does not need overriding.Option b) Get rid of the parameter altogether, and have a dedicated method getInstalledExtensionsInfo().
This method will be abstract in the base class. Or it may not exist there at all, if we think that some extension types (themes?) cannot be "installed" or "uninstalled".
Same as in (a), introduce a method getAvailableExtensionsInfo().
Option c) Choose one of (a) or (b). Then implement getAllInfo($installed = TRUE) or getInstalledExtensionsInfo() in the base class, and let call an abstract method which has the "installed" extension names.
Just saying, I still prefer
if ($this->extensionInfo === NULL)
. This lets both the IDE and the interpreter complain if we misspell the property name, or if we forget to declare it. Withisset()
, neither the IDE nor the interpreter will complain, because it has to assume that we intentionally work with undeclared properties.What about
getHumanName($extension_name)
, orgetLabel($extension_name)
?Does it?
Firstly, there should be a plural somewhere in this sentence.
Secondly, this is not the raw contents of the *.info.yml, but it is processed / modified.
The base class adds 'mtime' and $this->defaults, and the ModuleExtensionList even adds more stuff to it.
(And just to mention it: Even the ExtensionDiscovery can return extension objects with "processed" info arrays, because all the modifications leak down...)
What about
What I previously (e.g. in #150) said about profiles still applies..
We need to decide what is the intended behavior. Or try to stay as close as possible to the original behavior.
(*) Disclaimer: I still disagree with the idea to squeeze this all into one class hierarchy. But I want to be constructive, so the above comments are about the current approach.
Comment #165
almaudoh CreditAttribution: almaudoh commented#164: +1 to
getInstalledExtensionsInfo()
andgetAvailableExtensionsInfo()
or maybegetAllExtensionsInfo()
. Suggest we also renamegetInfo()
togetExtensionInfo($extension_name)
getLabel()
sounds better and conforms with existing method names.Will raise an issue for us to explore that further.
Comment #166
dawehnerTotally want with one of your suggestions!
Also, thank you for insisting on the profile detail! Now we have more test coverage
Comment #168
almaudoh CreditAttribution: almaudoh commentedInterestingly,
getExtensionInfo()
should not be affected by installation status. One should be able to get extension info for a non-installed extension and then can check<del>$info</del>$extension->status
if needed.However, I guess we're trying to maintain existing behavior
Wierd. Guess the second one should go.
Comment #169
donquixote CreditAttribution: donquixote as a volunteer commentedWell.. no :)
Extension info is just the info array, $extension->info.
The $extension->status is on the extension object, not on the info array.
To me this just proves that we are muddling too many concerns into one class hierarchy, and as a result we are getting confused.
Comment #170
donquixote CreditAttribution: donquixote as a volunteer commentedI opened #2720447: Naming convention for parameterized getter methods (->getItem($key)). for the
$extension_list->nameGetExtension($extension_name)
proposal.Comment #171
donquixote CreditAttribution: donquixote as a volunteer commentedBtw do we ever check if the extension has the correct type? E.g.
$extension->info['type'] === 'module'
.What if
modules/notamodule/notamodule.info.yml
says type === 'theme' ?Currently I think we assume the extension type based on the directory.
The type specified in the info file is redundant at best, but it may also contradict the type we expect based on the directory.
Comment #172
dawehner@donquixote
Actually the extension discovery filters out extensionsbof different types. There is an entire optimisation by not using a yml parser etc. That part totally works, but well, is also not changed as part of this issue.
Comment #173
dawehnerI reintroduced the state "cache" now, as its an optimization for cold caches.
Comment #175
XanoThis needs a code comment to explain why the block is empty, partly because the assignment can easily be mistaken for a condition in this context.
This is the other kind of static: http://php.net/manual/en/language.oop5.late-static-bindings.php (It's confusing indeed). This notation can be used as
@return static
as well and PhpStorm picks that up fine.Agreed. I really like how @dawehner implemented this!
I hadn't looked at it this way, but what you say makes total sense.
Why exactly? So we have reliable storage which can be overridden by faster caching backends? If so, good improvement, but I suggest we add some code comments to explain the rationale.
Comment #176
dawehnerWell, doesn't it say so?
Note: #2664322: key_value table is only used by a core service but it depends on system install might help with these remaining failures.
Comment #177
donquixote CreditAttribution: donquixote as a volunteer commentedNo it's not.
This method is always called with an object context. So either $this->getAllInfo(), or $list->getAllInfo().
It is not meant to be called in a static context.
Comment #178
almaudoh CreditAttribution: almaudoh commentedThis should be
$cache_id
as wellThis was a typo, I meant
$extension->status
. But I still think it's a valid point that the extension info should be available whether the extension is installed or not. The fact that one has requested info for a specific extension means he needs it and can always determine if the extension is installed or not for himself.Comment #179
donquixote CreditAttribution: donquixote as a volunteer commentedYes, I agree. This information has to come from somewhere else than getExtensionInfo(), but I still agree.
Comment #180
almaudoh CreditAttribution: almaudoh commentedIn this patch:
- Fixed some of the points raised by @donquixote in #150
- Moved the active profile additional initializations to ProfileListing::doListExtensions() which, now that I think about it is the correct place it should be.
- Also moved the make-drupal-think-a-profile-is-a-module hack to ModuleListing::doListExtensions(), so it is only added after processing. The benefit is also that the profile is not processed as a module, but rather as a profile.
- Fixed #178
Also:
I'm still convinced this method should not be restricted to only installed extensions. Even the phpDoc and exception message indicate this. I have taken the liberty of making this change. Feel free to revert if that was never the intent of this method.
Comment #182
dawehnerMh, I think this issue should try to keep the existing behaviour as much as possible, so for example the processing / alter look should still make it possible.
Do you mind open up a follow up where we can discuss that? I'm totally on our site, but you know, we cannot just fix everything at the same time.
Comment #183
almaudoh CreditAttribution: almaudoh commentedThe existing behavior is still maintained. Only that for profiles
hook_system_info_alter(array &$info, \Drupal\Core\Extension\Extension $file, $type)
is called with$type = 'profile'
instead of$type = 'module'
which I believe is a necessary result of the refactoring. We could always place that block of code before the processing, in which case that hook will be called twice for profiles.Opened #2721603: ExtensionList cleanup and separation of concerns for general discussions and actual patches for all the architectural changes, etc.
Yeah...
Comment #184
dawehnerLet's add a todo for one of the points @donquixote has
Comment #186
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedHigh-Level feedback:
- It would be great to have some docs in how the new extension system would work on a very high level.
- It would be fantastic to already have a draft CR for how things change from D7 (e.g. the mapping of old API to the new one)
Creating both should give a better idea on the overall big picture.
Code review:
This definitely should use its own special Exception type.
MissingExtensionException
I think this should be its own bug.
Should this not be injected instead?
I like this a lot, should this be injected?
Why does the order change?
I assume it is for cleanlyness of the diff to not re-indent this?
I think this is not the same.
The reset() should not be needed - except for the first request to this function.
Comment #187
dawehnerThank you fabian!
Hint, we have that actually :)
I totally agree
Well, catch fought hard here to not throw an exception, so we cannot convert it to an exception atm. Is there anything we can do about it then?
Injecting things into the installer is tricky, see #2380293: Properly inject services into ModuleInstaller but sure, we could give it a try.
Good question. Do you think this is a big deal? I guess its just the way how we organize the code, aka. let profile extend the module one.
You are right about this, but it also feels weird to keep a static around here to check that.
Comment #188
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThe change record needs some updates for the latest patch.
--
What I mean is:
catch (MissingExtensionException) {
trigger_error(...);
}
there might be other things that make this fail, which we don't want to catch, e.g. database errors.
--
The order change should be fine, but should be documented in the CR - as others might have similar tests?
--
It indeed feels weird, but I guess it is the best option and have reset() explicitly clear the 'system_rebuild_module_data' static also.
That would also keep BC for the non-API for the statics, which we tried to preserve BC in the past.
e.g. the static reset would still work - but only for code that continues to use system_rebuild_module_data().
So likely best would be to inject the drupal_static into the handler itself - if we want to keep BC there ...
Comment #189
dawehneroh gotcha, great point. Thank you!
Its a good conversation to have: Should the cleanness be preferred on the outer or inner level of the BC boundary.
Comment #191
almaudoh CreditAttribution: almaudoh commentedLet's make it explicit that we're waiting on #2664322: key_value table is only used by a core service but it depends on system install
Comment #192
dawehnerThis doesn't solve the
key_value
problem, but it solves one other issue as well as rerolls the patch.@almaudoh
Do you think you can address the points I made in #2664322: key_value table is only used by a core service but it depends on system install ?
Comment #193
larowlanThis is a cold review, didn't read the previous 192 comments, so ignore stuff that has been covered already
nit: On » In
It's a pity this is a 'per extension' alter hook and not an 'alter all the extensions in one pass' hook, like many other discovery ones are. But clearly, changing that now is an API change.
This method only includes installed extensions right? So the 'does not exist' is misleading, if we hit this exception its not installed right?
minor observation: we could use array_map here, not sure which is more efficient or if it even matters
I think we should tag this as @internal
three places but four are listed? Also doesn't include modules inside profiles, so technically there is five
same comment re profiles here
we could combine these into one if statement and avoid the elseif if we did
Nit, needs a comment for return
Could we put the call to drupal_get_profile() behind a protected method, just makes later refactoring easier.
Pity we can't move this to a class constant and deprecate the old one while we're at it.
Can this be injected? I realise there is some tight coupling here.
nice
Should we be using the new method here too?
Comment #194
almaudoh CreditAttribution: almaudoh commented#2664322: key_value table is only used by a core service but it depends on system install has been committed
Comment #195
almaudoh CreditAttribution: almaudoh commentedNow in 8.3.x
Comment #196
dawehnerThank you @larowlan for your great review!
Nope, this is about the list of available modules.
I'm all for it.
Mh, that would make the code less explicit.
Can we have its own issue for that? #2380293: Properly inject services into ModuleInstaller
I try to minimize changes here.
Comment #198
dawehnerI strongly believe #2156401: Write install_profile value to configuration and only to settings.php if it is writeable will help us with the next fixes.
Comment #202
dawehnerHere is another reroll.
Comment #204
dawehnerHere is some good progress with the issue.
* Just like HEAD we need to keep a static cache during the installer. This could allow us to drop some of the changes of
core.install.inc
Comment #206
dawehnerThere have been two remaining problems:
DrupalKernelTest
relied purely on thecontainer.modules
information. I think we should leverage that in general, as its static information which avoids reparsing on runtimesystem_module_rebuild_data()
. This interdiff fixes that by making the code more parallel to the original.Comment #207
dawehnerComment #208
jibranCan we please update the IS and mention that this change is BC.
This is a meta issue now. On which sub-issue does this depend? Do we need to create a new one?
This has been fixed.
White space.
Comment #209
dawehnerThank you @jibran for your review!
Great point! We can clean quite a bunch of ugliness up now, I guess.
Well that is not 100% sure yet. This issue is one of many steps :)
Comment #211
BerdirI'm guessing this was not on purpose ;)
I'm wondering if we want to convert some calls to drupal_get_filename() already here. The patch is already pretty big but we might be able to get rid of a bunch of @todo's.. in \Drupal\Core\Config\ExtensionInstallStorage::getAllFolders() and \Drupal\Core\Config\InstallStorage::getAllFolders() for example. Although maybe the point of the @todo is to change things completely, not just call a different method?
In the long run: https://www.usingenglish.com/forum/threads/135310-IN-or-ON-the-long-run
Wondering if that could be seen as an API change. I think not, as there is no reason to call this outside of tests and the ModuleInstaller. And if someone else uses it, I guess it is more likely that they used system_list_reset(), like config_installer.
any reason to not @deprecate those two functions here?
see #65.17 and the response in #68, not sure we can remove something just because it is not used in core.
Comment #212
dawehnerWow, how is Drupal not broken!
Yeah I think we should try to limit the scope of this issue as much as possible. These installer issues might be solveable properly ...
We argued on IRC about it, and I believe we both agree now its not.
I indeed added some additional deprecations.
Comment #214
Mile23See also: #2244917-24: Centralize extension name length validation into ExtensionDiscovery
Comment #215
jibranComment #216
Berdir23:14:48 Catchable fatal error: Argument 8 passed to Drupal\Core\Extension\ModuleExtensionList::__construct() must implement interface Drupal\Core\Config\ConfigFactoryInterface, instance of Drupal\Core\Extension\ProfileExtensionList given in /var/www/html/core/lib/Drupal/Core/Extension/ModuleExtensionList.php on line 70
This is causing the CI error
Comment #217
BerdirHm. drupal_get_filename() isn't used that much directly, but drupal_get_path() is just a slim wrapper around it and I'm seeing 350 calls to that in my installation, mostly by contrib modules.
This kind of @trigger_error() isn't free, and if we have a lot of calls to it, then that adds overhead.
Also, neither drupal_get_path() nor drupal_get_filename() are documented as deprecated at the moment. Because it isn't yet in all cases.
Should we create a follow-up of this and the theme extension list issue to convert most/all remaining usages of those two functions in core and then deprecate it with a trigger error, for all cases?
better, but pretty sure it must be long run, with space.
same here, IMHO we should at least convert the usages in core before starting to add trigger_error() warnings.
needs a message about what to use instead.
Comment #218
Wim LeersWow. Epic work.
Why do we need to move this?
(I grepped the issue, nobody asked this earlier.)
"exact location" sounds like "absolute file path" to me, but apparently a relative file path is sufficient?
This sounds edge-casey. It makes me think this is only necessary for tests.
What is a concrete use case?
So this one is similar to
$addedFileNames
, but for another edge case: installer.Why can't both edge cases use the same?
s/its/it is/
Why not just
getCacheId()
? "List" is kind of obvious/pointless, because it's already in the classname?Oh wait this is the other thing. I have no idea what this "info" is though.
Plural "filenames" vs singular "filename".
"whether" sounds strange here, I think "regardless" is a better fit?
"extension name" vs "machine name" not being used consistently here.
Builds the list of extensions.
("before caching it" is not something we usually add to these sorts of methods?)
Are defaults shared? I think they're extension type-specific? Also, this is not saving anything?
So I suggest:
Why does this only work for installed extensions? The name suggests this works for any available extension, installed or not.
Rather than this, I'd do
@see ::theDecoratingMethodThatDoesTheCaching
.Mentioning the cache ID (not cache "key"!) here explicitly causes you to need to keep them in sync in two places. Seems unnecessary.
I've never seen this pattern before.
Good :)
I almost wonder if we should have an installer-specific decorator for these classes, so that we only burden those decorators with the installer-specific needs?
This makes me wish for this to live in a decorator even more.
This says "filename", but I think it actually means "file path"?
The whole point is that you get the path to the info file (and hence the module/theme), regardless of where it may be stored on disk.
Also: "system resource" -> "extension".
Let's prefix these with dashes so that it's a list.
Also: this is missing an installation profile example.
Nit: s/drupal/Drupal/
Nit: incomplete.
Hm, the docs for
::setFilename()
said that this was only necessary in rare cases. This makes it look like it's always necessary?string[]
?Nit: unnecessary
\n
.I don't know what this means.
s/array()/[]/
NICE! :)
+1
(Although I do somewhat wonder if it's okay to still do this now that D8 has been out for so long.)
Comment #219
dawehnerThis is what my IDE is telling me as well.
I removed the trigger_errors' out of the critical path for now
Comment #221
dawehnerThan you for the review wim!
To be clear, this issue is not the end of the work here. There is more refactoring to be done and some more methods (
drupal_get_path()
/drupal_get_filename()
will be marked as deprecated in the future), but you know, we cannot solve all problems at once.Note: This does not address point #1 yet, which I will try out in the patch afterwards.
Well, the problem is that
drupal_get_filename()
used to have this feature using its static cache. We cannot just drop the feature for now, as it is a performance optimization. What we should do IMHO is to mark these explicit as internal.As explained on
\Drupal\Core\Extension\ExtensionList::setFilename
we need a static cache here, something which is kept during the installer. On the other hand for actual runtime we don't want to have an actual static variable floating around. I hope on the longrun we can at least clean up the installer, for example by having an installer kernel with extra services.Well, we have one cache ID per cache entry in this class, of which we have multiple.
I fixed the class to actually use it, which is use on
\Drupal\Core\Extension\ExtensionList::getAllAvailableInfo
Good point, it should be indeed plural.
Agreed
We are in the domain of extensions, so I believe using
extension machine name
, especially to make it explicit, is the way to go.I'm happy to drop it.
Good point, this is better indeed.
Well yeah, things are tricky. This is caused by
system_get_info()
was used before, which is explicit just about installed extensions. We could always add an additional method afterwards.Good point. These places should not care about the caching aspect of things.
We can indeed do that.
Well, its tricky. We discussed that before to have a dedicated installer kernel (also see the point above), but fixing this is non trivial and is A LOT OF effort. I hope marking things as internal allows us to make progress on the longrun.
I think I agree with you. Filename is confusing, but yeah
drupal_get_filename()
was always about that :) Do we think we should change this idea here?Well, we can actually drop it :)
Note: This is an internal function call ... and as such this is an optimization (... so we can skip using a cache get in most cases). We have all this kind of information already at our hand for modules, but also just for installed modules, while
getFilename
supports non installed extensions as well.... Note: All this technical debt exists in HEAD as well, we just make it more visible here.
Here is an example: You could argue: calling
getFilename()
is a rare thing for uninstalled modules, and as such doesn't have to cached, but well, if you'd remove that, #1081266: Avoid re-scanning module directory when a filename or a module is missing would be open again.Fixed that.
I guess there was some if around that in previous times.
Comment #222
dawehnerOh let's fix the fatal first.
Comment #223
dawehnerI'm quite sure that moving things further up should totally be possible.
Comment #224
jibranWTF :(
Should we make separate issue for this with tests?
Why do want to escape the '?
Comment #227
dawehnerNote: We have a comment about that in the issue summary ...
Heh, this was just a c&p
I just added some quick test coverage around that. Are you sure this is worth its own issue?
Berdir, webflo, wim and me talked about the naming of
::getFilename
vs.::getPathname
and we agreed on using::getPathname
as its in parallel to both\Drupal\Core\Extension\Extension
and\SplFileInfo
.Note: I also fixed the fatal and the coding style.
Comment #230
dawehnerReverting #218.1 causes the following error:
Let's keep this behaviour so. But you know, when we fix
drupal_get_filename()
we might not even need this workaround any longer.Comment #232
dawehnerLet's fix it, it was some testing invalidation issue.
Comment #233
jibranI think IS is updated now.
Comment #234
almaudoh CreditAttribution: almaudoh commentedGreat to see this issue coming back. Thanks @dawehner.
Where this may lead to ambiguity is that
drupal_get_path()
becomesExtensionList::getPath()
whiledrupal_get_filename()
becomesExtensionList::getPathname()
. I'm not familiar with the usage of "pathname" to refer to the full path of the .info.yml file. Is that what we actually mean? Because that's what that method is doing.getInstalledExtensionNames()
sounds better here.Since
getPathname()
is actually returning the full path to the .info.yml file, this should really beGets the info file path for an extension.
Comment #235
dawehnerYou are absolute right, this is a bit confusing when you come from the world of drupal. Both the
Extension
and the\SplFileInfo
class though provide those names already.Good point! I cannot agree more.
Fair, let's adapt this :)
Comment #237
dawehnerThe last patch had some other patch merged in.
Comment #239
dawehnerComment #240
alexpottI wish there were less dependencies.
out-of-scope :) and these exact words are used later.
Not sure this is necessary anymore. Since #2753733: AccountProxy can do unnecessary user loads to get an ID landed.
If we want a todo here we should file the follow up and link it here. I think just explaining why we don't reset it is okay. Or we could use a different extension list in the early installer... if we think that that is worth exploring we could file the followup.
Let's state that it's the machine name - so it's not confused with ::getName()
Let's just create s TranslatableMarkup object - less dependencies down here is always nice.
I think we should file follow-ups to do the deprecations. Doing the deprecations and triggering errors and doing the replacements in this patch will make it huge.
This can be deprecated here because all usages are accounted for.
We need to do this in a follow-up. There are way too many calls to system_rebuild_module_data() to handle in this issue.
Also, it's weird that system_rebuild_module_data() and _system_rebuild_module_data() are the same. Are we sure there is no performance impact to removing the static caching from system_rebuild_module_data()?
This has me thinking about whether we need to implement something in drupal_static_reset() to support calling the service instead. This would preserve a sort of BC. Not sure.
This is missing coverage of the reset method.
Comment #241
alexpottPatch addresses #240 - 2, 3, 5, 6, 7, 8, removes the deprecation mentioned in 9, and adds some missing test coverage.
Comment #242
alexpottsystem_rebuild_module_data() used to have a static cache... but now it calls
\Drupal::service('extension.list.module')->reset()->listExtensions()
. Are we sure this doesn't have some awful performance implications?Comment #243
alexpottStandard + simpletest install times on php 7.1:
With patch
Without patch
Seems like we have a performance regression here somewhere.
Comment #244
alexpottSo here's a blackfire comparison of the minimal install test scenario... https://blackfire.io/profiles/compare/259259f8-8213-417e-86e5-7f28d550e3...
We're calling Drupal\Core\Extension\ModuleHandler::parseDependency 11000+ more times :) - for example.
Comment #245
catchThat sounds like we need to replace some of the usages of system_rebuild_module_data() in this patch to avoid the regression - more or less anywhere that's not doing a drupal_static_reset() before the call but at least install_profile_modules() should help?
I think it's OK for system_rebuild_module_data() itself to have a performance regression though as long as the important spots in core are updated.
Comment #246
alexpottMakes no difference :( I'm guessing because installing a module forces a container rebuild. I think this all comes back to the idea that we should have an internal container that is never rebuilt and only contains a handful of services necessary for a running Drupal engine.
Yep that's it. I think we should make ExtensionList::$pathName static and not rebuild it on container rebuild. A module can not change location during a request. And if it does so be it.
Comparison to #241... https://blackfire.io/profiles/compare/17977d6e-abb9-4b91-bf2e-11844734cf... - 11,000+ less calls to
Drupal\Core\Extension\ModuleHandler::parseDependency
Here's a comparison to HEAD: https://blackfire.io/profiles/compare/df6d8410-6fc4-4b97-9f32-95434e97b9... - so here we can see the cost of the mostly the dependency container and calls to this thing that needs to be redone throughout the install because the container is being rebuilt. If we had an inner container of services that can't change on module install this could be avoided.
Comment #248
alexpottWhoops - also a proper static reset in the container - removes the performance improvement of using the static.
Comment #249
andypost@alexpott I guess you mean this issue #1530756: [meta] Use a proper kernel for the installer
Comment #250
alexpottWe reset the list twice during a module install... once via a direct call to \Drupal::service('extension.list.module')->reset(); and once via \Drupal::service('theme_handler')->refreshInfo();
Comment #251
alexpottHere's an issue to address other unnecessary rebuilds... #2872611: Optimise \Drupal\Core\Extension\ThemeHandler::refreshInfo() for the early installer
Comment #252
dawehnerThank you @alexpott for the amazing work. I feel bad that I haven't profiled it for a while.
I think for now its fine to add this static variable, but you know, we can always see whether we can actually improve it.
Do you think we need further work here?
That is helpful, thank you!
Comment #253
alexpott@dawehner there are the follow-ups we need to open for deprecation mentioned in #240 - I didn't address everything there. I noted what I did in #241.
Comment #254
larowlanQuestion: Should this be initialized to [] so we don't juggle types (at moment there's array or null)?. Would we ever need to distinguish between [] and null? I see strict checks for === NULL but would these ever be empty arrays?
Comment #255
dawehnerYes at the moment this is used to lazy fill them:
and
#240.1
Well, at least the users have less now ...
#240.4
I referencing to the system module installation issue.
#240.9
Sure, let's open it up: #2873051: Replace usages of system_rebuild_module_data with the extension.list.module service
#240.10
I remember we had an issue exactly about that I believe but I think we postponed it for concrete usecases.
#240.11
There is some test coverage for it now. I think testing reset() is a little bit tricky to be honest.
Comment #256
jibranAll the feedback is addressed let's see what maintainers think about this.
Comment #257
alexpottI think given the significance of the architecture change it needs at least one more framework manager to review this. It is big and complex. I'll ping effulgentsia and catch.
Comment #258
alexpottIs extension necessary? Tricky. Maybe this could be just
has()
?Given the above discussion -
get()
?list()? Also this vs getAllAvailableInfo() seems interesting - why do we need both?
We have a cache of all the available info - how come we're not using it - or is it important that this is fresh?
Does extension in this method name add anything?
This name is odd given the getName() method. But maybe it is okay - it seems we've settled on a convention that the human readable label is called 'name' and the machine name is 'extension name'. Not sure we can do better.
This is a funny distinction. Maybe
getPath()
could begetDirectory()
?Comment #259
Berdir8. getPath() and getPathname() are consistent with SplFileInfo as well as our existing Extension class.
Comment #260
dawehnerI totally believe that looking at the method names again could be valuable. Especially the first point though could cause an infinite discussion loop so we should be more thoughtful about that .
We had a discussion earlier in the issue, like on https://www.drupal.org/node/2208429#comment-11161611 ... It was argued that get/has doesn't give you enough context to know what it might be. I think get/has is clear, but
getInfo
maybe a bit less?We indeed don't actually need both, but I think we added
getAllInstalledInfo
to have straightforward function to be called insidesystem_get_info
.We could switch over to extension objects, when we don't use
system_get_info()
in the critical path, in which just getting the info, without the object, would be more efficient. I cannot find any instance in which we do that, so we could try to get rid of all that,and maybe keep a cache inside
system_get_info()
?Yeah this is kinda what we settled on. Its tricky, but if you have better ideas ... maybe we could have the concept of module IDs ;) Let's be honest, I think we should try to not move away too much from
\Drupal\Core\Extension\Extension
Note: This is coming from the same method name on
Extension
, see\Drupal\Core\Extension\Extension::getPath
, which is inherited from https://secure.php.net/manual/en/splfileinfo.getpath.phpComment #261
BerdirBlackfire still reports a 8% slower (55 instead of 59s) install time. A considerable amount of that is overhead due to more function calls, tracking with time is 43s instead of 41.6.
What I'm seeing is 24k more calls to Extension::getMTime(), which actually goes through __call() and creates an SplFileInfo object, as far as I see just for that piece of information. We could possibly micro-optimize that and add an actual implementation of that method. that directly uses filemtime()? And going even further, wondering if we should just deprecate "$extension->info['mtime']"? Outside of tests, I'm not seeing anything in core using that information and I'm also not exactly sure why someone would need this? But we're talking just about 100ms or so.
But anyway, the real problem is that we now store this information in a service that we rebuild after every module install as alexpott pointed out. Before it was drupal_static() which was not reset across module installs.
I'm worried this is too much of a difference to get in as-is. The problem is that the the additional calls/time grows exponentionally with larger install profiles because we both have more re-parse cycles as well as more data to parse. I think I agree with @alexpott that we should for now store this in a static property. Once it is internal and reset through the API, we can change this in the future if we e.g. have that low-level container that we do not rebuild.
Comment #262
dawehnerAn alternative for now is to provide installer only services which have all the statics, does that sound reasonable?
Comment #263
BerdirI guess that would work. Always using a static could cause troubles in tests I suppose. Likely requires quite a bit of code duplication though :(
One argument could be that the static would also help if you enable a lot of modules at once (a drush en tmgmt_demo enables 10 modules) but I think we can live with that being a bit slower.
If you agree on the mtime thing (deprecate the key in info (or possibly everything in info is deprecated already?) and make getMtime() an actual method on that oject) then I can open a follow-up for that.
Comment #265
dawehnerFor now this is just a reroll.
Comment #266
dawehnerThis change introduces a static cache during the installer, not just the early installer but throughout the entire installer process ...
I tried to profile the patch now with this better caching ... and I no longer see any problem.
Before
https://blackfire.io/profiles/0f632792-16ae-4e98-84ae-1049bf13dde0/graph
After
https://blackfire.io/profiles/8c206b1b-0fb7-4305-98a4-f883c6d1817c/graph
Compare
https://blackfire.io/profiles/compare/341d1b5c-cacb-4aaa-be0d-431ac4d9ee...
Screenshot of the comparison
https://www.drupal.org/files/issues/Screen%20Shot%202017-09-22%20at%2009...
https://www.drupal.org/files/issues/Screen%20Shot%202017-09-22%20at%2009...
@berdir It would be great if you could retest your installation profile as well
Comment #267
dawehner#258
1. No worries
2. I went with exists given that we use that in other places as well
3. Done
4. Well, one is optimized for getting the parsed info, one for the extension objects.
5. Well,
getAllAvailableInfo
depends onrecalculateInfo
, depends onlist
, depends ondoList
6. At least for now we use
extension_name
to make it clear its the machine name. ¯\_(ツ)_/¯7. Yeah see 6.
8. This is a funny distinction. Maybe getPath() could be getDirectory()? I put the answer of berdir into a comment ...
Comment #269
dawehnerI forgot to adapt the unit test after the method renames ...
Comment #270
phenaproximaThis patch is a serious happy-maker. It is so very much clearer than the current system. Really, I didn't find anything grievously wrong with it -- although I'm not versed in the dark corners of the extension system anyway, so I'm not sure I'd see dragons even if one was staring me in the face. But I would still love to see this land, because what is clear is that this is a significantly improved API.
I like this naming pattern.
Is it a BC break to return void here?
Nit: There's an extra character of white space at the end of this line.
Maybe this should import UseCacheBackendTrait, so that caching can be enabled or disabled by external code (useful for testing, perhaps)?
Is @internal needed? It's not like outside code can get to it, so it's not part of the API...
Should this be called resetCache() instead, for clarity?
If possible, we should catch a more specific exception.
This is an important detail and should therefore be in the method's doc comment.
Super useful. I love it.
Do we still need the @throws if this method does not directly throw an exception?
I believe the coding standards now dictate that we no longer put assignments inside if statements.
Nit: We could just do
$parent_profile = $this->configFactory->get('simpletest.settings')->get('parent_profile')
.This seems a bit icky. Can't we inject the extension.list.profile service and get the profile's info from there, to keep the abstraction sealed?
Needs a doc comment. Also, why is the class name prefixed with Installer? This is already in the Installer namespace.
Does marking this @internal constitute a BC break? I can't imagine it does, really, but it's worth asking.
Can this be injected?
Comment #271
dawehner1.
Yeah its all about namespacing.
2.
Site small catch!
3.
Gone
4. What about filing a follow up for that? Its absolutely an interesting idea.
5. Is @internal needed? It's not like outside code can get to it, so it's not part of the API...
Does it hurt? Its an indication that this property will not stay around ideally.
6. There are actually quite a lot of reset() functions out there, the language manager, config factory etc. IF you look at the actual code it also resets some state, so its not just about cache really at that point.
7.Good point. I used the DB exception wrapper for that
8. Good point!
9. :+1:
10. I would say yes, how else can people know about it?
11. I believe the coding standards now dictate that we no longer put assignments inside if statements.
I couldn't find any hint for that, nor I see phpcs failing, so ... let's not bikeshed, which is the entire point of phpcs
12. Nice catch!
13.
I'm happy given it a try, but I could imagine that there might be big issues we run into
14. Maybe the right answer is to move it into the extension folder?
15. Mhhh
16. Maybe, I have no idea where its actually used
Comment #273
dawehnerSorry, that's the wrong patch with the right interdiff. I didn't had the right local branch for some reason.
Comment #274
phenaproximaDiscussed with @dawhener on Slack. I think this looks good, and I'd RTBC it if I was more familiar with the extension system. But I'm not, so I'm tagging this for framework manager review in order to catch any bugs that might be lying in wait.
I don't mind RTBCing this if it passes muster with the framework managers.
Comment #275
alexpottRe-rolled since there was a conflict in system.module due to #2901562: Fix 'Squiz.WhiteSpace.SuperfluousWhitespace' coding standard. Also fixed a couple of coding standards things.
Comment #276
alexpottI'm not sure that this is not tested. Also I think potentially there is a way to not duplicate this code. Since this exact same message is emitted later in the method.
Out of scope change - and you use the exact same phrase later on in install.inc for a non-exact later. Lol I made the same point in #240.
The word exact is unnecessary and not correct as per #218
I think we fixed this in #2753733: AccountProxy can do unnecessary user loads to get an ID therefore all these changes are unnecessary and out-of-scope.
I think we should file a followup to deprecate this and remove the usages rather than deprecating here.
Need a followup to deprecate and remove usages.
Need to actually trigger the deprecation.
Comment #277
alexpottI don't think we should deprecate this function in this issue. I think we should do this in a followup.There's lots of usages. We should deprecate and swap out for the new service in a separate issue. This deprecation is causing hundreds of testing fails now we test for deprecations.
Comment #278
alexpottPatch addresses #276 and #277.
Created the following issues to handle the deprecations the require a lot of work to remove tech debt:
Comment #279
alexpottI think this patch is rtbc - all my contributions have in reviewing the issue. We've overcome the performance issues and we've created followups for the additional deprecations that are possible once this lands. The patch is a huge step towards refactoring extension listing into a maintainable service and encapsulating all the logic and statics. Great work @dawehner and all the others that have contributed to this issue.
Comment #280
almaudoh CreditAttribution: almaudoh commentedGreat to see this issue RTBC finally. Awesome work by @dawehner, @alexpott and all the other folks. All I have are docs nits.
Some consistency needed here - maybe "The machine name of the extension"
s/extension human name/machine name/
"keyed by machine name" sounds better here.
The machine name of the extension.
The machine name of the extension....
s/name/machine name/
Nit: "like module, theme and profile"
Should be
- profiles/baz/baz.info.yml
for consistency.s/getFilename/getPathname/
The drupal-root relative filename and path...
Nit: extra space.
@deprecated in Drupal 8.5.0 ....
Comment #281
almaudoh CreditAttribution: almaudoh commentedI added additional tests on PHP5.5+ because I saw a warning in my IDE. The
ExtensionList::list()
method is not allowed in PHP < 7 because it is a reserved keyword.If we want PHP5 compatibility, we have to change that method name.
Comment #282
alexpott@almaudoh great catch re #281! Definitely needs work for that and the other nits in #280.
Comment #283
alexpottI think
list()
can just begetList()
- I'll roll a new patch over the weekend unless someone beats me to it.Comment #284
jibranNight fairy stikes again.
Comment #286
jibranPHPStorm missed some renamings.
Comment #288
jibranComment #289
jibranComment #290
almaudoh CreditAttribution: almaudoh commentedA few more docs nits :)
A processed extension object for the extension with the specified machine name.
Returns a list of machine names of installed extensions.
The machine names...
I have also updated the change record to reflect the new API.
Comment #291
almaudoh CreditAttribution: almaudoh commentedI've fixed the small nits in #290
Change record has been updated. I have gone through the latest patch a second time and it looks good to go. Since all I've added to this version of the patch is three small docs nitpicks, I guess I'm allowed to re-RTBC this?
Comment #292
almaudoh CreditAttribution: almaudoh commentedThe two fails in PHP7.0-dev and PHP7.1-dev are fails in HEAD (https://www.drupal.org/pift-ci-job/819421 and https://www.drupal.org/pift-ci-job/819423)
Comment #294
oriol_e9gPer #292
Comment #295
alexpottNeeded a re-roll... git took care of it...
Comment #296
dawehnergit++
Comment #300
Anonymous (not verified) CreditAttribution: Anonymous commentedAmazing work! Back to RTBC after random failure. #2926309: Random fail due to APCu not being able to allocate memory
Comment #302
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #304
alexpottComment #306
alexpottHmmm maybe this patch is causing the memory problems :(
Comment #307
Anonymous (not verified) CreditAttribution: Anonymous commentedAt the moment, many issues have this memory problems. #2926309: Random fail due to APCu not being able to allocate memory
Comment #308
larowlanThis is looking good, couple of questions, leaving at rtbc
nit whitespace error here
should we deprecate this method in favour of using the new service directly? Follow up is ok
I assume this can't be injected because of a circular dependency?
Do we have a plan to resolve that?
A setter on the module handler that we call in the constructor of extension.list.module?
Doesn't have to be resolved here, just keen to know the plan
Can we create a follow up and add a @todo here, rather than just 'hoping' :)
Comment #309
alexpott@vaplas yes but it seems to happen more on this issue.
Comment #311
bircheractually it can be injected..
I guess it was not done because that method is littered with other things that are not injected..
That said, I have not opened up the follow-up issue and the place where the link goes is kind of empty.
Apart from that I looked over the patch but couldn't spot where the test failures would come from. then again the time right now is limited.
Comment #312
dawehnerWell, its not that easy.
First we have an issue which would be the right solution for this general injection problem: #2350111: Introduce an event to decouple the module installer from the entity manager. All this code should not be coupled that horrible to each other.
The actual reason though why this is not was commented in #187. When you install a module, you rebuild the container and after that you should really not reuse anything from the previous service container. Even this patch might be green, there could be some really hard problems hidden. I would recommend to not try this as part of this issue.
Comment #313
vijaycs85Just did a profiling with backfire locally. Between current 8.5.x HEAD vs 8.5.x HEAD + patch from 311. Comparision is available at https://blackfire.io/profiles/compare/257f3a85-6fec-47cf-9d24-356161ce26.... Seems we have bit of improvement on time(Time-14 ms (-1.8%) 757 ms → 744 ms) and memory(Memory-5.77 kB (-0.022%) 26.1 MB → 26.1 MB).
Comment #314
dawehner@vijaycs85
Thank you for your profile. This looks like a nice improvement!
Which bit did you actually profiled?
In #166I focused on the installer.
Comment #315
vijaycs85@dawehner it was just home page (on clean install) after a `drush cr`.
Comment #316
BerdirI don't think a cached frontpage call even calls the code being changed here. You can see that there are no call differences, so that's probably just random differences.
For a meaningful comparison, you would need to compare the first request after manually clearing the cache (truncate all cache tables, a drush cr or so would already populate them again). And you need to disable aggregation.
Comment #317
dawehnerLooking at the comparison: https://blackfire.io/profiles/compare/257f3a85-6fec-47cf-9d24-356161ce26...() is the main difference, using the same amount of calls. Given that this is basically a measurement of your file system / APCU cache :)
Comment #318
vijaycs85Thanks @Berdir and @dawehner for the input.
I did this (hopefully you meant blackfire aggregation) and the result seems much better now(at least from execution time POV): https://blackfire.io/profiles/compare/5b932935-ed5f-4170-9688-969b72de08...
Comment #319
dawehnerI started with #295 again and addressed the feedback in #308
✔️
Yeah, we have a follow up already.
See below.
I added the issue: #2934063: Remove the workaround in \Drupal\Core\Installer\InstallerModuleExtensionList::setPathname
Comment #320
jibrans/installation uninstallation/installation or uninstallation
> 80
Comment #321
dawehnerThank you @jibran
Comment #322
jibranThanks, @dawehner. Do we need a followup for https://www.drupal.org/files/issues/interdiff-2208429-295-311.txt?
Comment #323
dawehner@jibran
Nope :) There are issues adressing this problem already.
Comment #324
bircherYes actually the service should not be injected, the comment on the class makes that clear and also explains why all the other ones are also not injected.
Comment #325
dawehnerThank you @bircher!
Comment #328
jibranComment #329
catchOnly a partial review, but wanted to try to get a start on reviewing this again now it's RTBC. Overall I didn't notice anything major yet but I did spot these relatively minor points. Most of the things I'd complain about have follow-up issues, which is fine.
Since we only have 'profile' otherwise should we just add it here without the 'such as'?
Since these aren't static properties can we call this something else?
state service?
I feel like I'm missing something, but there's no ::getFileName() method on this class, is it getPathName()?
Also what is the situation where the cache will be cleared but the state entry won't be (except for say a memcache eviction)?
getPathName()?
Comment #330
dawehnerThank you for your review @catch!
Sure
Good point, let's remove "statically" and be done.
The state store seems to be popular in other areas in Drupal core.
Yes it is, see #227
Well yes this was the idea. Avoid disc IO as much as possible.
Good catch
Comment #331
jibranBack to RTBC as last round of feedback has been addressed.
Comment #332
catchThe comment has only been moved, but it should be updated to reference wherever this actually happens now.
Missing ().
Should this reference the fully qualified name? And remove 'static' here too?
This still says getFileName() but it should be getPathName().
I'm not sure about this, it means that it will persist past a bin flush. For example what happens if you move a module from modules/custom to modules/contrib?
Comment #333
dawehner✔️
✔️
Added the fully qualified name. I kept the static cache, because it is actually a static cache.
✔️
Well, in this case
\Drupal\Core\Extension\ExtensionList::reset
is called, which removes the state entry. Given that it is totally able to move modules.Also keep in mind, we do have
\Drupal::state()->set('system.module.files', $files);
already in core as it is. We are "just" moving it around basically. Does that makes sense?I'm assigning the issue to catch, given they seem to do a continue review at this moment in time. Everyone else is welcome to give more reviews as well :)
Comment #334
dawehner✔️
✔️
Added the fully qualified name. I kept the static cache, because it is actually a static cache.
✔️
Well, in this case
\Drupal\Core\Extension\ExtensionList::reset
is called, which removes the state entry. Given that it is totally able to move modules.Also keep in mind, we do have
\Drupal::state()->set('system.module.files', $files);
already in core as it is. We are "just" moving it around basically. Does that makes sense?I'm assigning the issue to catch, given they seem to do a continue review at this moment in time. Everyone else is welcome to give more reviews as well :)
Comment #335
catchThanks for the changes and the explanations. Moving back to RTBC.
Comment #337
catchCommitted 725641c and pushed to 8.6.x. Thanks!
Comment #338
almaudoh CreditAttribution: almaudoh commentedWow! This is great!! \o/
Comment #339
dawehner@almaudoh Thank your for all your help here!
Comment #340
Mile23I think this broke my contrib.
Try installing ajax_example from the examples project.
Here's the failing test in 8.6.x: https://www.drupal.org/pift-ci-job/870918
And the passing test in 8.5.x: https://www.drupal.org/pift-ci-job/870920
Comment #341
almaudoh CreditAttribution: almaudoh commentedI did this on simplytest.me and it installed just fine. https://d0gs.ply.st/admin/modulesEdit: I just realized that simplytest.me was installing an older version of Drupal. I'll check this out on my local machine now.
Comment #342
alexpottYep this issue introduced a breaking change to system_get_info() - see #2939904: Fix system_get_info() for non-installed modules
Comment #343
almaudoh CreditAttribution: almaudoh commentedCreated a meta issue to track all the deprecations from this issue and the
ThemeExtensionList
one: #2940190: [meta] Deprecations of old functions in Extension systemComment #344
almaudoh CreditAttribution: almaudoh commented