Related/duplicated issues

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

CommentFileSizeAuthor
#258 interdiff.txt1.57 KBsun
#257 config.system.257.patch77.97 KBsun
#256 config.system.256.patch77.45 KBsun
#249 config.system.249.patch80.64 KBsun
#246 config.system.246.patch80.63 KBsun
#246 interdiff.txt468 bytessun
#245 config.system.245.patch80.37 KBsun
#245 interdiff.txt3.18 KBsun
#243 config.system.243.patch78.76 KBAnonymous (not verified)
#243 intediff.txt4.61 KBAnonymous (not verified)
#234 config.system.234.patch78.71 KBsun
#234 interdiff.txt5.38 KBsun
#232 config.system.232.patch78.76 KBsun
#232 interdiff.txt15.5 KBsun
#232 interdiff.208.txt85.61 KBsun
#229 modules_install_list-2-do_not_test.patch24.92 KBpounard
#227 config.system.222.patch72.02 KBsun
#227 interdiff.txt44.45 KBsun
#214 config.system.214.patch67.47 KBsun
#214 interdiff.txt45.38 KBsun
#208 1608842_208.patch94.92 KBalexpott
#208 192-208-pseudo-interdiff.txt12.71 KBalexpott
#192 191-192-interdiff.txt1.29 KBalexpott
#192 1608842_192.patch94.49 KBalexpott
#191 190-191-interdiff.txt741 bytesalexpott
#191 1608842_191.patch95.95 KBalexpott
#190 1608842_190.patch96 KBchx
#190 1608842_190.patch96 KBchx
#188 1608842_188.patch96 KBchx
#186 interdiff-185-186.txt2.4 KBchx
#186 1608842_186.patch96 KBchx
#185 184-185-interdiff.txt2.7 KBalexpott
#185 1608842-185-rebuild.patch96.09 KBalexpott
#184 183-184-interdiff.txt1.44 KBalexpott
#184 1608842-184-rebuild.patch96.26 KBalexpott
#183 178-183-interdiff.patch2.31 KBalexpott
#183 1608842-183-rebuild-merge.patch96.45 KBalexpott
#179 171-178-interdiff.txt2.67 KBalexpott
#179 1608842-178-rebuild.patch95 KBalexpott
#171 170-171-interdiff.txt8.47 KBalexpott
#171 1608842_171.patch94.33 KBalexpott
#170 1608842_170.patch99.93 KBchx
#170 interdiff_blargh.txt384 byteschx
#168 1608842_168.patch100.11 KBchx
#168 interdiff-165-168.txt1.4 KBchx
#165 1608842_165.patch100.21 KBchx
#156 1608842.patch95.23 KBRobLoach
#154 1608842_154.patch95.44 KBeffulgentsia
#152 1608842_152.patch95.44 KBeffulgentsia
#150 1608842_150.patch94.86 KBeffulgentsia
#148 1608842_148.patch95.46 KBeffulgentsia
#146 1608842_146.patch95.46 KBchx
#146 interdiff-145-146.txt3.49 KBchx
#145 143-145-interdiff.txt4.41 KBalexpott
#145 1608842_145.patch94.29 KBalexpott
#143 1608842_143.patch93.07 KBchx
#143 interdiff-141-143.txt3.74 KBchx
#141 1608842_141.patch91.57 KBchx
#141 interdiff-140-141.txt1.06 KBchx
#140 1608842_140.patch91.77 KBchx
#140 interdiff-139-140.txt486 byteschx
#139 1608842_139.patch92.05 KBchx
#139 interdiff-137-139.txt2.31 KBchx
#139 result.txt218.48 KBchx
#137 1608842_137.patch90.7 KBchx
#137 interdiff-132-1137.patch1.58 KBchx
#134 1608842_134.patch90.71 KBchx
#134 interdiff_132_134.txt449 byteschx
#132 drupal-1608842-133.patch90.28 KBtim.plunkett
#128 1608842_128.patch90.82 KBchx
#128 interdiff-126-128.txt475 byteschx
#126 1608842_126.patch90.8 KBchx
#126 interdiff-124-126.txt10.14 KBchx
#124 interdiff-120-124.txt776 byteschx
#124 1608842_124.patch88.03 KBchx
#122 Snapshot 24:09:2012 00:59.png55.33 KBalexpott
#122 Screenshot 24:09:2012 00:38.png182.33 KBalexpott
#122 Snapshot 24:09:2012 00:36.png46.7 KBalexpott
#122 1608842_122.patch87.95 KBalexpott
#122 121-122-interdiff.txt585 bytesalexpott
#121 120-121-interdiff.txt952 bytesalexpott
#121 1608842_121.patch87.95 KBalexpott
#120 1608842_120.patch87.71 KBalexpott
#120 115-120-interdiff.txt3.46 KBalexpott
#115 1608842_115.patch86.14 KBchx
#115 interdiff.txt7.83 KBchx
#114 1608842_114.patch84.98 KBchx
#114 interdiff.txt796 byteschx
#112 1608842_112.patch84.33 KBchx
#112 interdiff.txt5.62 KBchx
#110 1608842_110.patch86.95 KBchx
#110 interdiff.txt16.46 KBchx
#107 105-107-interdiff.txt2.46 KBalexpott
#107 1608842_107.patch85.64 KBalexpott
#105 100-105-interdiff.txt974 bytesalexpott
#105 1608842_105.patch85.5 KBalexpott
#100 97-100-interdiff.txt3.97 KBalexpott
#100 1608842_100.patch85.05 KBalexpott
#99 update_test.txt1.37 KBeffulgentsia
#97 1608842_97.patch85.99 KBchx
#97 interdiff.txt12.92 KBchx
#91 1608842_91.patch86.5 KBchx
#91 interdiff.txt4.63 KBchx
#89 1608842_87.patch84.69 KBchx
#89 interdiff.txt3.34 KBchx
#87 1608842-87-cmi-modules.patch82.26 KBAnonymous (not verified)
#87 interdiff.txt800 bytesAnonymous (not verified)
#85 1608842_85.patch82.24 KBchx
#83 1608842_83.patch107.71 KBchx
#83 interdiff.txt1.07 KBchx
#81 1608842_81.patch107.28 KBchx
#78 1608842_78.patch109.17 KBchx
#78 interdiff.txt5.66 KBchx
#76 1608842_75.patch108.3 KBchx
#76 interdiff.txt11.44 KBchx
#73 1608842_73.patch98.4 KBchx
#73 interdiff.txt701 byteschx
#70 1608842_70.patch97.9 KBchx
#70 interdiff.txt692 byteschx
#68 1608842_68.patch97.8 KBchx
#68 interdiff.txt10.6 KBchx
#66 1608842_66.patch93.92 KBchx
#65 interdiff.txt28.28 KBchx
#65 1608842_65.patch86.82 KBchx
#64 1608842_64.patch86.82 KBchx
#64 interdiff.txt3.23 KBchx
#61 1608842_61.patch85.38 KBchx
#61 interdiff.txt3.23 KBchx
#59 1608842_59.patch85.13 KBchx
#57 1608842_57.patch82.72 KBchx
#49 1608842_49.patch94.62 KBchx
#49 1608842_49-without-state.patch55.63 KBchx
#46 1608842_46.patch81.62 KBchx
#44 1608842_43.patch56 KBchx
#42 1608842_42.patch53.75 KBchx
#27 1608842_29.patch56.9 KBchx
#27 interdiff.txt2.63 KBchx
#25 1608842_27.patch55.18 KBchx
#23 1608842_23.patch55.49 KBchx
#21 1608842_21.patch53.9 KBchx
#21 interdiff.txt3.81 KBchx
#19 1608842_19.patch50.22 KBchx
#19 interdiff.txt4.93 KBchx
#16 interdiff.txt1.7 KBchx
#16 1608842_16.patch50.47 KBchx
#14 1608842_14.patch50.5 KBchx
#12 1608842_12.patch50.87 KBchx
#9 1608842_9.patch51.43 KBchx
#8 1608842-8.drupal8.theme_default_config.patch29.84 KBalexpott
config.modules.0.patch22.42 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Do you think that config storage could be faster then fetching from db?
This way could be very useful for sqlite\config installer...

