Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Related/duplicated issues
- #1061924: system_list() memory usage
- I possibly re-invented the wheel here, but wanted to focus on what makes sense for the new config design.
- Also, let's do #1503224: Cleanup module_list() before or after that, as it further simplifies system list handling.
- #1067408: Themes do not have an installation status
- Essentially, this patch here doesn't try to paint lipstick on a pig, but starts to revamp the theme extension state from the reversed, lowest point in the system... (ZOMG... my eyes are still bleeding from the code I had to look through)
- #625444: Override enabled module/theme list dynamically using variable override in settings.php
- config() does not load from or actively use the file system - but: it supports $conf as override.
Goal
- Separate config objects for modules and themes, since they're being used in different spots. Store the list of enabled modules / themes only as keys, the values are weights for modules, meaningless for themes. Most information comes from the modules themselves because system_list and system_get_module_info are already cached. So, there's no need to store information readily available anywhere. Store schema_version into the K-V store.
system.module.yml
:
block: '-5' [...snip...] demo: '0' [...snip...] minimal: '1000'
system.theme.yml
:
stark: '1' bartik:: '1'
Notes
- There is also some overlap with #1331486: Move module_invoke_*() and friends to an Extensions class — given that themes are handled almost identically to modules with this patch here, a generic extensions manager becomes a lot more simple + possible.
Comment | File | Size | Author |
---|---|---|---|
#258 | interdiff.txt | 1.57 KB | sun |
#257 | config.system.257.patch | 77.97 KB | sun |
#256 | config.system.256.patch | 77.45 KB | sun |
#249 | config.system.249.patch | 80.64 KB | sun |
#246 | config.system.246.patch | 80.63 KB | sun |
Comments
Comment #1
andypostDo you think that config storage could be faster then fetching from db?
This way could be very useful for sqlite\config installer...
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedholy. cow. digesting...
Comment #3
cosmicdreams CreditAttribution: cosmicdreams commentedI'd like to note the following seeming unrelated conversation I had with an Umbraco developer today.
Umbraco has concepts within their CMS that are very similar to Drupal today and some concepts that seem similar to what Drupal 8 will eventually become. They have content types, the ability to add fields to standard content types, the ability to have per-node internationalization.
What's interesting is how they persist config and data. When a content type's settings are saved, data is written to an xml file and the db. When data is read, it's read from the file. This sometimes leads to issues where the config file is out of sync from the db but those where rare edge cases and could quickly be solved by "flushing the cache" (if I had a nickle for every time I've had to do that...)
So this may be a general area of performance improvements we can make. We could improve the performance of hook_entity_info().
And at the same time, we could resolve a major gripe I often hear about Drupal for other PHP developers: That Drupal mixes config and content.
Comment #4
catchThis has been discussed on and off in irc, nice to see an issue to thrash it out.
@andypost: yeah the number #1 advantage of this is that it gets us very close to being able to install system module in the same way as installing all other modules - in other words, to the point where the module system doesn't depend on system module.
Based on the summary though, whether a module is bootstrap or not is state, not configuration. Same goes for some other things in the system table, so we need to be careful there I think. i.e. the contents of .info files are neither state nor configuration (and they can't go into config as they are now since there's an alter hook anyway).
Comment #5
sunYes, info is intentionally not put into configuration here. That's why this patch slightly duplicates the work in #1061924: system_list() memory usage, and thus we should try to get that one resolved instead first -- though it would probably be wise to take the changes from this patch into account when doing so (e.g., the most noteworthy is system_list('module_enabled') still returning an associative array whose values are arrays/objects -- not the full $record including info like now, but only the 'filename' and 'weight' properties being contained in the config).
Not sure about bootstrap modules being state vs. config. The idea of putting them into config is to allow to bootstrap with the system.module config object only, because it literally contains everything that is needed to get going (for that reason also including the filesystem paths to the modules).
Comment #6
catchWell whether a module is 'bootstrap' or not, depends on whether it currently implements a hook defined as a bootstrap hook, which is an arbitrary list compared against function_exists(), that's really state rather than configuration. The filepath is similarly purely derived information. If there's a real performance trade-off between storing them separately we'll need to look at that though agreed.
I don't see why we'd want to access 'weight' during a normal request, that's already used to order the modules in the first place, so why would we need it beyond that? The filename we only want to prime the drupal_get_filename() cache (which is something which should really be pulled into a proper extensions subsystem to remove that trickery), but again isn't needed outside of that.
Comment #7
sunMy actual reason for starting to work on this is:
#1613424: [META] Allow a site to be installed from existing configuration :)
Comment #7.0
sunUpdated issue summary.
Comment #8
alexpottDid some work on convert theme_default admin_theme and maintenance theme variables in #1712250: Convert theme settings to configuration system. I've extracted the working patch.
Comment #9
chx CreditAttribution: chx commentedThe patch in #0 didn't look hopeful and the one in #8 doesn't belong to this issue IMO. This here splits the system database table into two config arrays: system.module and system.theme (maybe system.list.module and system.list.theme?). The system.module one contains enabled and disabled but not uninstalled modules while the theme one only contains enabled themes. This required some tweaking of the simpletest.module but nothing bad.
Comment #12
chx CreditAttribution: chx commentedThose are minor issues really.
Comment #14
chx CreditAttribution: chx commentedWhy would anyone ever check whether the install profile specified in a test exists?? That makes no sense! Removing that check suddenly makes most tests pass because now the authenticated role contains access content which is rather important... I also ripped out ArrayQuery as we only use ArraySelect.
Comment #16
chx CreditAttribution: chx commentedThis fixes the bootstrap list and the help test. Should get further with it. Now I am attaching interdiffs to show that it's really miniscule fixes necessary.
Comment #17
dawehnerWouldn't it be enough to stop after the first match fails? Depending on the amount of items this might be a performance improvement.
Comment #19
chx CreditAttribution: chx commented#17, the most conditions we ever have is 2. But, yes, I implemented that. I have changed system.theme to contain all themes -- apparently this is needed -- list_theme() is all themes and it's called runtime so callling a rebuild is out of question. I also fixed module_uninstall not resetting the schema cache.
Comment #21
chx CreditAttribution: chx commentedThis one has an upgrade path, extremely early.
Comment #23
chx CreditAttribution: chx commentedThis is how far I can get. Others need to take over. It's the language upgrade
and translation UIthat fails. (Plus an action test for kicks, but that's not a biggie)Comment #25
chx CreditAttribution: chx commentedWith less debug.
Comment #27
chx CreditAttribution: chx commentedThis passes the action tests. The problem was that config was sorting on save, messing up my carefully set order.
Comment #29
catchSee comments inline:
This is one of the issues highlighted in #1175054: Add a storage (API) for persistent non-configuration state and I'm not sure how we can handle those non-config things unless that one moves forwards or they stay in the {system} table for now.
Comment #32
Gábor Hojtsy#27: 1608842_29.patch queued for re-testing.
Comment #34
chx CreditAttribution: chx commented#27: 1608842_29.patch queued for re-testing.
Comment #36
sunThanks for moving this forward.
However, we need to go back to the previous versions of this patch, because most of the additional data in {system} is state information, not configuration. I spent quite some time with analyzing each of the various system fields already.
What we essentially want and need is a simple "bootstrap" config object that only includes the configured, enabled modules (and ideally also themes), and nothing else.
Disabled and non/uninstalled extensions are irrelevant and have to be tracked somewhere else. Likewise, schema information, .info file data, and all the other stuff also needs to go somewhere else. 'cos all of that is not configuration.
@catch made a pretty good point on the list of bootstrap modules not being configuration either, although my ultimate hope is to get rid of that notion entirely for D8...
Regarding themes, I made very very good progress on #1067408: Themes do not have an installation status already, which I think should be tackled and go in first.
Comment #37
chx CreditAttribution: chx commentedThere is no "previous version". The patch in #0 is an approach littered with comments like "@todo ^^ NFIAA how to migrate that. ^^"'. It's not a working approach. Mine is a patch written from ground up (which you'll get credit of despite having absolutely nothing to do with it thanks to our rotten crediting system so don't be too flustered about me restarting) which, right now has two problems: one test fail and moving some keys which are rarely used into state. I will fix the test and I will move those keys into state. Nothing else.
Constructive reviews are welcome.
Comment #38
sunAs mentioned in #36, we are not going to convert the state information in {system} into configuration.
The list of enabled modules and themes, and arguably where they are located, is configuration. The rest is not.
Comment #39
chx CreditAttribution: chx commentedPartially correct. So what is easy: name being the key, filename, schema_version and info being state. Weight is almost surely config: right now we have no way to change that easily, after this we can just edit a config file. Now, bootstrap is actually not config because human editing breaks that stuff -- but still I am not 100% that's reason enough to move out. So, at least weight and status stays and bootstrap will reveal itself once we have the state API.
For now, I think the disabled modules stay. This might change with the state API. We will see. I know you dislike this, I can't help that. The patch works, however and that counts for something.
Comment #40
sun- schema_version is state, not configuration. Definitely has no business in the configured module list.
- filename is technically state, too, but since we need it at the very same time we need the list and because a separate lookup would be bad for performance, the idea was to make an exception there.
- bootstrap is state, but shares performance characteristics of filename. My hope is to get rid of that entirely for D8, but as long as we have it, we can try whether it is possible to put it into the module list config.
That said, putting the filename + bootstrap into config definitely breaks the fundamental idea of #1670370: Enforce writing of config changes during import, even if the module callback handled it already - the two directions are strictly incompatible.
Comment #41
moshe weitzman CreditAttribution: moshe weitzman commentedI don't know about storing weight in lots of module specific config files. Reweighting stuff is going to require a lot of reviewing and editing. It would be more convenient to have one extension => weight array. Or maybe thats simply the sort order of the system_list()? This reordering might be a case where a DB column is more convenient than config files. I guess this is not strictly on topic and could be discussed elsewhere. Curious about your thoughts.
Comment #42
chx CreditAttribution: chx commentedThis is a reroll, this is not yet state, but the code is a lot nicer and the language upgrade test seemingly passed for me. If that's the case, this is a better base for the state conversion.
Comment #44
chx CreditAttribution: chx commentedExcept that missed ArraySelect.php
Comment #46
chx CreditAttribution: chx commentedSo this is not integrated with state but it includes the current version of the state patch. I bet there's something in there which made it pass for me, sorry for the two patches above, I am new to this roll-patches-off-git-branches thing a bit and this is my first attempt.
Edit: as we go forward i will post partial no-test patches with just the work in this issue for review purposes but keep the patches being tested contain state so that they can pass.
Comment #47
chx CreditAttribution: chx commentedComment #49
chx CreditAttribution: chx commentedI am not setting this to CNR because I know there are still bugs but it's worth showing progress (maybe someone reads it). This moves everything into keyvaluestore and the config is just what's enabled and a weight. That's what everyone wanted, right? I know minimal installs.
Comment #50
catchHmm I'm not sure what we want here...
Enabled vs. disabled feels like configuration to me, so you'd probably want to have that available via config().
On the other hand, we don't want 100 disabled modules or similar loaded into memory just because we need the list of enabled ones.
Does that mean a list of enabled modules, then a separate config object with disabled modules? Doesn't feel great.
Never installed we definitely don't want in config since that's just whatever happens to be in the filesystem.
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedlets not worry about perf to start with.
if it makes sense to have enabled and disabled together in a CMI file, lets do that.
we can add a specialised cache later if we need to.
Comment #52
cweagansWe could just include enabled modules and not worry about disabled modules. If it's not in the enabled file, then it's not enabled.
Comment #53
chx CreditAttribution: chx commentedAlready config contains just enabled module name as key and weight as value. Already we have code that copies the weight to K-V on disable and back to config on enable. (While writing this comment I realized I forgot to clear config in module_disable, this is now fixed, I will post when I have more substantial fixes)
Comment #54
catch@cweagans: how do you tell between 'not enabled' and 'not installed' then?
Comment #55
chx CreditAttribution: chx commentedEasy, K-V store has schema version stored.
Comment #56
gddSo we would have
enabled - in config
disabled - not in config but in state
uninstalled - not in config or state but on file system
I mean it works, and while I agree with @beejeebus that we don't want to prematurely optimize, it does seem a bit kludgy. I don't think we're going to find a perfect answer here either though, I'd rather get something in and working and see how it goes.
That'll require some special work on import/sync/whatever-we-call-it-now as well so that deployment actually installs/disables/uninstalls modules? I would assume that is part of the plan.
An interesting side-question that came up in IRC today is, in this world, what do we do with hook_module_implements_alter()? We have been resistant to implementing alter hooks for data in CMI, as it undercuts the idea that config in files is canonical.
Still need to review the actual patch but wanted to get these thoughts down.
Comment #57
chx CreditAttribution: chx commented#56 is not correct. if the module is installed it is in key-value. if the module is also enabled it is in config. simple. ie: enabled - in config and in k-v. the latter part was missing.
Rerolled against HEAD.
Comment #58
catchhook_module_implements_alter() isn't really altering config. The module list stays the same, the list/order of hooks defined by the modules is changeable - you can have the same list of modules on two sites and the hooks be different just from one being removed or added.
Comment #59
chx CreditAttribution: chx commentedThe language upgrade and the module enable/disable test passed for me. What about the others?
Comment #61
chx CreditAttribution: chx commentedThis fixes HelpTest, Actions LoopTest, TemporaryQueryTest, InfoAlterTest. Each fix is actually about fixing the test or test module and not some outside functionality. InstallationProfileModuleTestsTest and SimpleTestTest are still failing -- if someone else wants to take them on, please. I won't return to this until Sunday I am afraid.
Comment #62
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #64
chx CreditAttribution: chx commentedInstallationProfileModuleTests doesn't fail locally so if it fails still with this version I will need to switch to scratchtestbot. SimpleTestTest is a nasty bug not caused by this patch but certainly is revealed by this patch. I filed it under #1786402: SimpleTestTest is broken. But rolled into this one as well because it shows the fix works.
Comment #65
chx CreditAttribution: chx commentedWe discussed with catch and agreed that this is hideous (note: I raised this concern) despite working. The next version will remove filename, info, owner and bootstrap from KV because these are derived and cached already mostly by system_list (and system_get_module_info). We will keep schema_version in kv in one collection and disabled module weights in another. These two absolutely must be stored as they are unavailable anywhere else. The approach looks viable and makes Drupal a bit more lean even because nuking system_get_files_database and system_update_files_database (and _system_update_bootstrap_status even) is just gone as there is nothing stored in the database (or the KV store, to be more precise) that needs to be kept in sync. The schema_version can only change on install / update. The weight of disabled modules can now be changed via an API call where there was nothing before (same API call for enabled modules -- that didn't exist before either).
So I am posting what I have right now to show where this is going. It will fail tests.
Edit: compared to the state where the state API is committed this patch now stands at a negative 3 lines of code. Almost 200 LoC has been nuked compared to the previous patch.
Comment #66
chx CreditAttribution: chx commentedAccidentally I uploaded the same patch to #65 but since I added some nice fixes, so let's see how broken this is.
Comment #68
chx CreditAttribution: chx commentedSmall fixes to fix tests. Hopefully, most of them. I know the language upgrade path one passed.
Comment #70
chx CreditAttribution: chx commentedSo many moving parts... turns out that there is a call to system_get_info() before the profile is enabled.
Comment #72
Lars Toomre CreditAttribution: Lars Toomre commentedI do not understand this line. What is $map set to if state()->get('drupal_css_cache_files') returns something? I think our coding standards require something between '?:'. Or am I wrong?
Comment #73
chx CreditAttribution: chx commentedThat comment belongs to the state issue but -- $foo ? $foo : $bar is now just $foo ?: $bar in PHP 5.3. If that needs a CS extension oh well. It wasnt possible in any previous Drupal so doubtful there's a rule.
Comment #74
Lars Toomre CreditAttribution: Lars Toomre commented@chx Thanks for the explanation. It was the first time I encountered that construct.
Comment #76
chx CreditAttribution: chx commentedBefore going to sleep, here's another. Hopefully it's a bit faster than the previous and quite a bit more correct. Some tests running system_info_alter fail, I know that, working on it.
Comment #78
chx CreditAttribution: chx commentedI fixed the contributed module update test and hopefully sped up tests by storing the searched-infoparsed info into state if we are inside a test run. I expect 88 fails and 177 exceptions.
Comment #79
chx CreditAttribution: chx commentedComment #81
chx CreditAttribution: chx commentedComment #83
chx CreditAttribution: chx commentedHere's another with less notices :)
Comment #85
chx CreditAttribution: chx commentedjust a reroll to keep up w HEAD.
Comment #87
Anonymous (not verified) CreditAttribution: Anonymous commentedupdated patch to fix the help test exceptions.
Comment #89
chx CreditAttribution: chx commentedI fixed the module API test and the multitude of notices (that was trivial).
Edit: the interdiff is correct but the patch is before the update.inc hunk applied, hence still notices.
Comment #91
chx CreditAttribution: chx commentedIf I didn't botch up this reroll then we should be down to 5 fails in the two update API tests and nothing else.
Comment #92
chx CreditAttribution: chx commentedComment #94
catchApparently we're never supposed to cache any config (i.e. the config system should do it on the callers behalf so it can also clear the right caches on updates etc.). We likely need to do that here for performance, but I'm not sure if that's 'allowed' (or how bad it'd actually be if we didn't).
system_list() didn't used to actually load modules. And while I'm a bit skeptical of 'modules providing bundles' at the moment, doesn't this mean that we now load all modules whenever the list is requested (i.e. exactly at the point where people wanted to allow bundles to work during early bootstrap)?
Why KeyValueFactory directly and not state() here?
It's a shame having a wrapper around a wrapper (around the container which is kind of a wrapper too), but yeah that's a bit verbose otherwise..
There needs to be 1. some phpdoc here 2. an explanation in the phpdoc as to why we store weight in config when modules are enabled, vs. in KeyValue when it's not. Module weight /feels/ like config to me, at least if we allow people to set it themselves then they'll probably want to deploy those changes (and possibly before actually enabling a module).
If we allow arbitrary config classes to be passed a strings, that means there can never be an automated way for the system to present configuration - it'll always need an intermediary layer over the config API itself. I wasn't sure about this feature when it was originally an argument to config() and still not sure about it at all. Would be good to get some more discussion on the implications of this.
Why can't this preparation happen before calling config()->save() instead of overriding the method like that?
I don't like letting people specify arbitrary class names here either - it means you can never reliably swap out the KeyValue storage to a different storage engine, because contrib or custom modules can force it to use the database.
Should we change module_exists() then?
We should take the t() calls out of the info hooks and handle it the same way as watchdog, hook_menu() etc.
The test should create it's own table for test purposes (and then it could be a unit test I think?). The {variable} table is also a short timer at this point so once that eventually dies the same test will need to be updated until we have no required database tables left mwahahahahahahha
It's just lovely seeing so many lines removed throughout the patch.
tsk tsk.
This is amazing. Gets us about 50% closer to the point where you no long have to actually 'install' Drupal as such.
More loveliness.
Comment #95
chx CreditAttribution: chx commented> Apparently we're never supposed to cache any config (i.e. the config system should do it on the callers behalf so it can also clear the right caches on updates etc.). We likely need to do that here for performance,
I did not change caching. I changed how the cache is populated. Reacting to a cache import with module_enable is a followup. Important followup.
> system_list() didn't used to actually load modules.
Now it does but only if the cache is cold. Then it needs to load system first and if it needs to determine which modules implement bootstrap hooks then all modules are loaded as there is no way to determine which modules implement bootstrap hooks. If the cache is warm, no loading occurs.
> Why KeyValueFactory directly and not state() here?
Because I can do ->getAll() this way (aka namespacing). And I don't want to use a blob because modifying a blob's member properties is ugly, that happened in the previous iteration (which passed btw) but it was horrific ugly.
> It's a shame having a wrapper around a wrapper (around the container which is kind of a wrapper too), but yeah that's a bit verbose otherwise..
You must realize that module_config() is a copy of config() with just an extra argument added. Otherwise, it's the same logic.
> There needs to be 1. some phpdoc here 2. an explanation in the phpdoc as to why we store weight in config when modules are enabled, vs. in KeyValue when it's not.
Because only the list of enabled modules is config.
> If we allow arbitrary config classes to be passed a strings, that means there can never be an automated way for the system to present configuration - it'll always need an intermediary layer over the config API itself.
I don't understand this at all. And, about 99% of the time noone will use that. We should document not to use that argument.
> Why can't this preparation happen before calling config()->save() instead of overriding the method like that?
We had that before; then you have something like module_config_save($config) which is also very ugly especially because some places $config can either be config() or module_config()...
> I don't like letting people specify arbitrary class names here either - it means you can never reliably swap out the KeyValue storage to a different storage engine, because contrib or custom modules can force it to use the database.
I am fine with figuring out another way to plug this in a followup. Right now this classname is supplied through drupal_container() which allows for an easy override in settings.php.
> Should we change module_exists() then?
No. field_system_info_alter is an aberration. If you want to change it, then you need a global (yuck) to signal that module rebuilding is in progress and dont examine groups.
> We should take the t() calls out of the info hooks and handle it the same way as watchdog, hook_menu() etc.
As you wish.
> The test should create it's own table for test purposes (and then it could be a unit test I think?).
We can take a look at making it a unit test. I wanted to use {filter} :)
> InfoAlterTest
We discussed and this goes away. There is nothing we can convert the asserts into as there is no info storage any more.
Comment #96
chx CreditAttribution: chx commentedI will change the variable classnames into hardwired classnames and copy the ConfigFactory into ModuleConfigFactory and register that as a separate service into the container and be done.
Comment #97
chx CreditAttribution: chx commentedI have separated sorting into a separate callable. This and the inlining of the default sorting is a Crell-approved architecture :)
Comment #99
effulgentsia CreditAttribution: effulgentsia commentedRight now, when I try to use Drupal's web installer to install a new site with #97 applied, I get a fatal error during installation (watchdog table doesn't exist, i.e., something is trying to watchdog before that table is made). Not sure why that started happening, since I was able to install successfully earlier.
Trying to troubleshoot the UpdateCoreTest::testModulePageRegularUpdate() failure. Here's a helper patch to recreate the test environment outside of simpletest. You need to adjust the fetcher url accordingly for your localhost environment. What the test is expecting is for the update check to return UPDATE_NOT_CURRENT, but what's actually happening is it's returning UPDATE_NOT_SUPPORTED. Not sure why yet. Works correctly on HEAD with this helper patch.
Need to troubleshoot more. Just reporting interim progress.
Comment #100
alexpottPatch attached fixes some issues with the config sorting. Using
call_user_func
can not pass values by reference. Also the config object is only instantiated once per request so we need to inject the sorter into the save rather than set it.This patch installers without a fatal for me.
Comment #101
effulgentsia CreditAttribution: effulgentsia commented#97 installs successfully for me when I first clean out my sites/default/files folder. Installations after that fail: I'm guessing some auto-generated config files (or permissions) left behind. Anyway, I was able to test some iterations by clearing that folder each time.
I found that with #99, in update_calculate_project_update_status(), HEAD would have $project_data['existing_major'] correctly set to '7' (given #99's entry in update_test.settings.yml), but #97 would have it at '8' (ignoring the '#all' entry?), causing the
if (in_array($existing_major, $supported_majors)) {
to not be true. I'll look into it more when I have some time, but not sure when that will be, so if someone else wants a crack at it, I hope this helps.Comment #103
chx CreditAttribution: chx commentedAh yes, that's a known: you must clean out the config directory otherwise it goes bonkers.
Comment #104
andypostIs there a reason to store initial state in some sqlite-like storage like mini-kernel or make a boostrap cernel from swappable cache contaner? This will allow more precisely control the boostrap codebase... I think the classloader + config->init() is only required amount of code.
EDIT This about #1161486-71: Move IP blocking into its own module
Comment #105
alexpottGot a local Drupal\update\Tests\UpdateContribTest pass
Comment #107
alexpottPatch fixes Drupal\update\Tests\UpdateCoreTest
Basically it appears that hook_system_info_alter is not running early enough...
Patch also contain's chx's work on removing two SQL queries per rebuild cos that was a waste... and accidentally removes all SQL dependencies of system_rebuild_module_data
Comment #108
alexpottNo idea why "Drupal\system\Tests\Ajax\FrameworkTest" failed in #105 - it passes locally.
Comment #110
chx CreditAttribution: chx commentedThe parsed info array is still static cached but it's copied fresh into ->info every time system_rebuild_module_data is called otherwise system_info_alter leaks from the parent Drupal into the child Drupal, which caused the mysterious testbot failures. Edit: this allowed me to roll revert the module_test.module changes as well. I have removed the infoalter test as discussed before.
I filed #1792092: Remove t() from hook_stream_wrappers and #1792094: React to module config import as followups.
Comment #112
chx CreditAttribution: chx commentedThat's not hard to fix at all, just need to make sure the "extend" screen doesn't display the install profile. Also, for early callers of system_rebuild_module_info we no longer provide the info because it is unaltered. This allowed me to remove the second _module_build_dependencies call which was very irritating. I reverted some debug in testing too, removed a superflous drupal_classloader_register in system_list. Green?
Comment #114
chx CreditAttribution: chx commentedSo, we do not provide ->info before DRUPAL_BOOTSTRAP_CODE because the only info we can provide is one without hook_system_info_alter. During major update this breaks. The fix for tests is to simply acknowledge this special situation and work from unaltered info. That should work because changing the required modules or the version of a module is an absolutely asinine and improbably uses of hook_system_info_alter (of course update test do it :) ).
Edit: I do believe by this making explicit we are operating on unaltered info during a major update we are better off.
Comment #114.0
chx CreditAttribution: chx commentedUpdated issue summary.
Comment #115
chx CreditAttribution: chx commentedRemoved the variable classname from the keyvaluefactory, moved the disabled module weights into config. Almost straight search and replace, hopefully doesn't hurt our beautiful green state.
Comment #116
andypostAre you sure to make k/v storage only db-centric?
maybe call it databaseKeyvalueStorage then?
Comment #117
chx CreditAttribution: chx commentedYes I am sure if you want another storage then call drupal_container() from settings.php and replace it. So the service name stays. As for calling it Database ... I dunno,, maybe, I do not feel strongly about that.
Comment #118
cosmicdreams CreditAttribution: cosmicdreams commentedand since that would be a reorganization / rename / refactor thing it shouldn't hold back this patch. That should be a follow up if desired.
Comment #119
cosmicdreams CreditAttribution: cosmicdreams commentedWe previously had a service + argument registered. Now we just have a service registered.
We COULD register the methods of that service as well.
Is the value of doing that important?
Does this mean if I were to execute module_set_weight twice on a module that has a $weight = 0, then that module would always be disabled?
What's the difference between clear and clean?
Comment #120
alexpottMade the code clearer around module_set_weight to answer concern raised in #119 and added and few comments and docblocks.
Comment #121
alexpottFurther clean up of code in module_set_weight to ensure that when writing to system.module.disabled the module has actually been enabled on the system previously.
The new functions introduced by the this patch need tests especially module_set_weight().
Comment #122
alexpottCurrently we are rebuilding the static cache twice in
system_rebuild_module_data()
on every page request.The call stack the first time:
The call stack the second time:
This is happening because the of the
drupal_static_reset('system_rebuild_module_data');
insystem_list_reset()
.Here is the call stack at the point it does the
drupal_static_reset()
:In
system_list()
we call:I don't think it makes sense that calling
system_rebuild_theme_data()
should wipe out data statically cached in the previous function call. Interestingly just reversing the calls fixes this. See attached patch.I think that the correct solution might be to fix
system_list_reset()
to clear intelligently depending on what is being rebuilt.Comment #124
chx CreditAttribution: chx commentedOh dear. #120 was running for 28 minutes, the validity of this patch will be if it's significantly faster.
Edit: I meant #121.
Comment #125
chx CreditAttribution: chx commented#1793196: Untangle the system_rebuild_$type_data rebuild mess filed.
Comment #126
chx CreditAttribution: chx commentedHrm, still 27 minutes. But of course, when rebuilding the full list, the system_list_reset removed the bootstrap key which rebuild called system_list_reset which removed the full and full cycle. I have unified the system list rebuild process and removed the superb annoying system_list_reset call from the rebuild cos that was dumb.
Comment #128
chx CreditAttribution: chx commentedThat's not good news. What about this?
Comment #129
chx CreditAttribution: chx commentedComment #131
Lars Toomre CreditAttribution: Lars Toomre commentedI recognize that this patch is still being developed. Hence, I just read through the patch at a high level to try to better understand what it was doing. While looking at things from a high level, I noticed the following items which can be addressed after the patch turns green.
I think this should be something like:
* @param bool $load
* (optional) Indicates whether to call drupal_load() for the extension. Defaults to FALSE.
Maybe 'Sorts the module data by weight before saving to config object.'
This description should start with '(optional)'.
Each of these directives need a description line as well.
For each of the assertion tests that get touched, can we remove the t() around the assertion text?
Comment #132
tim.plunkett#131, please read #1791872: [Policy, no patch] Add special tag to identify issues ready for a high level only review
I attempted to reroll this after #1215104: Use the non-interactive installer in WebTestBase::setUp().
Comment #134
chx CreditAttribution: chx commented#132 is correct... but the linked issue was committed broken. Reopened it, reuploading it with the fix applied.
Comment #136
chx CreditAttribution: chx commented#132: drupal-1608842-133.patch queued for re-testing.
Comment #137
chx CreditAttribution: chx commentedWhat about this?
$lists['bootstrap'][$name] = $filename;
the missing $name was a problem.Comment #139
chx CreditAttribution: chx commentedWell. I did fix the simpletest functionality whether I fixed 5000 exceptions I know not because I am unable to reproduce them. An example is attached of running two tests. And yes, I have used the standard profile for the parent cos last time that was the culprit. I have enabled APC specifically for the purpose to be in line with the testbot. Really wtf is going on.
Comment #140
chx CreditAttribution: chx commentedI took the opportunity of the test running so slow that live-debugging was an option. The module_load_all reset in tearDown seems to be a problem, here because it triggers a system list rebuild at such a point when it's completely unnecessary and the profile does not line up. Just a few lines down we reset config directories and that seems be the end of it.
Edit: YES! I peeked at the testbot running this test and while it won't pass the torrent of exceptions are gone.
Comment #141
chx CreditAttribution: chx commentedAh yes. The system_list_warn change is for consistency only, the real diff is
$has_run = !$bootstrap;
. Once we get this pass again, I am afraid we are facing a speed crisis.Comment #143
chx CreditAttribution: chx commentedI do not see the point in the module_load_all resets, to be honest cos they are only ran in teardowns and you can't unload anything and because it's teardown it wont load anything new. So, it's removed. Reset calls, reset capability, all. Also I have put the kibosh on infinite recursion in the language upgrade test and everything else. I wonder whether the testbot not running xdebug might cause excessive time outs due to infinite recursion, hrm. What causes the other 2+2, that's update dependent, I have no idea. Hopefully alexpott will show up and fix update API stuff again :)
Comment #145
alexpottFixed the locale test and addressed some documentation issues raised in #131
I have not changed this as this description (imho) is more useful to the developer.
Comment #146
chx CreditAttribution: chx commentedIt seems that for a few days now the 27 min is the baseline. That's what 8.x reports too on qa.drupal.org and a few other random patches I checked. Please countercheck me! xhprof-ing in #1794970: Make {system} removal faster shows that Drupal itself becomes faster and it's tests that need a little help. This patch already passed in the sister issue but it's a great opportunity to see the speedup (if it's real again).
Edit: this test run was 24 min 1 sec. Seems the speedup is real.
Comment #147
Lars Toomre CreditAttribution: Lars Toomre commentedEleven percent decrease in elapsed time is great @chx!
I just noticed that there is a small change here in one of the actions tests. That means this patch or #1008166: Actions should be a module will need to be rerolled depending on which gets committed first.
Comment #148
effulgentsia CreditAttribution: effulgentsia commentedRerolled for #147 and another HEAD change.
Comment #150
effulgentsia CreditAttribution: effulgentsia commentedMore HEAD chasing. The drop keeps moving.
Comment #152
effulgentsia CreditAttribution: effulgentsia commentedMissed a hunk in #150's reroll.
Comment #154
effulgentsia CreditAttribution: effulgentsia commentedAck. Stupid copy/paste error. This is still just a reroll of #146 with no substantive change.
Comment #155
effulgentsia CreditAttribution: effulgentsia commentedOverall, I like the direction of this patch. Some of the hunks (especially in module.inc) are a bit hard to follow, so I don't feel like I can RTBC this quite yet. I'm hoping someone else can, but if not, I'll try again when I have a chunk of time to dig into the details.
My first impression reading this was that we were disabling system.module, but what's actually happening here is we're enabling it with a weight of 0. Similarly, looking at the generated system.module.yml file, seeing a bunch of
MODULE_NAME: 0
lines doesn't tell me the 0s are weights. Any reason not to change this toset('system.weight', 0)
?Also, this patch doesn't include a default system.module.yml file (or system.theme.yml or system.module.disabled.yml) in system.module. Do we have any precedent for config objects with no shipped default file? Should we add empty ones?
This reverts #1712352-5: Configuration system does not support themes. Why?
Comment #156
RobLoachComment #158
RobLoachThanks chx and QA gods!
Comment #159
sunI glanced over this patch a couple of times in the past days/weeks already and had a range of issues/concerns with it, so I definitely want to review this once more in-depth before it lands. I'll try to do that as soon as possible.
Comment #160
RobLoachPushing to major as there are a couple issues that are blocked by this. If you could post a couple of your main concerns, we could have a quick look. Just in a hackathon right now, so our time is limited. Thanks sun!
Comment #162
sunThanks everyone for working on this. I believe this is one of the most important changes since the introduction of the configuration system.
Given a relatively deep knowledge of the current system, I spent a few hours to step through this patch, to follow call chains, to compare the new with the old behavior, and to identify intended vs. unexpected behavior. I did not, however, apply this patch, and even though I silently followed this issue and glanced over issue follow-ups since its beginning, I did not re-read the issue before doing the review. Therefore, if anything in my review duplicates information that was discussed earlier already, then that inherently means that the information was not transferred into code. ;)
The too short summary of what follows is:
This patch goes in the right direction. But there are substantial problems with the new code, especially regarding overloading of several functions, which will cause major problems down the line and which also make this patch more complex than it needs to be in various places. These issues can be resolved, but will require some serious changes to the patch in its current incarnation.
(Also, I've heard rumors that some would think that this patch would unblock work elsewhere; specifically the Extensions service issue. I do not think that this is and can be the case. While this patch touches seemingly relevant parts, it does not touch the fundamentals of the system/module/theme/hook systems. The Extensions service issue primarily needs to combat the installer and tests [which share the same condition: There is no Drupal before Drupal.], and for that, it needs to introduce a separate InstallerExtensions or FixedExtensions service that is used instead of the regular one and which doesn't attempt to query or store anything anywhere. The config storage is indeed available "earlier", but that's a false way to approach the Extensions service problem space in the installer scenario.)
Here's what I found:
Summary of changes
Questions
Problems
_system_list_warm('module', 'system', 'core/modules/system.module', TRUE);
? It's not documented, so it's either not required, or (much more likely) a really tricky workaround for a nasty race-condition that deserves a long comment explaining the situation.Comment #163
chx CreditAttribution: chx commentedWow thanks for the detailed review! Comments below.
> drupal_get_filename() still statically caches, but always scans the filesystem on a static cache miss.
Because there is noone else to get the information from. It should not ever miss btw.
>drupal_bootstrap() is changed to only bootstrap "forward".
As you can't unload PHP functions this was functionality so. But this change is actually a bugfix so that a wrongly placed drupal_bootstrap doesn't stop you from fully bootstrapping.
>_system_list_warm() is introduced to... prime the drupal_get_filename() cache (which falls back to a complete/total filesystem scan on every single cache miss), pollute the classloader, and optionally also to load an extension's main file.
Wrong. It's not new. It's merely factored out. Aside from the load, it's code that is already present twice in system_list. I am not polluting anything or not more than HEAD.
> module_enable() is changed to unconditionally scan the filesystem, regardless of whether dependencies are checked or not.
Because there is nowhere else to get any information from. Like, filenames.
> The list of bootstrap modules is no longer persistently stored anywhere. The information is stored in a cache item only and cannot be regenerated without a full system list rebuild in case the cache gets flushed.
And?
> Why does module_set_weight() work on disabled modules?
Why not?
> Is module_set_weight() supposed to be used by modules as a public API function?
Why not?
> Why is 'system.module.schema' a completely new keyvalue service and not a collection?
It's not a new service. it's a collection.
>The change to drupalGet() doesn't seem to belong into this patch at all?
It's a bugfix against url() calling drupal_alter super early. Edit: this might not be necessary any more.
> When and from where is simpletest_classloader_register() called without $module_data?
Test runners. Check api.drupal.org It's only called with module_data from the admin page OTOH.
>Why is System\InfoAlterTest removed?
Because there is no storage to assert anything. There are aplenty of tests that break if system_info_alter itself doesn't work but testing like that test does, against a storage, that doesn't fly.
>How can module_load_all() work in tests without $reset?
Easily...? That reset is fired from tearDowns and never made a lick of sense. simpletest only runs from full bootstrap and after the test it needs to be in, erm, full bootstrap.
>The changes to drupal_bootstrap() manifest a bug
Nope. It fixes a bug. Try calling theme() from hook_boot and watch Drupal not boot after that point. Or just call drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE) yourself from hook_boot. It will see the new final to DRUPAL_BOOTSTRAP_DATABASE and boom. Edit: even just calling drupal_get_bootstrap_phase before full bootstrap will wreak havoc by setting the new ceiling to 0...
>The drupal_valid_test_ua() fix should be done in #1786402: SimpleTestTest is broken first. The fix looks much better now, but it should definitely not be performed repetitevily in setUp()/prepareEnvironment(); instead, only once in ::run(), so as to prevent $originalPrefix from being changed within the same test runner environment.
This looks like a valid problem.
>The comment in dfac() explicitly states "Rebuild module and theme data", but the functional line below is replaced with a system_list_reset(). *Rebuilding* is essential at that point. The code has to call _system_list_rebuild(), not system_list_reset()
Don't you worry, _system_list_rebuild will be fired the next occassion system_list is fired. We can add a system_list() after system_list_reset if you want.
>Config::save($sort_callback) doesn't fly, at all. We need to discuss #1785560: Remove the sorting of configuration keys instead.
To the contrary: we are not holding back this patch over such minutiae. This has been refactored several times to arrive to a more or less clean solution. We can remove the default later.
>I do not understand what _system_list_rebuild($test_only = FALSE) means, and why system_list() passes TRUE. A rebuild function that does not always rebuild is an overloaded function. ...on further inspection, this argument is a PoorMansLockingFramework? The only purpose seems to be to prevent a nested invocation during rebuilds within the same request...? Why would that happen in the first place?
Because many system_info_alter hooks end up calling t which calls locale which recently has been patched to call module_exists which calls system_list which ends up in infinite recursion causing a semi-infinite amount of rerolls of this patch until I needed to ssh into a production testbot to be able figure this out . We can replace with locking yes perhaps good to do that to avoid a stampede.
>Aforementioned point makes clear that system_list() with the call to _system_list_rebuild() is overloaded with this patch -- it never ever rebuilt the list of extensions, and that was a good design and idea IMHO. Rebuilding only ever happened through system_rebuild_module_data() & co, but never through system_list(). Boils down to a simple getter/setter design. Do we really want to change that? I'm skeptical and strongly opposed. In the entirety of Drupal's history, those "fluid, overly dynamic getters" have been the primary cause for extremely nasty problems that were close to impossible to resolve. That's why this is rings neon-red alarm bells for me.
I have no idea what fluid overly dynamic getters you are talking about. system_rebuild_module_data() doesn't rebuild anything any more btw. It merely finds data. I think I already filed a followup to rename.
>Why does _system_list_rebuild() hard-code a call to
_system_list_warm('module', 'system', 'core/modules/system.module', TRUE);
Because PHP gets superb cranky (ie fatal error) if I call system_rebuild_module_data before system.module is loaded.
>And why does _system_list_rebuild() call system_list_reset() directly afterwards? A builder calling a cache reset function looks dubious. Why is that necessary? It essentially undoes the _system_list_warm() call for system.module right before it.
See above for the real reason of the warm system call.
>Unconditionally scanning the filesystem in module_enable(), AFAIK, invalidates the primary reason for why dependencies were checked only conditionally. The performance-heavy part is the filesystem scan.
Agreed. But where are you going to get filenames from to load things? I already talked to catch to make info parsing on demand in a followup.
>The schema + weight futzing in module_enable() is undocumented.
Agreed, needs a reroll.
>I'm not sure whether module_set_weight() should be a public API function. Allowing a module to set its own weight is one thing, but this essentially allows and encourages any module to change the weight of any other module?
People are dying for an API. I can't imagine how to stop other modules from using this API, however. Do you want an install time hook called hook_module_weight?
>module_config_sort() only documents internals of fancy tricks, but not what it actually does and why it does what it does.
Fair. Needs more doxygen -- it replaces the ORDER BY weight, name
>The removal/replacement of the state[.storage] service makes no sense to me. This has been raised by others off-issue as well already. If we need a separate schema service to store schema information, then let's add that instead. I do question however why that needs a separate service definition & keyvalue storage object in the first place.
Because the API cal do a ->getAll to get every schema version and OTOH does not need to manipulate a blob when setting a single schema.
>theme_disable() clears out a theme entirely, whereas the original code did not.
I see nothing valuable kept so why not?
>update_prepare_d8_bootstrap() documents that system.module wouldn't be available "in some cases" and needs to be loaded manually -- which cases?
I guess when D7 is being upgraded to D8
>update_prepare_d8_bootstrap() sorts modules by weight, but not themes. But themes are sorted by weight, too.
themes have a weight??? Since when???
>Why is block_rebuild() not using list_themes(), now that the lengthy comment explaining the problem with list_themes() is seemingly resolved?
Good question, can be fixed.
>field_system_info_alter(): Why do module_hook() and module_exists() suddenly rely on system_rebuild_module_data()? That's caused by the overloaded system_list()/_system_list_rebuild() situation, right?
No, it's caused by the fact that system_list MUST call system_rebuild_module_data to get filenames.
>The change to LocaleLookup looks bogus to me - seems like that is duplicating and working around the root cause of
It is possible that since the infinite recursion protection it's no longer necessary. It is an older fix.
>The removal of System\InfoAlterTest looks invalid to me.
I am happy to discuss how to put it back.
>UpgradePathTestBase contains a fatal error. (Config::setSorter() does not exist) This either means that the test is not executed at all, or something else is wrong with the system and testing framework changes in this patch.
Wow! Excellent catch. We can nuke uninstallModulesExcept because it is not called anywhere.
>system.theme should be a default config file/object of System module, specifying that stark should be enabled. Removing the manual setting from system_install().
Good idea.
>We should retain the SCHEMA_* constants in install.inc instead of system.module. They belong to installation functionality, not System module.
Except that system.module blows up without them now that _system_rebuild_module_data is fixed to use SCHEMA_UNINSTALLED. I think it was using a -1 before!!
>...I'm running out of steam for tonight and can't review the system.module + run-tests.sh anymore tonight. Sorry
Thanks again.
Comment #164
chx CreditAttribution: chx commentedSo, here's a list of TODOs for whoever wants to reroll this. And huge thanks to sun again for this through review!
Comment #165
chx CreditAttribution: chx commentedFor future rerollers: there's a few TODO left.
For future reviewers: please think on whether your suggestion works when there is no database and there is nothing stored anywhere but the module info files. Chances are, it doesn't.
Comment #166
chx CreditAttribution: chx commentedAnd reacting to changing the keys to modulename.weight. One, moshe suggested the current format "It would be more convenient to have one extension => weight array". Two, when hand-editing the file, you will be faced with a bunch of negatives and an 1000 at the end. Chances are, you will see those not enabled-disabled values (because they are not 0-1) but something else. Three, most module code will ever only call module_set_weight. drupal_install_system() is not a normal case.
Comment #168
chx CreditAttribution: chx commentedSurprise! Things broke. But, phew, they are not the usual impossible-to-fix problems.
Comment #170
chx CreditAttribution: chx commentedFunny, the tests that broke actually passed with #168. But few others :P
Comment #171
alexpottPatch attach fixes revert of config object documentation and adds Drupal\system\Tests\System\InfoAlterTest back in (add makes it work :) )
Comment #172
alexpottTentatively setting bact to RTBC as @chx has answered @sun's questions with code and comments. #171 does not add anything of substance - it reinstates the InfoAlterTest and fixes an unnecessary revert.
Comment #173
chx CreditAttribution: chx commented#1799300: Replace the null lock backend with a file based variant is another (possible, optional) followup addressing 165.4
Comment #174
sunThere's still the critical problem, which unfortunately got buried into the large list of problems in #162:
system_list() MUST NOT call into _system_list_rebuild().
That's a massive architectural flaw, which will bite us badly. It leads to plethora of race-conditions and infinite recursion. Proven by a range of other changes in this patch.
In fact, this patch needs to fix "bugs" and account for race/recursion conditions that only exist due to the architectural flaw that it introduces on its own, but which do not exist in HEAD.
system_list() intentionally does not call into any rebuild functions in HEAD. This is by design, for many well-engineered reasons. Rebuilding must happen in a controlled environment and on explicit request only. system_list() must be able to retrieve the effective runtime list data at any time. The data may come from a cache, but if that is not available, the data/definitions must be stored readily available, without needing a rebuild operation.
The effective data must be stored in config and/or state. Rebuilding on the fly, at arbitrary times and in arbitrary requests, is unacceptable. This architectural flaw breaks the programming 101 rule of keeping data getters and setters separate. This is even more important here, since this code runs in the critical path.
I pretty much predict that resolving this problem will also reveal that many changes in this patch go way beyond the actual conversion to CMI and could have done in a separate issue. We'd be able to make much faster progress here if there wasn't this huge amount of additional architectural changes contained in this patch (a.k.a. scope-creep).
Comment #175
chx CreditAttribution: chx commentedHumor me. _system_list_rebuild does not lead to recursions, in fact the recursion protection only became necessary when #1215104: Use the non-interactive installer in WebTestBase::setUp() happened which added a module_exist into locale. The recursion comes from system_list / system_module_rebuild_data / hook_system_info_alter / t / locale / module_exist / module_list / system_list. This chain won't change if you put system_list into state instead of cache. There is a follow up to solve #174 yes, #1793196: Untangle the system_rebuild_$type_data rebuild mess .
Edit: even if you poke the infinite recursion in the middle, ie start with system_module_rebuild_data you still need to protect from recursion somewhere. system_list can break the recursion because it can reply to the particular call in a slower way.
Comment #176
sunNo, you're introducing a recursion problem yourself in this patch.
You nicely outlined the recursion problem, and the problem is the second function. You must not call the rebuild function from system_list().
This will break badly. It will lead to countless of headaches and horrible hours of debugging for hundreds of core developers down the line.
Remove that call to _system_list_rebuild(), and you can immediately eliminate all the other hacks throughout the system in this patch that are all caused by the race/recursion condition you're introducing.
system_list() never did this. system_list() reads and has to read a defined data set, only. system_list() does NOT build or rebuild data. You're introducing a huge regression that has countless of ugly consequences.
Comment #177
webchickThis looks like this patch still needs more discussion, sorry. Would it be helpful for you guys to jump on Skype to work it out in real-time? (And update the issue w/ summary afterwards, of course.)
Comment #178
chx CreditAttribution: chx commentedAlex is working on this.
Comment #179
alexpottOkay here goes... the patch attached stops
system_rebuild_module_data()
from being called duringsystem_list()
invocation - this is not currently possible forsystem_rebuild_theme_data()
as during the first step of installation no themes are installed!Comment #181
sunThe calls to system_rebuild_*_data() are irrelevant.
To repeat #174: system_list() must not call into _system_list_rebuild().
It still does:
Comment #182
chx CreditAttribution: chx commented_system_list_rebuild is a new function which is just the code existing in system_list for rebuilding factored out into a helper for readability. Your request to not call system_module_rebuild_data was heard and adhered to.
Comment #183
alexpottContinued with trying to remove the calls to
system_rebuild_theme_data()
andsystem_rebuild_module_data()
since as chx pointed out_system_list_rebuild()
is a new function and is only ever called fromsystem_list()
.Comment #184
alexpottThis patch changes
_system_list_rebuild()
to_system_list_prepare()
to indicate that it is not a rebuild function. Also removed the (hopefully) unnecessary recursion detection.Comment #185
alexpottAfter discussing with chx on irc improved comment about why
system_rebuild_theme_data()
can still be called during a system_list().Also renamed drupal7Get() to upgradeGet().
Comment #186
chx CreditAttribution: chx commentedI think we need more than a rename, we need to actually hardwire the update.php gets.
Comment #188
chx CreditAttribution: chx commentedComment #190
chx CreditAttribution: chx commentedApparently I can't get the simplest patch right.
Edit: o_O I have no idea why it got uploaded twice, they are the same.
Comment #191
alexpottRemoving an unnecessary
system_list_reset()
as a result of talking to @sun (to give credit where its due).From @sun in skype
Comment #192
alexpottReverting changes to field.module that where preventing a recursion that now is impossible.
Comment #193
chx CreditAttribution: chx commentedThe reasons for asking for a commit is this: while it was well-founded to hold until the rebuildness in system_list is removed, this patch is a cornerstone to untangle the bootstrap-two DIC unholy mess -- katbailey is working on #1331486: Move module_invoke_*() and friends to an Extensions class by posting this patch alongside her changes. That's a gigantic mess.
We can figure out how best handle the module sorting in a followup should someone not love ->call() which is about the, i dunno, fourth different solution I presented to the problem... and is an absolute miniscule problem.
Now that field is cleaned up, I believe that the changes are related to the disappearance of {system}. The following modules change: block (simplification), simpletest (different way to find modules), system, database_test (because it used {system}), system_test (doh). Inc files: bootstrap, common (for drupal_flush_all_caches), install, install.core, module, schema, theme, update, system.admin.
The single ugliness IMO is in LocaleLookup.php, but that is definitely a much much wider problem and it only occured now because we moved install_ensure_config_directory() to earlier in update. We need to ponder that in a followup and again we need to get this patch out of the way so we can retool our bootstrap.
Comment #194
webchickThis looks like a catch-patch. :)
Comment #195
sunI'm not able to leave a longer comment. I checked the latest patch. I do not think it is ready.
- The new code lacks documentation and comments.
- It adds a significant amount of workarounds, special cases, magic arguments, and most importantly a range of undocumented assumptions, making an already complex system even harder to understand.
- The latest patch still contains obsolete workarounds for problems that earlier patches introduced but which no longer exist.
- There are stale/orphan changes, and existing code is not appropriately updated.
- There have not been sufficient architectural reviews (only catch and me it seems), which are necessary, because the proposed changes go way beyond a simple CMI conversion.
- Almost none of the review issues in #162 have been addressed.
Comment #196
pounardI 100% agree with all those three points, I was following this thread and silently reading the incoming patches. This patch seriously mess up with the module discovery, various caches, PHP file handling and such.
Comment #197
catchYeah I'd like to see more reviews on this (including me when I'm able to get to it again, which is unlikely to be before Monday at the earliest), so bumping back to CNR. I know it's taken a long time to get this working, but that's also why it need more solid reviews before it actually goes in.
Comment #198
chx CreditAttribution: chx commentedComment #199
gddComment #200
chx CreditAttribution: chx commentedWhy did you reopen it? It's crystal clear there are some who does not want this to happen. Or we could get patches or at least a list of TODOs like #164 was (which btw completely bunks that #162 was not addressed). This comment is #200, the issue is already impossibly hard.
Comment #201
chx CreditAttribution: chx commentedTo clarify: #162 was useful and I am glad for it. #195 is not useful and to #196 the only possible answer is #198.
Comment #202
chx CreditAttribution: chx commentedOr, rather, this.
Comment #203
webchickSorry, but this is not RTBC.
Comment #204
chx CreditAttribution: chx commentedI am just asking Dries to apply the same standards to my patch as they apply to WSCCI patches. We send them in buggy, hacky and slow: we are down to 30 minutes per test run from close to 20 min. sun never reviews any major patch to death. We disregard our own processes regardless of morale. We regularly add hundreds or thousands of lines of inscrutable, abstracted beyond sanity code (hey I at least document them when I find them and sometimes fix them too).
This patch is practically a net zero (adds 27 LoC net), does not introduce new WTFs for developers or core hackers, there are very few layers of abstraction and as a bonus it speeds up test runs by 10%. On what grounds, compared to the average WSCCI patch at commit time this is not committable? Actionable items please, I will fix those.
Comment #205
effulgentsia CreditAttribution: effulgentsia commentedThat's not clear to me. Maybe I missed something, but I don't see any comments rejecting the idea, just acknowledging that it's a huge undertaking with large repercussions (many of which have been addressed, some of which haven't) that's hard and/or time consuming to review, which is why there haven't been many detailed reviews yet.
In #162, sun says:
And I completely agree.
Comment #206
effulgentsia CreditAttribution: effulgentsia commentedSorry, #205 xposted with #204. Resetting issue attributes. I don't agree with them, but I don't want to be the one to make that call.
Comment #207
chx CreditAttribution: chx commentedResetting issue attributes and unfollowing the issue.
Comment #208
alexpottI've done some work on this with the following aims:
drupal_get_filename()
should use the state system to get filenamesconfig::call()
method by introducing change from #1785560: Remove the sorting of configuration keysI'm tracking on-going work in this issue using the CMI sandbox
http://drupalcode.org/sandbox/heyrocker/1145636.git/shortlog/refs/heads/...
Interdiff to #192 useless due to re-roll attaching a pseudo-interdiff of the first two aims.
Comment #210
alexpott#208 failed on Drupal\system\Tests\Entity\EntityFormTest
Entity not found in the database. Other EntityFormTest.php 66 Drupal\system\Tests\Entity\EntityFormTest->testFormCRUD()
This passes locally :(
Comment #211
alexpott#208: 1608842_208.patch queued for re-testing.
Comment #212
alexpottYay passes... one very interesting thing about this patch is that it is quick.
http://qa.drupal.org/pifr/test/354213 took 25 mins...
A standard 8.x branch at the moment takes 30 mins... http://qa.drupal.org/8.x-status (this will change)
Comment #213
pounardSomething is really bothering me with this patch, while the goal is a must have (enabled module list in config) I don't think we should get rid of the system table. Considering what Drupal already has a huge module aggregation, and is leaning toward even smaller modules today, I think we absolutely must keep an highly specified and specific API and modules/package database.
The good thing of enable modules list in a config object is that in normal runtime we can get rid of database access and such, but for administrative operations, even cache rebuild, we have absolutely no use of getting rid of this database. Even more, if the infamous info blob could be extracted and normalized into database (especially for dependencies) and if we could maintain a complete an highly coherent API for reading that, it would be such as win: for UI we need it.
I see the enabled module list a different feature of having the system table, and I don't want to see the system table being wiped out: it doesn't make any sense to me. Given the complexity of Drupal module handling, we should start with simplifying it first, and decouple system table and list from runtime enabled module list. We should also get rid of this extremely stupid PHP file handling complexity which would greatly help to serve that purpose, and make it based only upon enabled module list.
When I say removing file handling complexity, I'm of course speaking of removing all those stupid file_exists() before include statements, and this ability Drupal has of auto fixing itself when someone physically removed a file: this doesn't make any sense to keep all that at runtime, it's just extra complexity and performance problems which is actually making this thread's patch worse than it should be.
In one word, core could run without the system list in the database table as soon as the config module list is set (with module pathes) but should be kept for both update module and module enable/disable UI in order to provide a better and simpler UI code.
A lot of people may thing I'm wrong or off topic, but I think I'm not. If you want this patch to be less invasive, easier to review, and less complex, we need:
- Keep the system table and don't mess up with admin: no need to patch that yet
- Remove all system_list() coupling into the file handling and such
- Once all file handling is system_list()-indepedent, we can proceed to moving system_list and change radically its meaning: keep it only as an enabled module list handling, where all the config stuff would happen
Comment #214
sun@pounard: I thought about your proposal of keeping the system table, but my conclusion is, we don't really have a need for having the data in a query-able table. Instead, it boils down to a proper usage of state + cache storages. That's perfectly possible, but the latest patch here doesn't really achieve that yet.
Spent some quality time with this code (seriously intended to have a nicer weekend), massively scaled down the amount of changes, and I think this is the direction we should approach. The patch is non-functional.
Reverted drupal_bootstrap() hack.
Reverted module_load_all() change.
Reverted obsolete ArraySelect reference.
Reverted unnecessary changes.
Added @todo (white-space) markers.
Fixed HelpTest::getModuleList().
Massively simplified code and reverted unnecessary changes. Non-functional.
This not complete. I ran out of steam. Feel free to continue where I left, the latest code is in cmi/config-system-1608842-sun.
EDIT: Trailing white-space is intentional. Denotes suspicious code/changes.
Comment #216
cosmicdreams CreditAttribution: cosmicdreams commentedIsn't "?:" inherently broken as a result of not properly firing exceptions?
Magic value is better handled as a constant
Comment #217
Anonymous (not verified) CreditAttribution: Anonymous commentedok, i'm currently reviewing #208 and #214.
having to review them both sucks.
but, i don't see a way forward here without being across the technical differences between the two approaches, given that it doesn't seem possible to get sun and chx to work it out themselves.
Comment #218
cweagansGiven the first paragraph of 193, and pretty much all of 204, I'd like to propose that 208 is committed to unblock work elsewhere. We can open follow ups for the remaining issue(s), and it'll be easier to track the changes that way.
Comment #219
chx CreditAttribution: chx commentedetc. So you came in after we posted like 80+ patches, fought our way to green really really hard and then you reverted changes which you think are not necessary and managed to get a non functional patch? Forget it. #208 is green and needs review.
Comment #220
chx CreditAttribution: chx commentedre #217, sun posted, so far , two patches in the issue. Neither of them were functional or close to functional. We have a green one and one that speeds up testbot runs by significant amounts. When sun posted legitimate concerns alexpott and I have addressed them and I pledged to continue to do so.
Comment #221
Anonymous (not verified) CreditAttribution: Anonymous commentedspent some time looking through both patches. ouch! this is some nasty code we're messing with.
i have some sympathy with sun's review, and keeping the scope as narrow as possible. but, i don't think taking chx/alexpott's work and going backwards from there is the way to go. i think we should commit something based on #208, then work on the follow ups.
for technical and non-technical reasons, that feels like the best option. follow-ups with much, much smaller scope will be easier to assess and move forward with. everybody wants this to land, and AFAICS this is code we can keep improving right up to code freeze as long as it lands before feature freeze.
Comment #222
pounardThere is a radically different approach to get this feature in. Considering that Symfony's Container in the full stack framework is the module manager (iterates over the DIC, instanciate bundles, let them register stuff into the DIC then compiles itself and run that) I think we should probably try the same approach.
Today in actual 8.x git version, we have two containers, one for early bootstrap, and the other for full bootstrap, I think this creates numerous problems: the two major problems are: "in which one to put my service (e.g. session was a problem)?", the other is "how do I compile both?". This leads to a lot of misunderstanding and probable errors: the more we will identify "critical" services, the more we will put stuff into the early bootstrap container and invalidate the whole goal of it being early.
Where does this stand for the bootstrap list? In SF2 the system_list() equivalent is the bundles list: it stands in a configuration file. When the kernel creates the container, it iterates over bundles in order to register services, compile the DIC, which allows then in the next bootstrap to only load the hardcoded compiled container and run gracefully and very fast.
I think in our case, the module list loading coul be handled this way:
If this is implemented this way we would have:
Comment #223
cweagans@pounard, So what are you suggesting? My first interpretation is that you want major architectural changes to this patch (as in, basically start over on this issue), and my reaction to that is "wtf man, where were you 200 comments ago?"
Comment #224
pounardYes I were, but when I see how messy are getting the patches in this thread, I'm thinking that the approach is fundamentally wrong, at least it goes against the architectural choice of having Symfony behind. It could be deeply simplified before starting being messy once again, if nobody is convinced by either of the proposed patch, it's maybe because this approach deeply smells and creates numerous problems. The approach I'm suggesting, and I would understand why people might not want it, actually may solve numerous other issues.
Comment #225
catch@pounard, that sounds like a follow-up to #1331486: Move module_invoke_*() and friends to an Extensions class, I have absolutely no qualms about committing an incremental improvement here then doing loads of follow-ups but I don't think this patch is going to make anything you suggested any harder and the fact you wanted to keep the
{system}
table, 99% of which here has been dumped here in favour of an already existing cache suggests you haven't sat down with the entire workflow here.The fact that we have to recompile the container when a module is enabled is already the focus of a critical bug and makes me deeply uncomfortable at the moment. Symfony is designed for static code registration and we have dynamic code registration in Drupal via the UI to enable modules. I'm not at all sure it was a good idea to mix those two concepts (i.e. allowing modules to be registered as bundles in general) but I'm witholding judgement on some of that for now 'cos it might work out fine in the end, who knows at this point.
Just to clarify because I think there was some misunderstanding in irc on Friday. I'd like to see more reviews of the patch (thank you beejeebus), if I manage to find time to take a serious look myself then that could be one of them, but it's not required before it goes RTBC again - so if someone other than chx, sun or alexpott does a serious review of this and RTBCs it then it absolutely doesn't have to be assigned to me.
Comment #226
sunAttached patch is expected to pass tests.
Note that this is based on #208 — but the important difference is that the patch does not attempt to rewrite the entire module/theme data handling. It focuses on getting the job done, without running into scope-creep.
Working on this only revealed how many significant problems and todos were buried and undocumented previously. Some of the original changes actually presented regressions compared to current 8.x, which are not covered by tests, because they cannot be tested (especially with regard to file system changes; previously taken care of by system_update_files_database()).
This patch contains extensive @todo comments for all the things that need to be cleaned up and improved in follow-up issues.
There's also one main @todo that keeps on repeating itself: #1067408: Themes do not have an installation status (I already mentioned at the beginning of this issue that it does not make sense to approach this before resolving that dependency... but no one listened.)
Changelog:
Fixed drupal_install_system() does not store a schema version if there are no updates yet.
Cleaned up system_rebuild_module_data() and system_rebuild_theme_data().
Fixed update_prepare_d8_bootstrap() does not correctly migrate system data.
Renamed system.module.schema collection to system.schema.
Fixed unnecessary KeyValueFactory use statements.
Added 'enabled' key to system.module/.theme to avoid setData() + allow for other system critical config data.
Consistently use $module_config/$theme_config vs. $enabled_modules/$enabled_themes variable names.
Added system.module + system.theme default config files.
Renamed state.storage collection to state.
Fixed upgrade path and LanguageUpgradePathTest tests.
Fixed system_list() returns improper theme status.
Fixed theme data state is not "reset" when required.
Renamed system.bootstrap_modules state into system.module.bootstrap (consistency).
Fixed installer still registers stale state.storage service.
Fixed system.module is not necessarily loaded when called from system_list().
Reverted obsolete drupal_flush_all_caches() change; prevents Drupal from picking up file changes.
Fixed Simpletest no longer loads test classes of themes.
Fixed system.theme.files state is unreliable as long as system.theme.data exists.
Interdiff is against #214.
Comment #227
sunComment #228
cosmicdreams CreditAttribution: cosmicdreams commentedDid I read the details of that test process correctly? It finished in 23 minutes? I've been hearing that some full test processes have completed in 35 minutes. If correct this sounds like a major win.
(for the rest of the D8 dev cycle, but likely not for real-world Drupal use?)
Comment #229
pounardFor the posterity, here is a PoC of what I described upper, which: Adds a specific kernel for maintainance; Adds a specific bundle for maintainance, based upon scoping which allow gracefull dowgrade/upgrade to database/config ready environement without rebuilding the DIC (very useful and efficient); Adds a static module list in DIC thanks to the module manager (which remains static if compiled, which is loaded from database if not); Fixes some pieces of install regarding config. It works very well when installing from the UI on my box, and site behaves like a charm.
Comment #230
sun@pounard:
That prototype looks interesting (especially the idea of a maintenance mode kernel), but actually belongs into a WSCCI/framework-related issue; e.g., #1331486: Move module_invoke_*() and friends to an Extensions class (though perhaps also a new one). This issue here is about CMI, converting the list of enabled modules and themes into configuration. We should focus on the actual scope at hand, since the conversion on its own involves sufficient problems already.
Comment #231
cosmicdreams CreditAttribution: cosmicdreams commentedMy priority for tonight is Date format stuff, but I'll try to review this as well.
Comment #232
sunClarified LocaleCompareTest change; no idea how this test is able to work in HEAD.
Clarified System/InfoAlterTest and removed unnecessary resetAll() calls.
Fixed Database/TemporaryQueryTest does not use DatabaseTestBase and database_test module tables.
Fixed Database/RangeQueryTest does not use DatabaseTestBase and database_test module tables.
Fixed stale code and comments referring to "{system}" or "system table".
FWIW, also attaching an interdiff to #208.
Comment #233
BerdirRead through most of the patch. Looks quite nice to me, I felt like I could follow/understand most of the code. Dreditor somehow messed up the patch at some point and only shows new added lines, can someone reproduce that? So no notes below that.
Some minor nitpicks, comments and questions below. Nothing that relevant, so leaving at needs review.
Edit: I reviewed #227, haven't checked if the updated patch already changes any of this.
Not sure if there is a list of follow-up issues already, this is happening in #1547008: Replace Update's cache system with the (expirable) key value store/#1764474: Make Cache interface and backends use the DIC.
Is that @todo still correct? Doesn't make sense to me.
nitpick; >80 chars comment.
callers like this => what is "this"?
Unlike config, state is currently not cached in any way, every single get() results in a db_query(), even for repeated calls of the same key. This isn't a performance critical path, but it might still make sense to pre-fetch this information for all modules using a single getAll()? Possibly in other cases too when called in a loop.
There is also a deleteMultiple() for delete() in loops.
nitpick; SortS?
There might be changes that are required to be able do a basic bootstrap, I believe we have a whole list of low-level stuff that we need to fix up to be able to even do a maintenance bootstrap. Haven't checked what exactly this is doing, though.
Missing documentation :)
I assume this is needed for running a test within a test?
Posted a patch over there that should take care of this. Can also be done as a follow-up. Reference should go to TestBase::verbose(), though, that's where it's defined?
Comment #234
sun@Berdir: Thanks!
Removed unused keyvalue service from installer's pre-install container.
Removed bogus @todo from drupal_install_system().
Clarified @todo for config storage cache.
Fixed phpDoc of module_config_sort().
Fixed phpDoc of KeyValueFactory.
Fixed bogus class reference in UpgradePathTestBase.
Clarified database prefix reset in TestBase::prepareEnvironment().
Comment #235
chx CreditAttribution: chx commented#208 Test run duration: 24 min 23 sec.
#234 Test run duration: 26 min 24 sec.
Comment #236
chx CreditAttribution: chx commentedBut it's still a speedup even if not as major as #208 were and if this is what's needed to get this in then let's do it. I do not care enough.
Comment #237
chx CreditAttribution: chx commentedTo clarify: as long as we do not hardwire our bootstrap into SQL I am happy. How that happens I do not care. I was against broken solutions above but I am not against anything that passes the testbot and the community.
Comment #238
effulgentsia CreditAttribution: effulgentsia commented#234 looks good to me too, though I didn't review the changes/implications in update.inc, UpgradePathTestBase, and the whole parent test runner prefix / child test runner prefix in detail, as some of that stuff is kind of over my head to begin with.
Shame to lose this. If there's consensus on doing so, or on discussing how to restore it in a follow up, then no need to hold this patch up on it. Perhaps I missed in the above comments where this was discussed?
Comment #239
chx CreditAttribution: chx commentedThat's just #1785560: Remove the sorting of configuration keys rolled into this one to make it work.
Comment #240
chx CreditAttribution: chx commentedFolllowups, already assigned to sun because he ripped these useful additions and bugfixes out of this patch, so I can only presume he intended to add them back as a followup:
#1806980: Add a Config::__call() method
#1806984: Move module weights to the top of system.module
#1806986: Move system.module.files into their own namespace in keyvalue
#1806988: Factor out duplicate code from system_list()
#1806990: Remove the unnecessary reset from module_load_all
#1806992: drupal_get_bootstrap_phase / drupal_bootstrap is broken
Comment #241
effulgentsia CreditAttribution: effulgentsia commentedDo we also need a follow up for dropping {system_list} on existing sites?
Comment #242
chx CreditAttribution: chx commentedDo we need to drop such small amounts of data? I dont think we usually drop old tables but I am not 100%.
Comment #243
Anonymous (not verified) CreditAttribution: Anonymous commentedattached patch includes some doc-only cleanups, interdiff.txt attached.
should this todo live at config()'s definition as well? not sure.
i think this means that accessing state()->get('system.theme.data') is unsafe, and modules should always use system_list() ? accessing the system table directly, whether it was right or wrong, used to be safe. perhaps we need some doc somewhere about that?
that's system.modules.yml after install. the weight seems to be broken for user ? tracing install, we call module_enable(array('user')) twice, and things go a bit funny on the second call. investigating, and setting to CNW until that is fixed.
Comment #244
Anonymous (not verified) CreditAttribution: Anonymous commentedahem.
Comment #245
sunFixed module_enable() does not properly check for NULL weight values.
See also: #1807058: Config stores a NULL value as an array
Comment #246
sunre #241:
There's actually nothing in update.php and the upgrade process that can reasonably rely on the {system} table to exist, and all of its data is either migrated or can be regenerated at runtime, so:
Added database update to remove obsolete {system} table.
Comment #248
Anonymous (not verified) CreditAttribution: Anonymous commentedyay, #245 passed tests, and i think it's RTBC. can we do #246 as a follow up?
Comment #249
sunMerged in HEAD. (new system module updates)
However, I'd personally recommend to commit/resolve the following issues first:
#1785560: Remove the sorting of configuration keys
#1786402: SimpleTestTest is broken
#1807058: Config stores a NULL value as an array
This patch can be re-rolled/merged very easily and quickly afterwards.
Comment #250
Anonymous (not verified) CreditAttribution: Anonymous commentedok, #249 then.
Comment #251
pounardThis comment is useless: we can understand this sort algorithm without it and I personally don't care much about that kind of detail. Nevertheless the whole function could use more documentation than this simple detail because this sort algorithm is not trivial.
Comment #252
cosmicdreams CreditAttribution: cosmicdreams commentedpounard: I have to disagree with #251. I love that level of detail.
Comment #253
Fabianx CreditAttribution: Fabianx commentedAgree with #252, the comment is useful.
Comment #254
pounardA more important thing than a debate about ASCII codes of + and - would be to document the sort algorithm first, which is not done here. IMO it's important because it's a very hacky non usual sort algorithm.
Something like: Because PHP array sort functions such as uasort() cannot work with both keys and values at the same time, we are going to proceed to weight and name sorting by computing strings with with both information concatenated (weight first, name following) so that PHP will proceed to a natural sort on it then using array_multisort() would do the trick.
Comment #255
catchI committed two of the bugfixes linked from #249.
#1786402: SimpleTestTest is broken isn't RTBC yet so first come first served.
This will need a re-roll. I'd like to know why the testbot is slower if we can to make sure it's not a runtime performance issue being introduced, if necessary I'll profile before commit.
Comment #256
sunMerged in HEAD.
Note that this patch is neutral in testbot performance (or possibly ~2 minutes faster). The performance numbers mentioned earlier were a difference between the latest patch and #208, which contained a range of additional changes, in particular to simpletest and run-tests.sh. However, the change with the biggest performance impact involved testing framework specific code within regular runtime code, essentially skipping certain code paths in a test environment. These changes looked very risky and really need and deserve their own discussion to make sure we don't introduce regressions. This issue is about converting the list of modules and themes into config, not about improving testbot performance.
Comment #257
sunAdditionally incorporated @pounard's remarks/suggestion from #254:
Fixed documentation of module_config_sort().
Comment #258
sunwhoopsie, forgot interdiff, sorry for the noise. :-/
Comment #259
chx CreditAttribution: chx commentedThe regular code path has been xhprof'd some revisions ago and we found it to be faster a little on module enable, actually and although sun's patch changed a lot, that's not going to change really. If you think of it, despite all the changes, the regular code paths see no change, once the cache is warm.
Comment #260
catchOK thanks, I'd not had time to look through all of sun's changes so fine with the slightly less performance-wonderful version as long as everyone else is, great that this still shaves a decent chunk off testbot times.
Gave this one more look through, there's a few @todos in here but overall it's removing quite a lot of weird code and replacing it with nicer stuff, and the ability to deploy module status changes is going to be very nice.
Committed/pushed to 8.x!
This will need a change notice for {system} removal and module_set_weight() if nothing else.
Comment #261
podaroktagging
Comment #262
Dave ReidWondering why there is no corresponding module_get_weight() for module_set_weight(). The typical use case is to set my module's weight to one higher than another module, but I don't necessarily know what the weight of the other module is:
I don't see a way to possibly do this with the newly-converted code.
Comment #263
chx CreditAttribution: chx commentedmodule_set_weight('mymodule', config('system.module')->get('enabled.othermodule') + 1)
Comment #264
sun#1808132: Move module_set_weight() into ModuleHandler::setWeight(), add ModuleHandler::getWeight() to replace missing functionality
Comment #265
Anonymous (not verified) CreditAttribution: Anonymous commentedfollow up: #1808248: Add a separate module install/uninstall step to the config import process
Comment #266
webchickOne note for the change notice which I discovered when updating Spark 8.x to latest HEAD:
New method of enabling themes in installation profiles
In the past, installation profiles were required to perform a direct query against the system table in order to enable new themes. For example:
Drupal 7:
Now, the proper way to do this is with a call to theme_enable():
Drupal 8:
Comment #267
chx CreditAttribution: chx commentedIn the past installation profiles could get away with messing system table, now forget it.
Comment #268
webchickAlso, after installing Spark, on the last page of the installer I am getting a bunch of notices that are new since last week:
Here's the surrounding code from system_list():
The modules in question seem to install fine and are enabled when I visit the modules page, and I don't get notices there. The modules are located in profiles/spark/modules/contrib. Not sure if that has something to do with it.
Happy to spin off a follow-up issue about this if I can get a clue on what the title should be. :) Any ideas?
Comment #269
sunLet's move #268 into #1809798: Installation profile throws PHP notices since system_list() config conversion, please. ;)
Same applies to any other issues that might be caused by this - create a follow-up issue and link to it from here.
We're slowly heading towards #300 ;)
Comment #270
amontero"git blame" led me here.
Linking related issue:
#1810258: PHP notices thrown in interactive installer
Comment #271
BerdirAnother one: #1810414: Re-enable verbose() in getUpdatePhp()
Comment #272
webchickThanks to chx, who created http://drupal.org/node/1813642 and which covers the high points. If it needs further refinements, feel free to edit it and/r post a comment there asking for clarification.
Marking this fixed and moving us back under thresholds again. YEAH!
Comment #273
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #275
larowlanNew followup #1887904: update_retrieve_dependencies() only considers enabled modules
Comment #276
xjmUntagging. Please remove the "Needs change notification" tag when the change notice task is complete.
Comment #276.0
xjmrewritten issue summary