Anonymous’s picture

holy. cow. digesting...

cosmicdreams’s picture

I'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.

catch’s picture

This 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).

sun’s picture

Yes, 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).

catch’s picture

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

sun’s picture

My actual reason for starting to work on this is:
#1613424: [META] Allow a site to be installed from existing configuration :)

sun’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Status: Needs work » Needs review
FileSize
29.84 KB

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

chx’s picture

FileSize
51.43 KB

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

Status: Needs review » Needs work
Issue tags: -Configuration system

The last submitted patch, 1608842_9.patch, failed testing.

Issue tags: +Configuration system

The last submitted patch, 1608842_9.patch, failed testing.

chx’s picture

Assigned: sun » chx
Status: Needs work » Needs review
FileSize
50.87 KB

Those are minor issues really.

Status: Needs review » Needs work

The last submitted patch, 1608842_12.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
50.5 KB

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

Status: Needs review » Needs work

The last submitted patch, 1608842_14.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
50.47 KB
1.7 KB

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

dawehner’s picture

+++ b/core/lib/Drupal/Component/ArraySelect.phpundefined
@@ -0,0 +1,88 @@
+      $match = TRUE;
+      foreach ($this->conditions as $condition) {
+        $match = $match && $this->match($element, $condition);
+      }

Wouldn't it be enough to stop after the first match fails? Depending on the amount of items this might be a performance improvement.

Status: Needs review » Needs work

The last submitted patch, 1608842_16.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
4.93 KB
50.22 KB

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

Status: Needs review » Needs work

The last submitted patch, 1608842_19.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.81 KB
53.9 KB

This one has an upgrade path, extremely early.

Status: Needs review » Needs work

The last submitted patch, 1608842_21.patch, failed testing.

chx’s picture

Assigned: chx » Unassigned
Status: Needs work » Needs review
FileSize
55.49 KB

This is how far I can get. Others need to take over. It's the language upgrade and translation UI that fails. (Plus an action test for kicks, but that's not a biggie)

Status: Needs review » Needs work

The last submitted patch, 1608842_23.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
55.18 KB

With less debug.

Status: Needs review » Needs work

The last submitted patch, 1608842_27.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
56.9 KB

This passes the action tests. The problem was that config was sorting on save, messing up my carefully set order.

Status: Needs review » Needs work

The last submitted patch, 1608842_29.patch, failed testing.

catch’s picture

See comments inline:

+    if (!$module_config) {
+      $enabled = FALSE;
+      $file = $module_data[$module];
+      $module_config = array(
+        'filename' => $file->uri, // This isn't config.
+        'name' => $file->name, // Nor this, although it might be the namespace of the config itself, why store it twice?
+        'status' => 1,
+        'schema_version' => SCHEMA_UNINSTALLED, // Also not config.
+        'bootstrap' => 0, // Nor this.
+        'info' => $file->info, // This neither.
+        'weight' => 0, // Arguable.
+      );
+      $config->set($module, $module_config);
+    }

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Configuration system

#27: 1608842_29.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1608842_29.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

#27: 1608842_29.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Configuration system

The last submitted patch, 1608842_29.patch, failed testing.

sun’s picture

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

chx’s picture

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

sun’s picture

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

chx’s picture

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

sun’s picture

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

moshe weitzman’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
53.75 KB

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

Status: Needs review » Needs work

The last submitted patch, 1608842_42.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
56 KB

Except that missed ArraySelect.php

Status: Needs review » Needs work

The last submitted patch, 1608842_43.patch, failed testing.

chx’s picture

FileSize
81.62 KB

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

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1608842_46.patch, failed testing.

chx’s picture

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

catch’s picture

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

Anonymous’s picture

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

cweagans’s picture

We could just include enabled modules and not worry about disabled modules. If it's not in the enabled file, then it's not enabled.

chx’s picture

Already 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)

catch’s picture

@cweagans: how do you tell between 'not enabled' and 'not installed' then?

chx’s picture

Easy, K-V store has schema version stored.

gdd’s picture

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

chx’s picture

FileSize
82.72 KB

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

catch’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
85.13 KB

The language upgrade and the module enable/disable test passed for me. What about the others?

Status: Needs review » Needs work

The last submitted patch, 1608842_59.patch, failed testing.

chx’s picture

FileSize
3.23 KB
85.38 KB

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

Tor Arne Thune’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1608842_61.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
86.82 KB

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

chx’s picture

FileSize
86.82 KB
28.28 KB

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

chx’s picture

FileSize
93.92 KB

Accidentally I uploaded the same patch to #65 but since I added some nice fixes, so let's see how broken this is.

Status: Needs review » Needs work

The last submitted patch, 1608842_66.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
10.6 KB
97.8 KB

Small fixes to fix tests. Hopefully, most of them. I know the language upgrade path one passed.

Status: Needs review » Needs work

The last submitted patch, 1608842_68.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
692 bytes
97.9 KB

So many moving parts... turns out that there is a call to system_get_info() before the profile is enabled.

Status: Needs review » Needs work

The last submitted patch, 1608842_70.patch, failed testing.

Lars Toomre’s picture

+++ b/core/includes/common.incundefined
@@ -3176,7 +3176,7 @@ function drupal_pre_render_styles($elements) {
+  $map = state()->get('drupal_css_cache_files') ?: array();

I 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?

chx’s picture

Status: Needs work » Needs review
FileSize
701 bytes
98.4 KB

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

Lars Toomre’s picture

@chx Thanks for the explanation. It was the first time I encountered that construct.

Status: Needs review » Needs work

The last submitted patch, 1608842_73.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
11.44 KB
108.3 KB

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

Status: Needs review » Needs work

The last submitted patch, 1608842_75.patch, failed testing.

chx’s picture

FileSize
5.66 KB
109.17 KB

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

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1608842_78.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
107.28 KB

Status: Needs review » Needs work

The last submitted patch, 1608842_81.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
107.71 KB

Here's another with less notices :)

Status: Needs review » Needs work

The last submitted patch, 1608842_83.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
82.24 KB

just a reroll to keep up w HEAD.

Status: Needs review » Needs work

The last submitted patch, 1608842_85.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
800 bytes
82.26 KB

updated patch to fix the help test exceptions.

Status: Needs review » Needs work

The last submitted patch, 1608842-87-cmi-modules.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.34 KB
84.69 KB

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

Status: Needs review » Needs work

The last submitted patch, 1608842_87.patch, failed testing.

chx’s picture

FileSize
4.63 KB
86.5 KB

If I didn't botch up this reroll then we should be down to 5 fails in the two update API tests and nothing else.

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1608842_91.patch, failed testing.

catch’s picture

+++ b/core/includes/module.incundefined
@@ -199,19 +213,37 @@ function system_list($type) {
+          'filepath' => $module_data[$name]->filename,
+        );
+      }
       cache('bootstrap')->set('system_list', $lists);

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, but I'm not sure if that's 'allowed' (or how bad it'd actually be if we didn't).

+++ b/core/includes/module.incundefined
@@ -199,19 +213,37 @@ function system_list($type) {
+function _system_list_warm($type, $name, $filename, $load = FALSE) {
+  drupal_classloader_register($name, dirname($filename));
+  drupal_get_filename($type, $name, $filename);
+  if ($load) {
+    drupal_load($type, $name);

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)?

+++ b/core/includes/module.incundefined
@@ -417,30 +450,34 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+  $schema_store = KeyValueFactory::get('system.module.schema');

Why KeyValueFactory directly and not state() here?

+++ b/core/includes/module.incundefined
@@ -1102,3 +1145,25 @@ function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+/**
+ * Retrieves the module list configuration object.
+ *
+ * @return Drupal\Core\Config\ModuleConfig
+ *   A configuration object.
+ */
+function module_config() {
+  return drupal_container()->get('config.factory')->get('system.module', 'Drupal\Core\Config\ModuleConfig')->load();

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

+++ b/core/includes/module.incundefined
@@ -1102,3 +1145,25 @@ function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+function module_set_weight($module, $weight) {
+  $config = module_config();
+  if ($config->get($module) !== NULL) {
+    $config
+      ->set($module, $weight)
+      ->save();
+  }
+  else {
+    KeyValueFactory::get('system.module.weight')->set($module, $weight);

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

+++ b/core/lib/Drupal/Core/Config/ConfigFactory.phpundefined
@@ -57,11 +57,13 @@ class ConfigFactory {
-  public function get($name) {
+  public function get($name, $class_name = 'Drupal\Core\Config\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 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.

+++ b/core/lib/Drupal/Core/Config/ModuleConfig.phpundefined
@@ -0,0 +1,34 @@
+/**
+ * Defines the module configuration object.
+ */
+class ModuleConfig extends Config {
+
+  public function save() {
+    if ($this->data) {
+      $sort = array();
+      foreach ($this->data as $name => $weight) {
+        // We can't use the sign directly because + (ASCII 43) is before
+        // - (ASCII 45). So negative nubmers get a 0, non-negative numbers
+        // a 1 prefix.
+        $prefix = (int) ($weight >= 0);
+        // PHP_INT_MAX is at most 19 characters so make every number equally
+        // 19 digits long.
+        $sort[] = $prefix . sprintf('%019d', abs($weight)) . $name;
+      }
+      array_multisort($sort, SORT_STRING, $this->data);
+    }
+    $this->storage->write($this->name, $this->data);
+    $this->isNew = FALSE;
+    $this->notify('save');
+    return $this;

Why can't this preparation happen before calling config()->save() instead of overriding the method like that?

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.phpundefined
@@ -0,0 +1,18 @@
+namespace Drupal\Core\KeyValueStore;
+
+class KeyValueFactory {
+
+  /**
+   * @param string $collection
+   * @param string $class_name
+   * @return Drupal\Core\KeyValueStore\KeyValueStoreInterface
+   */
+  static function get($collection, $class_name = 'Drupal\Core\KeyValueStore\DatabaseStorage') {
+    return new $class_name($collection);

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.

+++ b/core/modules/field/field.moduleundefined
@@ -354,7 +363,8 @@ function field_system_info_alter(&$info, $file, $type) {
+        // Calling module_exists() here leads to infinite recursion.

Should we change module_exists() then?

+++ b/core/modules/locale/lib/Drupal/locale/LocaleLookup.phpundefined
@@ -36,7 +36,16 @@ class LocaleLookup extends CacheArray {
+      // stream wrappers which calls locale_stream_wrappers() which has t()

We should take the t() calls out of the info hooks and handle it the same way as watchdog, hook_menu() etc.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/RangeQueryTest.phpundefined
@@ -34,11 +34,11 @@ class RangeQueryTest extends WebTestBase {
   function testRangeQuery() {
     // Test if return correct number of rows.
-    $range_rows = db_query_range("SELECT name FROM {system} ORDER BY name", 2, 3)->fetchAll();
+    $range_rows = db_query_range("SELECT name FROM {variable} ORDER BY name", 2, 3)->fetchAll();
     $this->assertEqual(count($range_rows), 3, t('Range query work and return correct number of rows.'));
 
     // Test if return target data.
-    $raw_rows = db_query('SELECT name FROM {system} ORDER BY name')->fetchAll();
+    $raw_rows = db_query('SELECT name FROM {variable} ORDER BY name')->fetchAll();
     $raw_rows = array_slice($raw_rows, 2, 3);
     $this->assertEqual($range_rows, $raw_rows, t('Range query work and return target data.'));

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

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.phpundefined
@@ -49,11 +49,7 @@ class ModuleApiTest extends WebTestBase {
-    db_update('system')
-      ->fields(array('weight' => 20))
-      ->condition('name', 'contact')
-      ->condition('type', 'module')
-      ->execute();

It's just lovely seeing so many lines removed throughout the patch.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/InfoAlterTest.phpundefined
@@ -8,12 +8,13 @@
 class InfoAlterTest extends WebTestBase {
-  public static function getInfo() {

tsk tsk.

+++ b/core/modules/system/system.installundefined
@@ -1392,75 +1388,6 @@ function system_schema() {
 

This is amazing. Gets us about 50% closer to the point where you no long have to actually 'install' Drupal as such.

+++ b/core/modules/system/system.moduleundefined
@@ -2600,116 +2610,6 @@ function system_check_directory($form_element) {
-function system_get_files_database(&$files, $type) {
-  // Extract current files from database.
-  $result = db_query("SELECT filename, name, type, status, schema_version, weight FROM {system} WHERE type = :type", array(':type' => $type));

More loveliness.

chx’s picture

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

chx’s picture

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

chx’s picture

Status: Needs work » Needs review
FileSize
12.92 KB
85.99 KB

I have separated sorting into a separate callable. This and the inlining of the default sorting is a Crell-approved architecture :)

Status: Needs review » Needs work

The last submitted patch, 1608842_97.patch, failed testing.

effulgentsia’s picture

FileSize
1.37 KB

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
85.05 KB
3.97 KB

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

effulgentsia’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1608842_100.patch, failed testing.

chx’s picture

Ah yes, that's a known: you must clean out the config directory otherwise it goes bonkers.

andypost’s picture

Is 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

alexpott’s picture

Status: Needs work » Needs review
FileSize
85.5 KB
974 bytes

Got a local Drupal\update\Tests\UpdateContribTest pass

Status: Needs review » Needs work

The last submitted patch, 1608842_105.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
85.64 KB
2.46 KB

Patch 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

alexpott’s picture

No idea why "Drupal\system\Tests\Ajax\FrameworkTest" failed in #105 - it passes locally.

Status: Needs review » Needs work

The last submitted patch, 1608842_107.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
16.46 KB
86.95 KB

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

Status: Needs review » Needs work

The last submitted patch, 1608842_110.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.62 KB
84.33 KB

That'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?

Status: Needs review » Needs work

The last submitted patch, 1608842_112.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
796 bytes
84.98 KB

So, 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.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

FileSize
7.83 KB
86.14 KB

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

andypost’s picture

Are you sure to make k/v storage only db-centric?

diff -u b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.php b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.php
--- b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.php
@@ -13,6 +13,6 @@
    * @return Drupal\Core\KeyValueStore\KeyValueStoreInterface
    */
-  static function get($collection, $class_name = 'Drupal\Core\KeyValueStore\DatabaseStorage') {
-    return new $class_name($collection);
+  static function get($collection) {
+    return new DatabaseStorage($collection);

maybe call it databaseKeyvalueStorage then?

chx’s picture

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

cosmicdreams’s picture

and since that would be a reorganization / rename / refactor thing it shouldn't hold back this patch. That should be a follow up if desired.

cosmicdreams’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.incundefined
@@ -2484,8 +2484,7 @@
-      ->register('state.storage', 'Drupal\Core\KeyValueStore\DatabaseStorage')
-      ->addArgument('state');

We 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?

+++ b/core/includes/module.incundefined
@@ -1147,13 +1152,21 @@
+  elseif ($weight) {
+    $config_disabled
+      ->set($module, $weight)
+      ->save();
+  }
+  elseif ($config_disabled->get($module)) {
+    $config_disabled
+      ->clean($module)
+      ->save();

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?

alexpott’s picture

Status: Needs work » Needs review
FileSize
3.46 KB
87.71 KB

Made the code clearer around module_set_weight to answer concern raised in #119 and added and few comments and docblocks.

alexpott’s picture

Issue tags: +Needs tests
FileSize
87.95 KB
952 bytes

Further 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().

alexpott’s picture

Currently we are rebuilding the static cache twice in system_rebuild_module_data() on every page request.

The call stack the first time:
36.png

The call stack the second time:
38.png

This is happening because the of the drupal_static_reset('system_rebuild_module_data'); in system_list_reset().
Here is the call stack at the point it does the drupal_static_reset():
59.png

In system_list() we call:

      $module_data = system_rebuild_module_data();
      $theme_data = system_rebuild_theme_data();

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.

Status: Needs review » Needs work

The last submitted patch, 1608842_122.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
88.03 KB
776 bytes

Oh dear. #120 was running for 28 minutes, the validity of this patch will be if it's significantly faster.

Edit: I meant #121.

chx’s picture

chx’s picture

FileSize
10.14 KB
90.8 KB

Hrm, 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.

Status: Needs review » Needs work

The last submitted patch, 1608842_126.patch, failed testing.

chx’s picture

FileSize
475 bytes
90.82 KB

That's not good news. What about this?

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1608842_128.patch, failed testing.

Lars Toomre’s picture

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

+++ b/core/includes/module.incundefined
@@ -148,68 +147,117 @@ function system_list($type) {
+ * @param boolean $load
+ *   Whether to call drupal_load() for the extension.

I think this should be something like:
* @param bool $load
* (optional) Indicates whether to call drupal_load() for the extension. Defaults to FALSE.

+++ b/core/includes/module.incundefined
@@ -1102,3 +1166,62 @@ function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+ * Helper function Drupal\Core\Config\Config::save() to sort module data.

Maybe 'Sorts the module data by weight before saving to config object.'

+++ b/core/lib/Drupal/Core/Config/Config.phpundefined
@@ -308,9 +308,15 @@ class Config {
+   * @param mixed $sorter
+   *   A callable which sorts the data before save. FALSE to disable sorting.
+   *   Defaults to Drupal\Core\Config\Config::sortByKey().

This description should start with '(optional)'.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.phpundefined
@@ -0,0 +1,18 @@
+   * @param string $collection
+   * @param string $class_name
+   * @return Drupal\Core\KeyValueStore\KeyValueStoreInterface

Each of these directives need a description line as well.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/TemporaryQueryTest.phpundefined
@@ -43,7 +43,7 @@ class TemporaryQueryTest extends WebTestBase {
-      $this->assertEqual($this->countTableRows("system"), $data->row_count, t('The temporary table contains the correct amount of rows.'));
+      $this->assertEqual($this->countTableRows("filter"), $data->row_count, t('The temporary table contains the correct amount of rows.'));
       $this->assertFalse(db_table_exists($data->table_name), t('The temporary table is, indeed, temporary.'));

For each of the assertion tests that get touched, can we remove the t() around the assertion text?

tim.plunkett’s picture

Status: Needs review » Needs work

The last submitted patch, drupal-1608842-133.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
449 bytes
90.71 KB

#132 is correct... but the linked issue was committed broken. Reopened it, reuploading it with the fix applied.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Configuration system

The last submitted patch, 1608842_134.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Configuration system

#132: drupal-1608842-133.patch queued for re-testing.

chx’s picture

What about this? $lists['bootstrap'][$name] = $filename; the missing $name was a problem.

Status: Needs review » Needs work

The last submitted patch, interdiff-132-1137.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
218.48 KB
2.31 KB
92.05 KB

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

chx’s picture

FileSize
486 bytes
91.77 KB

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

chx’s picture

Issue tags: -Needs tests
FileSize
1.06 KB
91.57 KB

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

Status: Needs review » Needs work

The last submitted patch, 1608842_141.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.74 KB
93.07 KB

I 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 :)

Status: Needs review » Needs work

The last submitted patch, 1608842_143.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
94.29 KB
4.41 KB

Fixed the locale test and addressed some documentation issues raised in #131

+++ b/core/includes/module.incundefined
@@ -1102,3 +1166,62 @@ function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+ * Helper function for Drupal\Core\Config\Config::save() to sort module data.

I have not changed this as this description (imho) is more useful to the developer.

chx’s picture

FileSize
3.49 KB
95.46 KB

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

Lars Toomre’s picture

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

effulgentsia’s picture

FileSize
95.46 KB

Rerolled for #147 and another HEAD change.

Status: Needs review » Needs work

The last submitted patch, 1608842_148.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
94.86 KB

More HEAD chasing. The drop keeps moving.

Status: Needs review » Needs work

The last submitted patch, 1608842_150.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
95.44 KB

Missed a hunk in #150's reroll.

Status: Needs review » Needs work

The last submitted patch, 1608842_152.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
95.44 KB

Ack. Stupid copy/paste error. This is still just a reroll of #146 with no substantive change.

effulgentsia’s picture

Overall, 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.

+++ b/core/includes/install.inc
@@ -418,20 +409,15 @@ function drupal_install_system() {
+  config('system.module')
+    ->set('system', 0)

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 to set('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?

+++ b/core/profiles/standard/standard.install
@@ -419,7 +419,7 @@ function standard_install() {
-  theme_enable(array('seven'));
+  config('system.theme')->set('seven', 1)->save();

This reverts #1712352-5: Configuration system does not support themes. Why?

RobLoach’s picture

FileSize
95.23 KB
  1. Removed the theme_enable change Alex referred to. If there is a reason for that change, let's make a follow up.
  2. In some cases during update, system.module wouldn't be available so early, so updating would fail. Including system.module manually fixed it.
  3. Let's move the other concern to #1797180: Sanitize the arguments of config('system.module').
  4. If this passes, RTBC++.
RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks chx and QA gods!

sun’s picture

Assigned: Unassigned » sun
Status: Reviewed & tested by the community » Needs review

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

RobLoach’s picture

Priority: Normal » Major

Pushing 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!

sun’s picture

Assigned: sun » Unassigned
Status: Needs review » Needs work

Thanks 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

  • drupal_get_filename() still statically caches, but always scans the filesystem on a static cache miss.
  • drupal_bootstrap() is changed to only bootstrap "forward".
  • state.storage service is replaced with a keyvalue (factory) service (without arguments, excluding former 'state' argument).
  • drupal_valid_test_ua() is changed to account for and allow to set a new test prefix in sub-requests. Essentially fixing #1786402: SimpleTestTest is broken.
  • system_rebuild_module_data(), system_rebuild_theme_data(), and _system_update_bootstrap_status() are replaced with system_list_reset().
  • A new module_set_weight() API function is added that allows to change the weight of a module.
  • drupal_container()->get('keyvalue')->get('system.module.schema')->set($module, $version); is used to save the schema version of a module.
  • config($name)->save($sort_callback) is introduced to allow for the caller of ::save() to specify a custom key sorting function instead of the config system's default.
  • The $reset argument to module_load_all() is removed.
  • _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.
  • _system_list_rebuild() is introduced to... wow. (see problems below)
  • module_enable() is changed to unconditionally scan the filesystem, regardless of whether dependencies are checked or not.
  • A config('system.module.disabled') is introduced to track the configuration of disabled modules (specifically, their weights). Next to the schema version info, this plays a key role in enabling a module. The configured weight of a disabled module is taken over when it is enabled.
  • 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.

Questions

  • Why does module_set_weight() work on disabled modules?
  • Is module_set_weight() supposed to be used by modules as a public API function?
  • Why is 'system.module.schema' a completely new keyvalue service and not a collection?
  • The change to drupalGet() doesn't seem to belong into this patch at all?
  • When and from where is simpletest_classloader_register() called without $module_data?
  • Why is System\InfoAlterTest removed?
  • How can module_load_all() work in tests without $reset?

Problems

  1. The changes to drupal_bootstrap() manifest a bug that is wrongly documented today already: Fact is, Drupal can be rolled back to an earlier bootstrap phase, and it is able to "re-bootstrap". Re-boostraping has nothing to do with code being loaded. Given that we'll keep the current testing framework for one more release, this is an essential problem.
  2. 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.
  3. 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().
  4. Config::save($sort_callback) doesn't fly, at all. We need to discuss #1785560: Remove the sorting of configuration keys instead.
  5. 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?
  6. 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.
  7. Why does _system_list_rebuild() hard-code a call to _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.
  8. 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. Unless, of course, _system_list_warm() does not only prime system_list()... hrm. :( So is the purpose to prime the classloader? No, it additionally passes a magic TRUE flag, which makes _system_list_warm() also load system.module. That flag isn't passed from anywhere else... not even from system_list() when processing the bootstrap module list. We want to remove that flag and the overloaded functionality.
  9. 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.
  10. The schema + weight futzing in module_enable() is undocumented.
  11. 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? (See also #127641: Module weight in .info)
  12. module_config_sort() only documents internals of fancy tricks, but not what it actually does and why it does what it does.
  13. 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. Combining the storage name ('system.module.schema') with the key being stored ($module) should give the desired result already. (Also, KeyValueFactory hard-codes DatabaseStorage....?)
  14. theme_disable() clears out a theme entirely, whereas the original code did not. The original code changed the theme status to 0 (disabled), which is different from deleting it entirely. This is one of the major reasons why I kept on repeating that we need to do #1067408: Themes do not have an installation status first. It really doesn't make sense to retain the current situation of enabled/disabled/uninstalled themes, because it does not make sense. (this change nicely proves that ;))
  15. update_prepare_d8_bootstrap() documents that system.module wouldn't be available "in some cases" and needs to be loaded manually -- which cases?
  16. update_prepare_d8_bootstrap() sorts modules by weight, but not themes. But themes are sorted by weight, too.
  17. Why is block_rebuild() not using list_themes(), now that the lengthy comment explaining the problem with list_themes() is seemingly resolved?
  18. 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?
  19. The change to LocaleLookup looks bogus to me - seems like that is duplicating and working around the root cause of #922996: Upgrade path is broken since update.php does not enforce empty module list
  20. The removal of System\InfoAlterTest looks invalid to me.
  21. 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.
  22. 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().
  23. We should retain the SCHEMA_* constants in install.inc instead of system.module. They belong to installation functionality, not System module.
  24. ...I'm running out of steam for tonight and can't review the system.module + run-tests.sh anymore tonight. Sorry.
chx’s picture

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

chx’s picture

So, here's a list of TODOs for whoever wants to reroll this. And huge thanks to sun again for this through review!

  1. Investigate whether the bugfix in drupalGet is necessary. Revert and run the upgrade tests. If it had blown up then the bugfix is still necessary :).
  2. Check whether you can assert InfoAlterTest against something. Your best hope is to use system_get_info().
  3. Bump the drupal_valid_test_ua from setUp/prepareEnvironment to run(). Make sure to patch script and browser both.
  4. Replace the recursion detecting in system_list / _system_list_rebuild with a pretty little lock. Make sure to still return config()->get() if called with module_enabled but can't get the lock as the patch does now.
  5. Collect followups from the issue into the summary (there are plenty) and add one to make info parsing only on demand.
  6. module_config_sort() needs more doxygen to explain that it is replacing ORDER BY weight, name.
  7. update_prepare_d8_bootstrap() documents that system.module wouldn't be available "in some cases" and needs to be loaded manually -- which cases? <= check this. I think when going from D7 to D8.
  8. Why is block_rebuild() not using list_themes(), now that the lengthy comment explaining the problem with list_themes() is seemingly resolved? <= please fix this.
  9. Revert the change in LocaleLookup and verify that the languageupgrade test still passes. If it doesn't then file a followup if I didn't already to fix it. (It might become a duplicate of #922996: Upgrade path is broken since update.php does not enforce empty module list )
  10. Nuke UpgradePathTestBase::uninstallModulesExcept because it seems it's not used anymore.
  11. 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().
  12. Edit: $this->data = call_user_func($sorter ? $sorter : array($this, 'sortByKey'), $this->data); collapse this into $sorter ?: array().
chx’s picture

Status: Needs work » Needs review
FileSize
100.21 KB
  1. drupalGet has been reverted.
  2. TODO: Check whether you can assert InfoAlterTest against something. Your best hope is to use system_get_info().
  3. Bumping is not possible, you need the database prefix which only happens during setup().
  4. Moving to a lock brings back the infinite recursion. It's simply too early, there's no working DB yet, often, here.
  5. TODO: collect followups into issue summary, add one for on demand .info parsing.
  6. Added more doxygen.
  7. It's very complicated to explain that if op wasn't null or access was denied but your session handler worked... meh. Read update.php for yourself. Changed doc to: We include system.module because update.php does not always include it.
  8. I put list_themes() there. I'll be surprised if it doesn't break somewhere.
  9. Nope, LocaleLookup can't be reverted.
  10. Unused uninstallModulesExcept deleted.
  11. TODO: 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(). Note: I deliberately did not change this. What works, works.
  12. I changed the sort mechanism to use a call() method. Hope this is better received. Even if not, I very strongly recommend not holding further this patch over it. We can do better in the sort removal issue. Note: the sort removal issue is presumed, I only added $this->sorted to Config.php to get us through the interim.

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

chx’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1608842_165.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
100.11 KB

Surprise! Things broke. But, phew, they are not the usual impossible-to-fix problems.

Status: Needs review » Needs work

The last submitted patch, 1608842_168.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
384 bytes
99.93 KB

Funny, the tests that broke actually passed with #168. But few others :P

alexpott’s picture

FileSize
94.33 KB
8.47 KB

Patch attach fixes revert of config object documentation and adds Drupal\system\Tests\System\InfoAlterTest back in (add makes it work :) )

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

#1799300: Replace the null lock backend with a file based variant is another (possible, optional) followup addressing 165.4

sun’s picture

Status: Reviewed & tested by the community » Needs work

There'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).

chx’s picture

Status: Needs work » Reviewed & tested by the community

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

sun’s picture

No, 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

This 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.)

chx’s picture

Assigned: Unassigned » alexpott

Alex is working on this.

alexpott’s picture

Assigned: alexpott » Unassigned
FileSize
95 KB
2.67 KB

Okay here goes... the patch attached stops system_rebuild_module_data() from being called during system_list() invocation - this is not currently possible for system_rebuild_theme_data() as during the first step of installation no themes are installed!

Status: Needs review » Needs work

The last submitted patch, 1608842-178-rebuild.patch, failed testing.

sun’s picture

The calls to system_rebuild_*_data() are irrelevant.

To repeat #174: system_list() must not call into _system_list_rebuild().

It still does:

@@ -144,22 +135,20 @@ function system_list($type) {
+      $lists = _system_list_rebuild();
...
+      if (_system_list_rebuild(TRUE) && $type == 'module_enabled') {
...
+      $lists = _system_list_rebuild();
chx’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
96.45 KB
2.31 KB

Continued with trying to remove the calls to system_rebuild_theme_data() and system_rebuild_module_data() since as chx pointed out _system_list_rebuild() is a new function and is only ever called from system_list().

alexpott’s picture

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

alexpott’s picture

After 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().

chx’s picture

I think we need more than a rename, we need to actually hardwire the update.php gets.

Status: Needs review » Needs work

The last submitted patch, 1608842_186.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
96 KB

Status: Needs review » Needs work

The last submitted patch, 1608842_188.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
96 KB
96 KB

Apparently I can't get the simplest patch right.

Edit: o_O I have no idea why it got uploaded twice, they are the same.

alexpott’s picture

FileSize
95.95 KB
741 bytes

Removing an unnecessary system_list_reset() as a result of talking to @sun (to give credit where its due).

From @sun in skype

All information that system_list() needs (on a cache miss) needs to be readily available and readable without any building/rebuilding/preparing/scanning/processing.

The reasoning and measure is simple: Consider 100 requests/sec bombing your site. There is no time for expensive recalculations.

Consider that you have a fast page cache enabled. So you only need the bootstrap modules for most requests. But on a cache miss for bootstrap modules, system_list() calls into the rebuild/prepare function, which recalculates tons of other stuff that is not needed.

The rebuild/prepare function still calls into system_list_reset(), which cannot be right, because a getter never flushes caches.

alexpott’s picture

FileSize
94.49 KB
1.29 KB

Reverting changes to field.module that where preventing a recursion that now is impossible.

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Assigned: Unassigned » catch

This looks like a catch-patch. :)

sun’s picture

I'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.

pounard’s picture

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

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

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

chx’s picture

Status: Needs review » Closed (won't fix)
gdd’s picture

Status: Closed (won't fix) » Needs review
chx’s picture

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

chx’s picture

To clarify: #162 was useful and I am glad for it. #195 is not useful and to #196 the only possible answer is #198.

chx’s picture

Assigned: catch » Dries
Status: Needs review » Reviewed & tested by the community

Or, rather, this.

webchick’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Needs review

Sorry, but this is not RTBC.

chx’s picture

Assigned: Unassigned » Dries
Status: Needs review » Reviewed & tested by the community

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

effulgentsia’s picture

Assigned: Dries » Unassigned
Status: Reviewed & tested by the community » Needs review

It's crystal clear there are some who does not want this to happen.

That'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:

I believe this is one of the most important changes since the introduction of the configuration system.

And I completely agree.

effulgentsia’s picture

Assigned: Unassigned » Dries
Status: Needs review » Reviewed & tested by the community

Sorry, #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.

chx’s picture

Assigned: Dries » catch
Status: Reviewed & tested by the community » Needs review

Resetting issue attributes and unfollowing the issue.

alexpott’s picture

I've done some work on this with the following aims:

I'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.

Status: Needs review » Needs work

The last submitted patch, 1608842_208.patch, failed testing.

alexpott’s picture

#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 :(

alexpott’s picture

Status: Needs work » Needs review

#208: 1608842_208.patch queued for re-testing.

alexpott’s picture

Yay 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)

pounard’s picture

Something 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

sun’s picture

Assigned: catch » Unassigned
FileSize
45.38 KB
67.47 KB

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

Status: Needs review » Needs work

The last submitted patch, config.system.214.patch, failed testing.

cosmicdreams’s picture

+++ b/core/includes/module.incundefined
@@ -135,20 +145,22 @@ function system_list($type) {
+      $bootstrap_list = state()->get('system.bootstrap_modules') ?: array();

Isn't "?:" inherently broken as a result of not properly firing exceptions?

+++ b/core/modules/system/system.moduleundefined
@@ -2729,97 +2733,81 @@ function system_get_module_info($property) {
+  $modules[$profile]->weight = 1000;

Magic value is better handled as a constant

Anonymous’s picture

ok, 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.

cweagans’s picture

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

chx’s picture

Assigned: Unassigned » catch
Status: Needs work » Needs review
  1. Reverted drupal_bootstrap() hack. That's not a hack. That's a bugfix. If you pass in a lower bootstrap value during bootstrap than the full and the current then Drupal will break. The only reason this usually doesn't happen because there's not much (aside from hook_boot) where a module can do something. But call theme() from hook_boot() for whatever reason and watch Drupal burn.
  2. Reverted module_load_all() change. Which was obsolete code, which was only used in tearDown and absolutely not necessary. I explained this already.

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

chx’s picture

re #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.

Anonymous’s picture

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

pounard’s picture

There 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:

  • Create an hardcoded (no dynamic registration) container for "module list bootstrap only"
  • Add database as the first service into this DIC
  • Add a "module manager" on top of that using the database as backend for loading the module list
  • Allow this micro container to compile itself so that the module list would be hardcoded into it
  • At bootstrap, load this container (compiled or not) and pass it to the kernel and run the kernel
  • Adapt the actual Drupal kernel to get the module and bundle list from this earlier container
  • Rewrite system_list() to use the registered module manager
  • When you enable or disable a module, just invalidate the compiled version of the early bootstrap container which contains only the database and the module manager
  • Every other module-related cache handling works the same (hook cache, system data, and whatever else)
  • If needed only hardcode a PHP-file-based config reader into it which then will be chained with the real config instances once the later container is bootstrapped which may allow some bits of configuration into this micro container (enable/disable compilation for devel sites for example)

If this is implemented this way we would have:

  • 0 database usage once compiled
  • Early module-provided bundles init before anything else possible
  • Earlier full container compilation possible (including config stuff and cache which may actually solve a lot of chicken an eggs problems, and allow both very early cache system bootstrap and services overrides by modules possible)
  • Once the real kernel got the "module list only container" all other services can be registered thanks to bundles only (and we come closer to Symfony yay) which means all services are usable from this point whatever are the dependencies, even if we didn't bootstrapped Drupal at all, we are closer to a real Symfony application
  • Removes every bit of chicken and egg problem between module list and configuration and key value etc...
  • Module list and database are the one and one hardcoded services, which is not worse of what we have today, Drupal always have been database dependent anyway, we don't really care removing this for module list right now
  • Making module list living in a key-value store is possible, this can be improved later as a follow up, but this would revert all this effort back and make the module list a chicken and egg problem once again
  • Non invasive modifications for this single patch right now: only one isolated container, a module manager, but no modification in the rest of core at all
cweagans’s picture

@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?"

pounard’s picture

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

catch’s picture

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

sun’s picture

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

sun’s picture

FileSize
44.45 KB
72.02 KB
cosmicdreams’s picture

Did 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?)

pounard’s picture

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

sun’s picture

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

cosmicdreams’s picture

My priority for tonight is Date format stuff, but I'll try to review this as well.

sun’s picture

FileSize
85.61 KB
15.5 KB
78.76 KB

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

Berdir’s picture

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

+++ b/core/includes/bootstrap.incundefined
@@ -885,23 +885,22 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+    // @todo Inject database connection into KeyValueStore\DatabaseStorage.

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.

+++ b/core/includes/install.core.incundefined
@@ -322,9 +322,9 @@ function install_begin_request(&$install_state) {
+    // @todo State service is gone; needs keyvalue + NullStorage replacement.
     $container
-      ->register('state.storage', 'Drupal\Core\KeyValueStore\DatabaseStorage')
-      ->addArgument('state');
+      ->register('keyvalue', 'Drupal\Core\KeyValueStore\KeyValueFactory');

Is that @todo still correct? Doesn't make sense to me.

+++ b/core/includes/install.incundefined
@@ -420,28 +410,26 @@ function drupal_install_system() {
+  // System module needs to be enabled and the system/module lists need to be
+  // reset first in order to allow config_install_default_config() to invoke
+  // config import callbacks.
+  // @todo Installation profiles may override the system.module config object.
+  // @todo Config\InstallStorage should actually throw an exception on write, why does this work?
+  config('system.module')
+    ->set('enabled.system', 0)

nitpick; >80 chars comment.

+++ b/core/includes/module.incundefined
@@ -132,6 +132,11 @@ function module_list_reset() {
+ * @todo There are too many layers/levels of caching involved for system_list()
+ *   data. Consider to add a config($name, $cache = TRUE) argument to allow
+ *   callers like this to force-disable any possible configuration storage
+ *   caching, or some other direct access to circumvent it/take it over.

callers like this => what is "this"?

+++ b/core/includes/module.incundefined
@@ -436,26 +475,34 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
   foreach ($module_list as $module) {
     // Only process modules that are not already enabled.
-    $existing = db_query("SELECT status FROM {system} WHERE type = :type AND name = :name", array(
-      ':type' => 'module',
-      ':name' => $module))
-      ->fetchObject();
-    if ($existing->status == 0) {
+    $enabled = TRUE;
+    $weight = 0;
+    if (!$schema_store->get($module)) {
+      $enabled = FALSE;
+    }
+    elseif (!$module_config->get("enabled.$module")) {
+      $enabled = FALSE;
+      $weight = $disabled_config->get($module);

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.

+++ b/core/includes/module.incundefined
@@ -655,8 +708,11 @@ function module_uninstall($module_list = array(), $uninstall_dependents = TRUE)
+    $schema_store->delete($module);

There is also a deleteMultiple() for delete() in loops.

+++ b/core/includes/module.incundefined
@@ -1124,3 +1180,59 @@ function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+ * Sort module config.

nitpick; SortS?

+++ b/core/includes/update.incundefined
@@ -129,6 +137,10 @@ function update_prepare_d8_bootstrap() {
+    // @todo update.php stages seem to be completely screwed up; the initial
+    //   requirements check is not supposed to change the system. All of the
+    //   following code seems to have been mistakenly/unknowingly added here and

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.

+++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.phpundefined
@@ -0,0 +1,29 @@
+namespace Drupal\Core\KeyValueStore;
+
+class KeyValueFactory {
+
+  protected $stores = array();

Missing documentation :)

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -742,6 +749,10 @@ protected function prepareEnvironment() {
+    if (drupal_valid_test_ua()) {
+      $this->originalPrefix = drupal_valid_test_ua();
+      drupal_valid_test_ua($this->databasePrefix);

I assume this is needed for running a test within a test?

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.phpundefined
@@ -264,21 +263,35 @@ protected function performUpgrade($register_errors = TRUE) {
+    // @todo WebTestBase::verbose() cannot be called here yet, since Simpletest
+    //   verbose output parameters are not prepared before test execution and
+    //   instead determined at runtime; i.e., file_create_url() calls into
+    //   system_list(), before update.php has upgraded the system list.

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?

sun’s picture

FileSize
5.38 KB
78.71 KB

@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().

chx’s picture

#208 Test run duration: 24 min 23 sec.

#234 Test run duration: 26 min 24 sec.

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

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

effulgentsia’s picture

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

+++ b/core/lib/Drupal/Core/Config/Config.php
@@ -346,7 +346,6 @@ public function load() {
   public function save() {
-    $this->sortByKey($this->data);
     $this->storage->write($this->name, $this->data);
     $this->isNew = FALSE;
     $this->notify('save');
@@ -370,26 +369,6 @@ public function rename($new_name) {

@@ -370,26 +369,6 @@ public function rename($new_name) {
   }
 
   /**
-   * Sorts all keys in configuration data.
-   *
-   * Ensures that re-inserted keys appear in the same location as before, in
-   * order to ensure an identical order regardless of storage controller.
-   * A consistent order is important for any storage that allows any kind of
-   * diff operation.
-   *
-   * @param array $data
-   *   An associative array to sort recursively by key name.
-   */
-  public function sortByKey(array &$data) {
-    ksort($data);
-    foreach ($data as &$value) {
-      if (is_array($value)) {
-        $this->sortByKey($value);
-      }
-    }
-  }
-
-  /**

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?

chx’s picture

That's just #1785560: Remove the sorting of configuration keys rolled into this one to make it work.

chx’s picture

effulgentsia’s picture

Do we also need a follow up for dropping {system_list} on existing sites?

chx’s picture

Do we need to drop such small amounts of data? I dont think we usually drop old tables but I am not 100%.

Anonymous’s picture

FileSize
4.61 KB
78.76 KB

attached patch includes some doc-only cleanups, interdiff.txt attached.

+ *
+ * @todo There are too many layers/levels of caching involved for system_list()
+ *   data. Consider to add a config($name, $cache = TRUE) argument to allow
+ *   callers like system_list() to force-disable a possible configuration
+ *   storage controller cache or some other way to circumvent it/take it over.

should this todo live at config()'s definition as well? not sure.

+  // Remove last known theme data state.
+  // This causes system_list() to call system_rebuild_theme_data() on its next
+  // invocation. When enabling a module that implements hook_system_info_alter()
+  // to inject a new (testing) theme or manipulate an existing theme, then that
+  // will cause system_list_reset() to be called, but theme data is not
+  // necessarily rebuilt afterwards.
+  // @todo Obsolete with proper installation status for themes.
+  state()->delete('system.theme.data');

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?

enabled:
  block: '-5'
  color: '0'
  comment: '0'
  contextual: '0'
  dblog: '0'
  field: '0'
  field_sql_storage: '0'
  field_ui: '0'
  file: '0'
  filter: '0'
  help: '0'
  image: '0'
  menu: '0'
  node: '0'
  number: '0'
  options: '0'
  overlay: '0'
  path: '0'
  rdf: '0'
  search: '0'
  shortcut: '0'
  system: '0'
  taxonomy: '0'
  text: '0'
  toolbar: '0'
  update: '0'
  user: {  }
  standard: '1000'

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.

Anonymous’s picture

Status: Reviewed & tested by the community » Needs work

ahem.

sun’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
80.37 KB

Fixed module_enable() does not properly check for NULL weight values.

See also: #1807058: Config stores a NULL value as an array

sun’s picture

FileSize
468 bytes
80.63 KB

re #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.

Status: Needs review » Needs work

The last submitted patch, config.system.246.patch, failed testing.

Anonymous’s picture

Status: Needs work » Reviewed & tested by the community

yay, #245 passed tests, and i think it's RTBC. can we do #246 as a follow up?

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
80.64 KB

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

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

ok, #249 then.

pounard’s picture

+    // We can't use the sign directly because + (ASCII 43) is before
+    // - (ASCII 45). So negative nubmers get a 0, non-negative numbers
+    // a 1 prefix.

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

cosmicdreams’s picture

pounard: I have to disagree with #251. I love that level of detail.

Fabianx’s picture

Agree with #252, the comment is useful.

pounard’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
77.45 KB

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

sun’s picture

FileSize
77.97 KB

Additionally incorporated @pounard's remarks/suggestion from #254:

Fixed documentation of module_config_sort().

sun’s picture

FileSize
1.57 KB

whoopsie, forgot interdiff, sorry for the noise. :-/

chx’s picture

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

catch’s picture

Title: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config » Change notice for: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config
Priority: Major » Critical
Status: Reviewed & tested by the community » Active

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

podarok’s picture

Issue tags: +Needs change record

tagging

Dave Reid’s picture

Wondering 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:

  $weight = (int) db_query("SELECT weight FROM {system} WHERE name = 'dependency'")->fetchField();
  db_update('system')
    ->fields(array('weight' => $weight + 1)) 
    ->condition('name', 'mymodule')
    ->execute();

I don't see a way to possibly do this with the newly-converted code.

chx’s picture

module_set_weight('mymodule', config('system.module')->get('enabled.othermodule') + 1)

Anonymous’s picture

webchick’s picture

One 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:

  // Enable the admin theme.
  db_update('system')
    ->fields(array('status' => 1))
    ->condition('type', 'theme')
    ->condition('name', 'seven')
    ->execute();
  variable_set('admin_theme', 'seven');
  variable_set('node_admin_theme', '1');

Now, the proper way to do this is with a call to theme_enable():

Drupal 8:

  // Enable the admin theme.
  theme_enable(array('seven'));
  variable_set('admin_theme', 'seven');
  variable_set('node_admin_theme', '1');
chx’s picture

In the past installation profiles could get away with messing system table, now forget it.

webchick’s picture

Also, after installing Spark, on the last page of the installer I am getting a bunch of notices that are new since last week:

	•	Notice: Undefined index: admin_icons in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: aloha in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: bunnypoint in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: caption in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: gridbuilder in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: layout in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: libraries in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: region in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: spark in system_list() (line 194 ofcore/includes/module.inc).
	•	Notice: Undefined index: spark_demo in system_list() (line 194 ofcore/includes/module.inc).

Here's the surrounding code from system_list():

      $enabled_modules = config('system.module')->get('enabled');
      $module_files = state()->get('system.module.files');
      foreach ($enabled_modules as $name => $weight) {
        // Build a list of all enabled modules.
        $lists['module_enabled'][$name] = $name;
        // Build a list of filenames so drupal_get_filename can use it.
        $lists['filepaths'][] = array(
          'type' => 'module',
          'name' => $name,
          'filepath' => $module_files[$name], ### LINE 194.
        );
      }

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?

sun’s picture

Assigned: catch » Unassigned

Let'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 ;)

amontero’s picture

"git blame" led me here.
Linking related issue:
#1810258: PHP notices thrown in interactive installer

Berdir’s picture

webchick’s picture

Status: Active » Fixed

Thanks 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!

Tor Arne Thune’s picture

Title: Change notice for: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config » Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config
Priority: Critical » Major

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

larowlan’s picture

xjm’s picture

Issue tags: -Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

xjm’s picture

Issue summary: View changes

rewritten issue summary