Anyone putting a breakpoint on it should be able to catch all joint points (hooks). foreach (module_implements()) should disappear. This should be possible with the &$a1= NULL construct, making it possible to pass around references.

Following an irc discussion, we came up with this possible draft for a refactored hook/extension system. Needs fleshing out.


namespace Drupal;

class Extensions {

  protected $modules = array();

  protected $themes = array();

  public function moduleEnable($module) {
  }

  public function moduleDisable($module) {
  }

  public function themeEnable($theme) {
  }
  
  public function themeDisable($theme) {
  }

  public function invoke($hook) {
  }

  public function alter() {
  }
  
  public function isImplemented($hook) {
  }
  
  public function moduleInfo($key) {
  }

}

function extensions() {
  static $extensions_instance;
  if (!isset($extensions_instance)) {
    $extensions_instance = new Extensions;
  }
  return $extensions_instance;
}
CommentFileSizeAuthor
#249 module.handler.249.patch147.87 KBsun
#249 interdiff.txt6.01 KBsun
#236 1331486.module_handler.236.patch142.87 KBkatbailey
#236 interdiff.txt751 byteskatbailey
#215 module.handler.215.patch142.13 KBsun
#215 interdiff.txt1.01 KBsun
#211 module.handler.211.patch141.12 KBsun
#211 interdiff.txt1.31 KBsun
#207 module.handler.200.patch140.91 KBsun
#207 interdiff.txt80.66 KBsun
#205 1331486_204.patch150.21 KBchx_
#205 interdiff.txt3.46 KBchx_
#204 1331486_204.patch0 byteschx
#202 1331486_202.patch0 byteschx
#202 interdiff.txt750 byteschx
#197 module.handler.197.patch147.84 KBsun
#193 1331486.module_handler.193.patch147.96 KBkatbailey
#193 interdiff.txt591 byteskatbailey
#190 1331486.module_handler.190.patch147.38 KBkatbailey
#190 interdiff.txt783 byteskatbailey
#186 1331486.module_handler.186.patch146.62 KBkatbailey
#186 interdiff.txt30.64 KBkatbailey
#174 1331486.extension_handler.174.patch148.03 KBkatbailey
#174 interdiff.txt18.17 KBkatbailey
#167 1331486.extension_handler.167.patch142.74 KBkatbailey
#167 interdiff.txt69.82 KBkatbailey
#159 1331486.extension_handler.159.patch141.46 KBkatbailey
#159 interdiff.txt19.88 KBkatbailey
#158 1331486-158-extension_handler.patch142.98 KBAnonymous (not verified)
#156 1331486-156-extension_handler.patch142.93 KBAnonymous (not verified)
#150 1331486.extension_handler.150.patch142.73 KBkatbailey
#150 interdiff.txt2.24 KBkatbailey
#147 1331486.extension_handler.147.patch140.49 KBkatbailey
#147 interdiff.txt3.27 KBkatbailey
#145 1331486.extension_handler.145.patch138.73 KBkatbailey
#145 interdiff.txt3.43 KBkatbailey
#143 1331486.extension_handler.143.patch139.4 KBkatbailey
#143 interdiff.txt19.53 KBkatbailey
#140 1331486.extension_handler.140.patch132.31 KBkatbailey
#140 interdiff.txt4.55 KBkatbailey
#138 1331486.extension_handler.138.patch129.72 KBkatbailey
#138 interdiff.txt26.78 KBkatbailey
#136 1331486.extension_handler.136.patch129.49 KBkatbailey
#136 interdiff.txt13.3 KBkatbailey
#132 1331486.extension_handler.132.patch118.34 KBkatbailey
#132 interdiff.txt25.4 KBkatbailey
#130 1331486.extension_handler.130.patch113.94 KBkatbailey
#130 interdiff.txt1.5 KBkatbailey
#127 1331486.extension_handler.127.patch113.94 KBkatbailey
#127 interdiff.txt576 byteskatbailey
#122 1331486.extension_handler.122.patch113.89 KBkatbailey
#122 interdiff.txt15.95 KBkatbailey
#118 1331486.extension_handler.118.patch106.05 KBkatbailey
#118 interdiff.txt6.43 KBkatbailey
#114 1331486.extension_handler.114.patch110.27 KBkatbailey
#112 1331486.extension_handler.112.patch110.27 KBkatbailey
#109 1331486.extension_handler.109.patch100.17 KBkatbailey
#102 1331486.extension_handler.102.patch130.3 KBkatbailey
#102 interdiff.txt1.92 KBkatbailey
#100 1331486.extension_handler.100.patch129.38 KBkatbailey
#100 interdiff.txt12.46 KBkatbailey
#96 1331486.extension_handler.96.patch124.44 KBkatbailey
#96 interdiff.txt2.06 KBkatbailey
#94 1331486.extension_handler.93.patch124.36 KBkatbailey
#91 1331486.extension_handler.91.patch123.93 KBkatbailey
#89 1331486_89.patch123.96 KBchx
#87 1331486.extension_handler.87.patch123.95 KBkatbailey
#84 1331486.extension_handler.84.patch123.91 KBkatbailey
#84 interdiff.txt7.12 KBkatbailey
#75 1331486.extension_handler.75.patch117.66 KBkatbailey
#75 interdiff.txt41.12 KBkatbailey
#72 1331486_grrr_grrr.patch105.6 KBchx
#70 1331486.patch105.08 KBchx
#70 interdiff.txt3.64 KBchx
#67 1331486_less_exceptions.patch104.15 KBchx
#67 interdiff.txt773 byteschx
#64 1331486_hope_dies_last.patch104.1 KBchx
#62 1331486_the_grass_is_green_i_wonder_whether_this_is_too.patch104.53 KBchx
#59 1331486.patch104.12 KBchx
#52 1331486-52.patch102.85 KBamateescu
#52 1331486-interdiff-49-52-do-not-test.patch1.79 KBamateescu
#49 1331486_47.patch102.84 KBchx
#47 1331486_47.patch85.73 KBchx
#42 1331486-41-extension-handler.patch99.49 KBkatbailey
#39 1331486-39-extension-handler.patch98.53 KBkatbailey
#36 1331486-36-extension-handler.patch88.69 KBkatbailey
#33 1331486-33-extension-handler.patch362.59 KBkatbailey
#27 1331486.OOPify-module-inc.patch97.83 KBkatbailey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

For those who abhor PHP debuggers (me too) the ideal outcome is that every. single. hook call can be caught by print statements inside module_invoke_all.

Do we want to nuke #foo callback arrays too? I feel a tentative yes but I need to figure out how to identify on those.

chx’s picture

Title: Make module_invoke_all the single weaver » Allow debugging all multiple-variable-function-call at one place.

So

  1. We change the function signature to allow references. This is slightly ugly code:
    
    function a(&$a) {
      $a++;
    }
    
    function invoke(&$arg0 = NULL, &$arg1 = NULL) {
      $args = func_get_args();
      for ($i = 0; $i < func_num_args() && $i < 2; $i++) {
        $args[$i] = &${"arg$i"};
      }
      call_user_func_array('a', $args);
    }
    
    $a = 1;
    invoke($a);
    print "$a\n";
    
  2. We check whether $hook[0] == '#' and if so then we invoke everything listed in $arg0[$hook]. This results in module_invoke_all('#submit', $form, $form_state); inside form.inc. Pretty! We might want to rename module_invoke_all as a followup.
  3. $hook needs to be able to handle an array of hooks to accomodate drupal_alter(). drupal_alter becomes a 1 line wrapper for module_invoke_all.
  4. We change module_implements to _module_implements to discourage from using it.
chx’s picture

On further discussion we could do more awesome by moving these into a class, let's say \Drupal\Module and then hide everything in there (module_list and module_implements and friends) and just expose invoke() and isImplemented() for the sake of node_access and the like.

We could move module_implements writing into the destructor / investigate moving to DrupalCacheArray.

A tentative skeleton by Justin Randell is at http://paste.pocoo.org/show/505043/ Edit; if we move the extensions() static to drupal_static it can be reset and the constructor can reread the config and then reloading the modules is a configuration problem. So either CMI solves it or we allow for something like http://drupal.org/project/environment_modules

catch’s picture

Priority: Normal » Major

OK discussed this with some length with chx and beejeebus in irc. chx is adamant that we don't make the Extensions class pluggable for simplicity- which I'd fully agree with for the module system, especially if {system} storage is moved to CMI - pluggable storage for module/theme/install profile metadata would be the only reason to make it pluggable for me but if that is handled via CMI all the better.

The module system as it is now is extremely fragile, see issues like #1061924: system_list() memory usage, #222109: module_list() should be rewritten as 2 functions: module_list() and module_list_bootstrap(), and #1103910: bootstrap_invoke_all() queries bootstrap modules twice (and countless others). Moving it to a class will not fix every issue, but the overall clean up effort would go a long way towards improving things, as well as the original intention here of making it easier to find out where hooks are invoked from, which is a major complaint at the moment in Drupal 7.

So I'm bumping priority of this to 'major', it's a refactoring task I don't think we can afford to skip.

chx’s picture

Yes -- variable classnames combined with forbidding #363802: Document class methods with class, factory and defining interfaces is a blight I want to avoid.

Crell’s picture

Module-as-class is a non-starter, for many reasons we've gone over before. However, moving more functionality into classes would help with performance and modularity. I could also see an argument for a "module metadata as class", as long as it is not expected to contain the entire module's code base.

(I've been toying in the back of my head with hooks-as-classes, inspired by an old blog post of chx's, but there's considerable registration issues I've not worked out yet.)

chx’s picture

Noone wants to move modules into classes. module.inc itself, that's a different question.

Crell’s picture

Ah, I see. Yes, moving the logic in module.inc over to classes makes a lot of sense. (I had no context on this apparent conversation from last night.)

catch’s picture

Pasted Justin's draft direct into the summary so there is more context here.

Crell’s picture

Aside from the contents of the class autoloading, which will happen on virtually every request, is there any actual benefit from the proposal currently in the summary? It looks like it's just moving existing functions into methods without really rethinking them.

catch’s picture

The draft in the summary was a very quick write up based on the tail-end of an irc conversation, I just wanted it in the summary rather than buried in a link since it looked like there was misunderstanding in this issue about what it was about at all.

There is more to this than 'just moving existing functions into methods' though already. Having a class would quickly allow for resolving some of the bizarre state tracking in the module system (cf. module_list() and module_implements_write_cache()).

The original motivation for this issue was being able to debug hook invocations from one place (so not having to deal with function calls come from module_implements() + $function, module_invoke_all(), module_invoke(), node_invoke(), user_module_invoke(), drupal_alter(), bootstrap_invoke_all() or any of the other places hooks can currently be called from. We even allow the same hook to be called in different ways from different places.

Autoloading of the module system is not about avoiding loading code on every request (same as autoloading http libraries does not do that either). What it would hopefully do is allow us to lazy-initialize that system during each request though - some requests need all modules loaded before others do, so it is about timing rather than frequency. That in turn could prevent some of the chicken and egg situations we have with full bootstrap at the moment. There may well be some parts of module.inc (and especially system module and install.inc) that could properly be lazy loaded and would be rarely installed (I'm not sure enabling modules belongs in this class for example).

The current brittleness of the module system is one of my concerns with the WSCCI patch as it currently stands, as you know (either you put things in a bootstrap hook and kill performance, or you hope they get called before something else needs them).

It's also the cause of a large number of race conditions in core (some 'fixed', some not), most of which come down to module_list() and module_implements() returning completely different things at different times (incomplete class registries, incomplete theme registries etc. etc.). A patch on this issue that left that bizarre behaviour intact is unlikely to get in.

Our tracking of state in the module system is a real mess with functions like module_implements_write_cache(), moving that logic into a class lets us clean it up almost immediately. And the class is called extensions.inc for a reason - the module, theme and install profile APIs are all parallel to each other and freely depend on each other in the strangest of ways (like half of module.inc depending on functions in system module for a start).

catch’s picture

Title: Allow debugging all multiple-variable-function-call at one place. » Move module_invoke_*() and friends to an Extensions class

Making it clearer what the current proposal is.

sun’s picture

I agree with the fundamental idea. Putting this into the DI container will allow us to bootstrap the module/hook system on demand.

The draft/prototype in the summary looks "workable" to me. I'd call it Drupal\Core\Extension\Manager though.

Given #1497366: Introduce Plugin System to Core, perhaps a Module, Theme, and Profile could be variants/derivatives of an abstract Extension plugin type?

(...so a Libraries API in D8 could declare an additional Library variant...? hm!)

Please also note that #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config is touching many related parts of this puzzle, and working on that revealed that the current enable/disable routines of modules and themes have little differences (which of course is based on the embedded #1067408: Themes do not have an installation status change).

Crell’s picture

So it sounds like the idea here is to move module existence declaration to a per-module class. If we're going to do that, we should look at Symfony's Bundle definitions in the kernel which do precisely that. It also includes extension points for exposing information to Dependency Injection compiler passes, which we will likely need in order to make the DI performant.

(In fact, if we start using bundles, even subclasses of them, it opens the possibility of supporting at least some Symfony bundles in Drupal... which would be really cool.)

aspilicious’s picture

But in the end we need to add these bundles. So we should have a hook_info or some kind of symfony .info file. Or something different.

Crell’s picture

Symfony uses magic/conventional naming for its bundles, I think. So if we did something like Drupal\$modulename\$modulenameBundle, that should work. The only caveat there would be capitalization, but since the module name is taken from the info file, not the directory, we could use that? (So a module whose info file calls it My Great Module would become Drupal\mymodule\MyGreatModuleBundle.)

RobLoach’s picture

Yes, yes, and YES! Been wanting to get this pattern in ever since the Kernel hit. Instead of invoking hooks on the Bundles though, we would simply pass in Drupal's Container. From there, the Bundle registers what it needs to the Container. Symfony has a compiler pass, allowing it to "compile" its configuration and save it to improve performance, but I don't see us getting that far with it. As a first pass for this issue, I'd consider the initial Bundle set up pretty good. As long as Bundles are passed the $container on instantiation, then we could then move on to do fancy stuff with it.

Event/hook registration discussion should probably be talked about over at #1509164: Use Symfony EventDispatcher for hook system and #1599108: Allow modules to register services and subscriber services (events).

catch’s picture

There's two parts to this I think:

- discovery/retrieval etc. of the list of modules/themes - currently the drupal_rebuild_*() functions, module_list() etc.
- invoking hooks which is what this issue deals with, I've been assuming the module list would be injected into this class.

Sounds like Symfony's bundle system is dealing with the former, we don't have an issue for that, and I don't think this is it - it might be worth discussing in another issue though.

This issue and the EventDispatcher one are dealing with the latter. Moving 100% over to EventDispatcher would make at least some of the current functionality redundant, but actual hook registration is thorny, so what would be a relatively simple change here that wouldn't affect hook registration/discovery at all (except for some internal refactoring since module_implements() moves into this class) seems worth doing to try to get the hook system loaded on demand and out of an explicit bootstrap stage. Obviously we can't properly lazy load it without also sorting out the module listing stuff alongside it.

Crell’s picture

If we move to EventDispatcher entirely, then registration is quite straightforward: We use the build method of the bundle class to specify what subscribers should be attached to the event dispatcher, the event dispatcher itself is put into the dependency injection container, and then the container is dumped to disk. That's what Symfony itself does, and after talking with Fabien last week at Symfony Live I think it makes sense. (Technically Symfony full stack uses config files to populate the EventDispatcher, not raw code; that's an implementation detail.) Given that we have to use EventDispatcher at least somewhat, this is something we need to do anyway.

If we keep hooks around for another version (which speaking pragmatically I think is likely), then I agree moving the core of the module system into an object is a good thing. Not so much for lazy loading, but for the testability. Rather than having an extensions() method, I'd actually go straight to putting it in the DIC. Then we could for testing purposes swap out the default object in the DIC (which uses the same PHP global state registration mechanism as now) with one just for testing that has a hard-coded list of functions to call for a given hook. Just that simple change would greatly improve our ability to test things (we could eliminate a fair number of test modules, I think), so I support that change on that ground alone.

The code dump currently in the summary made me think it was intended to be an Extension class per module a la Bundles, which I think is what confused me. Looking at it again, I see that it's just a singleton version of module.inc. As long as that ExtensionManager class ends up in the DIC, I'm +1 on it.

pounard’s picture

While I agree I'd love to maximise the use of the event dispatcher and listeners, I'd be afraid that it might create a lot of CPU overhead. I have as example a Commerce site which calls almost 2000 times drupal_alter() per page run for example, which is a lot. If we go this way, we'd probably have to reduce the number of hooks we have (all kind of hooks) in the flavor of cached/PHP built code registration mecanisms, at least for all stuf such as plugins/components (I'm not thinking only cache backends or views handlers, but also for form API elements definition, all info array, etc...) and discourage modules to allow those alterations at normal runtime.

I like the config based definitions, it's clean and highly readable, and all it takes it merging configs altogether when enabling modules instead of doing it at runtime. We'd end up with larger registries, consuming more memory, but more static ones, consuming a lot less of CPU time on normal runtime. And if the kernel and DI patches goes as expected, we can save a lot of memory at many places and use those savings for this kind of registry mecanism.

Anonymous’s picture

Assigned: Unassigned »

i'll have a crack at a PoC patch for this tomorrow to get discussion going again.

Anonymous’s picture

Assigned: » Unassigned

sorry, this is beyond my programming skills.

i don't understand Symfony and our usage of it enough to know where to start here.

Crell’s picture

You don't need much Symfony-fu here. Really just the DIC, which isn't even step 1. :-)

Basically, I'd start by porting modules.inc to a Modules class, which takes in its constructor a DB connection or whatever. It should contain methods that are largely a direct port of whatever is in modules.inc, with any statics converted to object properties (not class properties). It may be hard to unit test since by design it relies on global state, but at least you can get a sense for how that would work. Post that here and we'll see where you get to. :-)

(Yes we also want that for themes, but one step at a time.)

pounard’s picture

#23's plan sounds fine as a start.

Sylvain Lecoy’s picture

Jumping from #1774388: Unit Tests Remastered™ #14, if I understand the raison d'être of this issue is to move from a Template Method to a Strategy pattern solution with Dependency Injection in mind for hook system ?

This would allow greater encapsulation and more testable unit of source code.

katbailey’s picture

Assigned: Unassigned » katbailey

I am taking an initial stab at this as it affects #1784312: Stop doing so much pre-kernel bootstrapping.

katbailey’s picture

Status: Active » Needs review
FileSize
97.83 KB

Ok, this is as far as I got with this this weekend - I only tackled moving system_list() and related functions, i.e. leaving everything to do with hooks alone. Maybe I am slightly hijacking this issue - my main priority is allowing us to not bootstrap beyond configuration before instantiating the kernel. In this patch I have a Drupal\Core\Extension\Modules class (which I actually think should be renamed to just Drupal\Core\System but naming can wait...). We instantiate it in DrupalKernel and then inject it into the brand new DIC as a synthetic service.

It took a stupidly long time for me to get this to work for WebTests, which is why I only got as far as I did. Maybe dealing with the invoke/implements stuff won't be as scary as I'm imagining, I just haven't gotten to it yet...

Status: Needs review » Needs work

The last submitted patch, 1331486.OOPify-module-inc.patch, failed testing.

The last submitted patch, 1331486.OOPify-module-inc.patch, failed testing.

catch’s picture

I think we'll need two services - one which manages the lists of extensions (modules + themes), and one which handles hook invocation (which can have the former injected into it). Maybe not but it may not be possible to do the one without the other so I don't think it's a hijack anyway :)

katbailey’s picture

Status: Needs work » Postponed

Hmm, I think it makes a lot of sense to wait until #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config gets in before doing any more work on this. For one thing, I think that one of the problems I'm having with the installer might be solved by chx's patch there.

chx’s picture

That will solve some, raise new concerns but I am a little worried how this got steered from " Allow debugging all multiple-variable-function-call at one place." let's keep that as a very major concern at least in a followup it really needs to happen.

katbailey’s picture

Status: Postponed » Needs review
FileSize
362.59 KB

I've been reworking this on top of #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config which looks like it might land soon and touches a lot of the code that this patch needs to move around. Just wanted to roll a combined patch and see if testbot can give me some indication of where we're at with tests.

Status: Needs review » Needs work

The last submitted patch, 1331486-33-extension-handler.patch, failed testing.

chx’s picture

The Creating default object from empty value, Undefined property: stdClass::$filename, Undefined property: stdClass::$parsed_info, Undefined property: stdClass::$name, Undefined property: stdClass::$name family of notices usually arise when you have a profile mismatch in what drupal_get_profile sees and what is in config('system.modules')->get().

usort(): Array was modified by the user comparison function can easily mean that an error was thrown. It doesnt mean the array was actually was changed.

katbailey’s picture

Status: Needs work » Needs review
FileSize
88.69 KB

So as it turned out, trying to solve this problem on top of #1608842: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config wasn't such a great idea - I had thought that one was closer to landing than it was and I had also thought there were problems with my intended approach here that relied on it, but that doesn't seem to be the case. Curious to see what testbot makes of this one...

Status: Needs review » Needs work

The last submitted patch, 1331486-36-extension-handler.patch, failed testing.

katbailey’s picture

Durr... I had only tested the installer with an existing settings.php :-/ Totally forgot about this problem, which Sun had talked about in the other issue:

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.

I'll try and tackle this tomorrow.

katbailey’s picture

Status: Needs work » Needs review
FileSize
98.53 KB

OK, here's a fairly crude abstraction of the ExtensionHandler stuff into an interface and two implementations, one for the installer that never looks to the db, and one for regular use. Although it is crude, I'm not sure there'd be much point in refining it as the installer is going to get rewritten anyway. Testbot should get a bit further with this one...

Status: Needs review » Needs work

The last submitted patch, 1331486-39-extension-handler.patch, failed testing.

chx’s picture

Don't be so sure that the installer work is going to happen... I am certainly not :/

katbailey’s picture

The one outstanding test failure was due to the following code:

$static = drupal_static('module_implements'); 

There is no such statically cached variable anymore - this information is stored in an instance variable of the ExtensionHandler class. So I added a getter method for that to use in the test instead.

katbailey’s picture

Status: Needs work » Needs review
katbailey’s picture

OK, so now that it's green I should clarify what this patch does:

  • It moves most of the functions out of module.inc and into a Drupal\Core\ExtensionHandler class.
  • It overrides the Kernel class's boot() method in DrupalKernel, where it instantiates an ExtensionHandler for use when registering bundles (to check which modules are enabled that might be providing bundles). It then registers this ExtensionHandler as a synthetic service to the DI container it builds so that it can be reused throughout the request.
  • In index.php we now only bootstrap to DRUPAL_BOOTSTRAP_CONFIGURATION instead of DRUPAL_BOOTSTRAP_CODE before instantiating the kernel.
  • It adds the config service definitions to the CoreBundle, where they belong (instead of grabbing them from drupal_container() and merging them in). This means we are duplicating code but the hope is that we will eventually eliminate the need for the minimal container (with this patch it is no longer used on normal page requests) or at least replace it with a precompiled one per #1703266: Use a precompiled container for the minimal bootstrap container required outside of page requests.

Given my obvious agenda with this patch, i.e. #1784312: Stop doing so much pre-kernel bootstrapping, I hope it still achieves the goals that others have in mind.

Crell’s picture

Wow, great work, Kat! Review below.

+++ b/core/includes/bootstrap.inc
@@ -2469,6 +2473,96 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
 /**
+ * Returns an ExtensionHandlerInterface object.
+ *
+ * This is a temporary function that will instantiate a new ExtensionHandler if
+ * there isn't one present in the container. This will only happen if the
+ * boostrap container is being used, e.g. during installation.
+ * @todo Remove this and convert all calls to it to
+ *  drupal_container()->get('extension_handler') once the installer is using a
+ *  Kernel.
+ */
+function drupal_extension_handler($installer = FALSE) {

I'm contractually obligated to note that the description shouldn't specify the interface but the object, but there should be a @return directive that specifies the interface. *duck*

+++ b/core/includes/update.inc
@@ -408,10 +408,10 @@ function update_module_enable(array $modules) {
-    // system_list_reset() is in module.inc but that would only be available
+    // drupal_extension_handler()->systemListReset() is in module.inc but that would only be available

Is it in module.inc? Hard to tell from the patch but it looks like it's in bootstrap.inc.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -26,6 +29,16 @@
+   * ExtensionHandler instance holding the list of enabled modules.
+   */
+  protected $extension_handler;

camelCase, please.

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -36,6 +49,21 @@ public function init() {
   /**
+   * Overrides Kernel::boot().
+   */
+  public function boot() {
+    // Instantiate an ExtensionHandler which the Kernel itself needs in order to
+    // find out which modules are enabled. In the buildContainer() method we
+    // register this and the database connection we pass to it as synthetic
+    // services to the container so that they do not need to be instantiated
+    // over again.
+    $this->connection = Database::getConnection();
+    $this->extension_handler = new ExtensionHandler($this->connection, CacheFactory::get('cache'), CacheFactory::get('bootstrap'));
+    parent::boot();
+    drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);
+  }
+

Just an FYI, there are something like a half dozen patches in the queue right now that would break this. :-)

CacheFactory may not survive http://drupal.org/node/1764474. I think that issue is also trying to move database.info into the DIC, and then reference it for instantiating the database.

And of course there's http://drupal.org/node/1608842, which just went RTBC again as I was writing this and eliminates the database need here. So, all in this is clever but is the main place we'll need to chase head, methinks. Hopefully it gets easier with each patch, not harder...

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  /**
+   * Cache backend for storing enabled modules.
+   */
+  protected $bootstrap_cache;

camelCase, please.

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  /**
+   * Array of enabled modules.
+   */
+  protected $module_list;

camelCase, please.

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  /**
+   * Keeps track internally of hook info.
+   */
+  protected $hook_info;

You guessed it. :-)

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  /**
+   * Keeps track internally of alter functions.
+   */
+  protected $alter_functions;

Ibid.

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  public function loadAll($bootstrap = FALSE, $reset = FALSE) {
+    if ($reset) {
+      $this->loaded = FALSE;
+    }
+    if (isset($bootstrap) && !$this->loaded) {
+      $type = $bootstrap ? 'bootstrap' : 'module_enabled';
+      foreach ($this->moduleList($type) as $module) {
+        drupal_load('module', $module);
+      }
+      // $has_run will be TRUE if $bootstrap is FALSE.
+      $this->loaded = !$bootstrap;
+    }
+    return $this->loaded;
+  }

Is this a direct port of existing code? Because it looks like it replicates existing design flaws, too. Folding multiple operations (load bootstrap module list and non-bootstrap list) into the same operation was never a good idea, so we shouldn't keep doing it if we don't have to.

The drupal_load() function call should get turned into a method, too, I think.

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  public function moduleList($type = 'module_enabled', array $fixed_list = NULL, $reset = FALSE) {
+    if ($reset) {
+      $this->module_list = NULL;
+      // Do nothing if no $type and no $fixed_list have been passed.
+      if (!isset($type) && !isset($fixed_list)) {
+        return;
+      }
+    }
+
+    // The list that will be be returned. Separate from $module_list in order
+    // to not duplicate the static cache of drupal_extension_handler()->systemList().
+    $list = $this->module_list;
+
+    if (isset($fixed_list)) {
+      $this->module_list = array();
+      foreach ($fixed_list as $name => $module) {
+        drupal_get_filename('module', $name, $module['filename']);
+        $this->module_list[$name] = $name;
+      }
+      $list = $this->module_list;
+    }
+    elseif (!isset($this->module_list)) {
+      $list = $this->systemList($type);
+    }
+    return $list;
+  }

module_list() was a noun. Methods should be verbs. getEnabledModules(), perhaps?

Also see about regarding $type.

Can we eliminate the external call to drupal_get_filename()?

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  /**
+   * Implements Drupal\Core\ExtensionHandlerInterface::moduleListReset().
+   */
+  public function moduleListReset() {
+    $this->moduleList(NULL, NULL, TRUE);
+  }

Why is this not just $this->moduleList = array()? (Or NULL, or whatever.)

Using moduleList() (or getEnabledModules(), more properly) as a reset is an artifact we can and should remove.

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  public function systemList($type) {

Same note as above regarding bootstrap vs. not.

We really ought to pull themes out of this, too. Ideally they should go in their own separate object. I think the "eliminate system table" patch moves in that direction, doesn't it?

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+      foreach ($this->lists['bootstrap'] as $module) {
+        drupal_classloader_register($module->name, dirname($module->filename));
+        drupal_get_filename('module', $module->name, $module->filename);
+      }

This looks like something else that should get injected. (The class loader itself.)

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  public function systemListReset() {
+    $this->lists = NULL;
+    drupal_static_reset('system_rebuild_module_data');
+    drupal_static_reset('list_themes');
+    $this->bootstrap_cache->deleteMultiple(array('bootstrap_modules', 'system_list'));
+    $this->cache->delete('system_info');
+  }

Are the drupal_static_reset() calls still necessary?

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+          $dependency_data = drupal_parse_dependency($dependency);

The dependency parsing should get moved to an object, too, if it isn't already. Probably a follow-up, as that's likely non-trivial.

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,563 @@
+  public function moduleImplementsWriteCache() {
+    // Check whether we need to write the cache. We do not want to cache hooks
+    // which are only invoked on HTTP POST requests since these do not need to be
+    // optimized as tightly, and not doing so keeps the cache entry smaller.
+    if (isset($this->implementations['#write_cache']) && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD')) {
+      unset($this->implementations['#write_cache']);
+      $this->bootstrap_cache->set('module_implements', $this->implementations);
+    }
+  }

This method name should get verb-ified.

+++ b/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php
@@ -47,7 +48,7 @@ public function __construct($collection, $table = 'key_value') {
   public function getMultiple(array $keys) {
     $values = array();
     try {
-      $result = db_query('SELECT name, value FROM {' . db_escape_table($this->table) . '} WHERE name IN (:keys) AND collection = :collection', array(':keys' => $keys, ':collection' => $this->collection))->fetchAllAssoc('name');
+      $result = Database::getConnection()->query('SELECT name, value FROM {' . db_escape_table($this->table) . '} WHERE name IN (:keys) AND collection = :collection', array(':keys' => $keys, ':collection' => $this->collection))->fetchAllAssoc('name');

Isn't there another issue somewhere to make this injected? If not, it should be injected.

pounard’s picture

Is that really useful that the extension handler is a property of the kernel? Can it be registered directly into the DIC? It seems it implies lots of static still being in here. Just for discussion, I linked this patch into another issue: https://drupal.org/files/modules_install_list-2-do_not_test.patch which may propose some bits of alternative solution to get rid of all that statics. Because you hardcode database and cache stuff into the kernel it makes it harder to boot it earlier in the request and so harder to get rid of old school procedural bootstrap: ideally -and I think this can be achieved without sweting too much- the kernel can boot right after the autoloader has been loaded: the only chicken and egg problem being the module list.

This can be a follow up, but I don't like those statics introduced in the Kernel.

chx’s picture

FileSize
85.73 KB

As I promised, I did a reroll of the patch after the system removal. Only some of Crell's concerns are addressed. Also, there's a state() call now in systemList which is not injected but I couldn't figure that one out. Drupal\Core\KeyValueStore\DatabaseStorage is ripe for injection now 'cos I converted it to use $this->connection instead of db functions, I just dont know what to inject into in the bootstrap container. That might need to wait until we remove the bootstrap container?

I am not 100% this will pass, but it installs and passes a few tests, so let's hope.

chx’s picture

FileSize
102.84 KB

Oh doh.

Status: Needs review » Needs work

The last submitted patch, 1331486_47.patch, failed testing.

chx’s picture

I can continue tomorrow, if someone else wants to continue, all I could reproduce was an exception in system_get_info() saying Undefined index: testing which means on test installs the info cache is not wiped properly. As system_info is cleared in systemList (and that's the correct cid indeed) I am at a bit of a loss what happened. I can't find any other error much less a fatal.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.79 KB
102.85 KB

Let's try this one, it was a silly replaced-too-much error :)

Status: Needs review » Needs work

The last submitted patch, 1331486-52.patch, failed testing.

amateescu’s picture

The log is filled with this stuff, but I can't reproduce them locally:

Next exception 'Drupal\Core\Database\DatabaseExceptionWrapper' with message 'SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupaltestbotmysql.simpletest214715key_value' doesn't exist: SELECT 1 AS expression
FROM
{key_value} key_value
WHERE ( (name = :db_condition_placeholder_0) AND (collection = :db_condition_placeholder_1) ) FOR UPDATE; Array
(
    [:db_condition_placeholder_0] => system.theme.data
    [:db_condition_placeholder_1] => state
)
' in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php:532
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Select.php(420): Drupal\Core\Database\Connection->query('SELECT 1 AS exp...', Array, Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Query/Merge.php(417): Drupal\Core\Database\Query\Select->execute()
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/KeyValueStore/DatabaseStorage.php(98): Drupal\Core\Database\Query\Merge->execute()
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/system.module(3006): Drupal\Core\KeyValueStore\DatabaseStorage->set('system.theme.da...', Array)
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ExtensionHandler.php(200): system_rebuild_theme_data()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ExtensionHandler.php(117): Drupal\Core\ExtensionHandler->systemList('module_enabled')
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/ExtensionHandler.php(83): Drupal\Core\ExtensionHandler->moduleList('module_enabled')
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc(2567): Drupal\Core\ExtensionHandler->loadAll(false, true)
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(874): module_load_all(false, true)
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(824): Drupal\simpletest\TestBase->tearDown()
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(637): Drupal\simpletest\WebTestBase->tearDown()
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#12 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('539', 'Drupal\xmlrpc\T...')
#13 {main}FATAL Drupal\xmlrpc\Tests\XmlRpcMessagesTest: test runner returned a non-zero error code (1).

Maybe we need this first? #1067408: Themes do not have an installation status

chx’s picture

Status: Needs work » Needs review

#52: 1331486-52.patch queued for re-testing.

chx’s picture

It was 38,289 pass(es), 105 fail(s), and 13 exception(s)., let's see what a retest brings.

Status: Needs review » Needs work

The last submitted patch, 1331486-52.patch, failed testing.

jthorson’s picture

Missed a spot!

PHP Fatal error: Call to undefined function system_list() in .../core/includes/update.inc on line 262.

chx’s picture

Status: Needs work » Needs review
FileSize
104.12 KB

Status: Needs review » Needs work

The last submitted patch, 1331486.patch, failed testing.

chx’s picture

I ran all the aggregator tests on a bot, here are the failures:

Aggregator configuration 14 passes, 6 fails, and 0 exceptions
Import feeds from OPML functionality 50 passes, 5 fails, and 6 exceptions
Test run duration: 2 min 32 sec

wildly different from the "1 fail, go away" results. And the manual run on the bot reliably produces these fails so I will look into them now.

Edit: false alarm. These pass if I run the scripts as www-data.

chx’s picture

Status: Needs work » Needs review
FileSize
104.53 KB

Status: Needs review » Needs work

The last submitted patch, 1331486_the_grass_is_green_i_wonder_whether_this_is_too.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
104.1 KB

Status: Needs review » Needs work

The last submitted patch, 1331486_hope_dies_last.patch, failed testing.

chx’s picture

That worked, now real errors remain. The only change between #59 and #64 is that when we reset loadAll, we no longer rebuild immediately ( I could add an interdiff but that'd be hard to understand, an if is changed to an elseif, that's it).

chx’s picture

Status: Needs work » Needs review
FileSize
773 bytes
104.15 KB

Let's see this one.

Status: Needs review » Needs work

The last submitted patch, 1331486_less_exceptions.patch, failed testing.

chx’s picture

By the way, if you want to see the interdiff between #59 and #64 then see #1608842-140: Replace list of bootstrap modules, enabled modules, enabled themes, and default theme with config. Yes, this caused problems before and yes this was fixed already once.

chx’s picture

Assigned: katbailey » chx
Status: Needs work » Needs review
FileSize
3.64 KB
105.08 KB

Status: Needs review » Needs work

The last submitted patch, 1331486.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
105.6 KB
chx’s picture

Assigned: chx » Unassigned

My work here is done, I just wanted to get back where Kat was before I shattered her patch by removing {system} :)

katbailey’s picture

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

Just wanted to post an update here because I'd intended to have this rerolled by now - I've been working on it, but some of the innocuous-seeming comments in Crell's last review amount to some serious overhaul :-P And now the installer is of course giving me grief.
Anyway, I'll be back at it tonight and will hopefully be able to produce a new patch.

katbailey’s picture

Status: Needs work » Needs review
FileSize
41.12 KB
117.66 KB

Just want to see what testbot makes of this as there are a lot of changes...

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.75.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review

#75: 1331486.extension_handler.75.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.75.patch, failed testing.

donquixote’s picture

Would it bring a benefit to split hooks into their own objects?
(not saying one class per hook, just one object per hook)

E.g.

drupal_extension_handler()->hook($hook)->invokeAll($arg0, $arg1, $arg2);

There could be one class per hook signature (number of by-ref and by-value arguments, and possibly the type of return value), and then have one instance per hook. Of course, we can only take full advantage of this if we actually know the hook signature.

Currently the only benefit I see is breaking a big class into smaller ones - which can already be a good thing for itself.
It might also open up some possibility for performance gain, by eliminating some if/else, array merge, func_get_args(), by replacing call_user_func_array() with $function(), etc. This would need further investigation, obviously.

Crell’s picture

don: I don't think that buys us enough for the amount of work it would take. If someone wants to use a more powerful dispatching/notification system, they can use EventDispatcher. For now, we're just trying to clean up code and let is lazy-load, and put the caches in the right place so that we can unbork bootstrap. Heavier refactoring of hooks is, I think, unnecessary work with EventDispatcher already available. Let's stick to bootstrap unborking for now, when we are 6 weeks from feature freeze. :-(

catch’s picture

Just a note, while I'd love this patch to land as soon as possible, it's going to be completely valid until code freeze, not feature freeze.

Crell’s picture

Sure, but my concern is that feature-sensitive patches get held up by the bootstrap fugly that this patch helps to address.

catch’s picture

Should we bump it to critical then?

katbailey’s picture

Status: Needs work » Needs review
FileSize
7.12 KB
123.91 KB

Now to address Crell's review above (#45):

Are the drupal_static_reset() calls still necessary?

This patch doesn't remove system_rebuild_module_data() or list_themes() so it seems we still need those, yes.

The dependency parsing should get moved to an object, too, if it isn't already. Probably a follow-up, as that's likely non-trivial.

Seems it was fairly trivial after all, unless I'm missing something...

As for untangling the existing logic of moduleList and friends, I created a follow-up here: #1815610: Untangle the logic of moduleList() and systemList() in the ExtensionHandler.
See this interdiff and the previous one for how I've addressed the other points.

Oh, and I don't know what's with the random test failures - possibly a problem in WebTestBase. I'll see how this one fares and then look into it.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.84.patch, failed testing.

katbailey’s picture

Hmm, if no other patches are exhibiting this strange behavior then there must be a problem in WebTestBase or somewhere. Getting to the bottom of that is going to be fun... :-(

katbailey’s picture

Status: Needs work » Needs review
FileSize
123.95 KB

Just want to see if this solves the rogue test failure problem...

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.87.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
123.96 KB

Just a little offset.

Status: Needs review » Needs work

The last submitted patch, 1331486_89.patch, failed testing.

katbailey’s picture

Just a reroll against 8.x - also, I've pushed a branch for this to the wscci sandbox just for collaboration purposes because patches are dumb:
http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/modu...
If anyone else wants to collaborate on this, please use the sandbox, but ping me if you're rebasing the branch (as in deleting and recreating).

katbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.91.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
124.36 KB

Hackety-hack on the stupid WebTestBase tearDown() - ugh.

Crell’s picture

+++ b/core/lib/Drupal/Core/ExtensionHandler.php
@@ -0,0 +1,776 @@
+    if (isset($this->implementations['#write_cache']) && ($_SERVER['REQUEST_METHOD'] == 'GET' || $_SERVER['REQUEST_METHOD'] == 'HEAD')) {

We shouldn't be using $_SERVER. That information is provided in the Request object. Which means... ugh, does that mean extension handler depends on the request??

I'm a bit concerned that the Extension class is so huge. That feels wrong, and I think we still need to split modules from themes in this regard. However, I'm comfortable making that a follow-up at this point.

The changes in the KeyValueStore are going to conflict with, or maybe duplicate, #1809206: KeyValueFactory hard-codes DatabaseStorage. I defer to Dratchick on how we want to deal with that race condition. :-)

I think this is really close to commitable, even though there's follow-up work.

katbailey’s picture

We shouldn't be using $_SERVER.

Heh, another great example of the lazy-ass copy/paste approach I've taken to this patch ;-) I've fixed this - see interdiff.

I'm a bit concerned that the Extension class is so huge.

Yeah, this thing will definitely need further refactoring, but it would be great to get it in sooner rather than later because of how it affects the whole bootstrap problem.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.96.patch, failed testing.

chx’s picture

Some observations: [20:35:01] Review host: (drupaltestbot659-mysql). Checking tailing the Apache error log on 659 shows no errors between 20:01:51 and 22:00:21 and the mysql error is not available. Will try to get hold of that.

Berdir’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.phpundefined
@@ -36,6 +49,22 @@ public function init() {
   /**
+   * Overrides Kernel::boot().
+   */
+  public function boot() {
+    // Instantiate an ExtensionHandler which the Kernel itself needs in order to
+    // find out which modules are enabled. In the buildContainer() method we
+    // register this and the database connection we pass to it as synthetic
+    // services to the container so that they do not need to be instantiated
+    // over again.
+    $this->keyValue = new KeyValueFactory();
+    $class_loader = drupal_classloader();
+    $this->extensionHandler = new ExtensionHandler($this->keyValue, CacheFactory::get('cache'), CacheFactory::get('bootstrap'), $class_loader);
+    parent::boot();
+    drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);

Comment talks about database connection but it's actually creating a keyvaluefactory? I assume the database connection will only happen within that, once accessed?

Also wondering how that will play together with having the database in the DIC too, we'll see about that :)

Because my issue where I'm working on that does inject the database info as an argument to Database() and actually instantiates that, instead of parsing global variables.

I guess that together with HttpCache wrapping the Kernel, wie can still avoid connecting to the database completely if we have non-database cache/state backends? Considering that cache should live in the container itself too.

Not sure how all these pieces will fit together...

katbailey’s picture

Status: Needs work » Needs review
FileSize
12.46 KB
129.38 KB

<sarcasm>Well, that was fun. </sarcasm>

Anyway, I finally got this working with a compiled DIC. I know the patch won't be green, because there are 2 fails that I already know about, not to mention the random fails I haven't figured out yet, but just wanted to see if testbot has any other surprises for me...

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.100.patch, failed testing.

katbailey’s picture

Ugh, apart from two of the DrupalKernelTest fails, I haven't found any others that I can reliably reproduce. And I still don't understand the problem with the DrupalKernelTest fails. In the meantime I think that moving the dumping of the container to after we bootstrap code might help.
I guess I'm running into the race conditions that sun keeps talking about :-/

katbailey’s picture

Status: Needs work » Needs review
Berdir’s picture

Will try to reproduce when I find the time. Did you run it in the UI or with run-tests? Maybe it doesn't work with run-tests.sh because there's no global kernel, I've had problems with that too in other issues.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.102.patch, failed testing.

katbailey’s picture

OK, now I just want to smash something :-(

Berdir’s picture

I can confirm that I can reproduce these random failures when running run-tests.sh with a concurrency of 8 but I haven't gotten much further.

I'm not sure how exactly we're trying to delete these service container php storage directories but I guess it might be that when we running into some sort of unexpected state when multiple instances are dumping containers?

katbailey’s picture

w00t! #1784312: Stop doing so much pre-kernel bootstrapping just got in - that contained the whole unborking the bootstrap piece that I originally started working on in this issue. So now this issue can just focus on the ExtensionHandler, will try to get a new patch up today.

katbailey’s picture

This patch is not fit for consumption by testbot. For starters it is rolled on top of #1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths. It is functional, but probably fairly riddled with holes. The main differences between it and its predecessors are:

  • The ExtensionHandler gets the list of enabled modules injected into it from a container parameter
  • The ExtensionHandler deals only with modules, so system_list() has been left in place to deal with themes.

I just wanted to put it up because I won't be around over the weekend and thought people might be wondering where I'm at with it seeing as it's been a while.

andyceo’s picture

Status: Needs work » Needs review

Invoking bot.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.109.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
110.27 KB

Well, if we want the bot to look at it we need to do a combined patch with #1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.112.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
110.27 KB

Hmm, might get further with this one...

Berdir’s picture

Status: Needs review » Needs work

@katbailey if you don't want to send a patch to the bot, use do-not-test.patch, otherwise, all patches will be sent to the bot once the issue is set to needs review :)

Had a quick look through it, nice that this can now actually focus on what it's supposed to do and not do 10 other things that are required first. Much easier to review :)

Just one question at the moment...

+++ b/core/includes/bootstrap.incundefined
@@ -2460,6 +2473,92 @@ function drupal_container(Container $new_container = NULL) {
 /**
+ * Retrieves the ExtensionHandler that manages the list of enabled modules.
+ *
+ * This is a temporary function that will instantiate a new ExtensionHandler if
+ * there isn't one present in the container. This will only happen if the
+ * boostrap container is being used, e.g. during installation.
+ * @todo Remove this and convert all calls to it to
+ *  drupal_container()->get('extension_handler') once the installer and the
+ *  run-tests.sh script are using a Kernel.
+ *
+ * @return Drupal\Core\ExtensionHandlerInterface
+ */
+function drupal_extension_handler() {

That happend, shouldn't that mean that this function is no longer necessary?

(Not sure if you just didn't get to remove it yet or if there's a reason it's still in here).

Berdir’s picture

Status: Needs work » Needs review

Crosspost.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.114.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
6.43 KB
106.05 KB

Figured I'd at least throw up a rebased patch now that #1849700: DrupalKernel's container.modules param should contain module filenames, not full namespace paths is in. I have fixed some tests too, although testbot can't even get to the point of running tests it seems and I don't understand what's happening there. Installation works fine locally for me, as does enabling modules, and running tests using run-tests.sh. Having trouble with the module enable/disable tests, but I think it will be easy enough to figure out what's going wrong there.

@berdir:

shouldn't that mean that this function is no longer necessary

Actually I think I still need it, because we need to be able to use a minimal extension handler (one that doesn't attempt to talk to the db) in certain situations (installer, tests), although it is quite possible we could come up with a way around it. I certainly need to update the docblock at least though :-/

Basically, there is still a lot of work to do on this patch, but figuring out why testbot is choking on it is probably as important a piece as any...

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.118.patch, failed testing.

effulgentsia’s picture

We certainly are in no need of any more justification for this issue, but just as an FYI, I opened #1859684: Expand routeBuilder unit test and postponed it on this issue. We probably have a lot of other similar examples.

catch’s picture

Priority: Major » Critical

I'm going to bump this to critical. There's a lot of one-off event listeners cropping up in patches which could just as well be hooks if we'd got this done, and it's necessary to remove DRUPAL_BOOTSTRAP_CODE. #1860026: Remove hook_exit() for cached page requests is a good example of where we're re-implementing exactly the same functionality in two different places with two different systems.

katbailey’s picture

Assigned: katbailey » Unassigned
Status: Needs work » Needs review
FileSize
15.95 KB
113.89 KB

Well, I tried to get rid of the drupal_extension_handler() wrapper function and just use drupal_container()->get('extension_handler') - installation worked fine without it, but tests exploded. I've changed the docblock, leaving a todo to figure this out.

This version of the patch also eliminates module_list() entirely.

At this point, without understanding what exactly is going wrong when testbot tries to enable simpletest module, I have no idea how to move this along, so I'm hoping someone else can help.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.122.patch, failed testing.

pounard’s picture

+function drupal_extension_handler() {
+  $extension_handler = &drupal_static(__FUNCTION__, NULL);
+  if (isset($extension_handler)) {
+    return $extension_handler;
+  }
+  if (drupal_container()->has('extension_handler')) {
+    $extension_handler = drupal_container()->get('extension_handler');
+  }
+  else {
+    $extension_handler = new ExtensionHandlerMinimal();
+  }
+  return $extension_handler;
+}

This function is useless and adds one level of indirection in legacy wrappers. Plus it caches something that is already referenced in the container. You probably should drop it. I might be wrong, but the sole use case I see it usefull is for downgrade (the ExtensionHandlerMinimal instance) but I think the code for which this is targeted should deal with the exception, and not the normal runtime code.

katbailey’s picture

@pounard, I totally agree - read my first paragraph in #122.

pounard’s picture

Oh right, I missed it, I skipped directly to the patch review. Sounds odd.

katbailey’s picture

Status: Needs work » Needs review
FileSize
576 bytes
113.94 KB

HUGE thanks to chx and jthorson for pinpointing the problem that testbot was having. Turned out I had broken drupal_get_filename() - see https://twitter.com/chx/status/277634267947859968 :-D Fun.

This patch *should* get us to the point of seeing how many test failures there are - I have no doubt it will be a crap-ton, but that would still be progress...

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.127.patch, failed testing.

Dave Reid’s picture

+  /**
+   * Implements Drupal\Core\ExtensionHandlerInterface::moduelList().
+   */
+  public function getEnabledModules() {
+    return $this->moduleList;
+  }
+
+  /**
+   * Implements Drupal\Core\ExtensionHandlerInterface::setModuleList().
+   */
+  public function setModuleList(array $module_list = array()) {
+    $this->moduleList = $module_list;
+  }

This section looked odd/wrong. getEnabledModules() vs the converse setModuleList() - doesn't match up. modulelList() [sp] vs getEnabledModules().

katbailey’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
113.94 KB

So, drupal_alter was borked - specifically for multi-hook calls (form_alter, form_FORM_ID_alter, etc) - I'm thinking that will account for a lot (a majority even?) of the fails.
That's the only change I'm making in this new patch as I am anxious to see how far it gets us towards green.
@Dave yeah, it should just be getModuleList and setModuleList - I will change that in the next reroll.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.130.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
25.4 KB
118.34 KB

This last reroll was a bit hairy so I'm hoping I haven't broken more tests than I fixed. I also renamed getEnabledModules() to getModuleList() (and that change constitutes most of the interdiff so it'll be hard to see the other changes).

One test I am battling with is BareMinimalUpgradePathTest - it has this weird Heisenbug thing going on where if I put debug statements in certain places the test passes, but otherwise it fails because somehow the list of module filenames gets corrupted such that action module ends up with image module's filename. Fun. Anyway it only accounts for 1 fail so I guess I'll deal with all the others first and then get back to it.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.132.patch, failed testing.

Sylvain Lecoy’s picture

I would move and rename the ExtensionHandlerMinimal class to:

DRUPAL_ROOT . /tests/Drupal/Core/Tests/Extension/ExtensionHandlerStub

@see #1872006: Add parity test classes for lib/Core and lib/Component for the idea.

chx’s picture

Using debug during update is not a good idea; write to a file instead or use a debugger. Update AFAIK has its own error handler and I have seen it doing the weirdest things and debug works by trigger_error().

katbailey’s picture

Status: Needs work » Needs review
FileSize
13.3 KB
129.49 KB

This should fix the DUTB tests and is largely thanks to chx. DUTB tests that installed system module after a bunch of other modules were enabled by the parent's setUp() method resulted in the enabled modules config getting wiped (not sure why this problem was only exposed by this patch).

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.136.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
26.78 KB
129.72 KB

Before tackling the rest of the tests I really do want to get rid of the horrible drupal_extension_handler() wrapper, so let's see how this one fares...

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.138.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
4.55 KB
132.31 KB

One step forward, two steps back... :-(

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.140.patch, failed testing.

Wim Leers’s picture

#140: Seems like it's still another step forward! Very impressive work here! :)

katbailey’s picture

Status: Needs work » Needs review
FileSize
19.53 KB
139.4 KB

Rather than *merely* being a slave to testbot, I added an actual important improvement in this latest version: got rid of module_load_all(), which currently performs 4 different functions. I replaced it with 4 different methods on the ExtensionHandler class - loadAll, loadBootstrap, isLoaded and reloadModules.

Who knows what the net effect will be, test-wise, though I did also add some fixes to the ModuleAPI and Dependency tests.

(Thanks for the words of encouragement, Wim :))

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.143.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
138.73 KB

I had been ignoring the upgrade path tests for the last while because they were so difficult to get to the bottom of (the heisenbug mentioned in #132) - apparently they were still a problem in the last patch, but after rebasing and doing some minor cleanup which shouldn't really affect anything, I am getting them all passing locally. Posting a new patch to see if the problem has miraculously gone away.

Also, I really want to try and get this green this weekend - I'm using a branch in the wscci sandbox in the hope that I might get some collaborators to help with the remaining tests: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/modu.... Just post a comment here if you are interested in helping and say which tests you are focusing on.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.145.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
3.27 KB
140.49 KB

We *should* now be down to just the 2 ContextualDynamicContextTest failures, which I can't for the life of me figure out but are probably due to something totally stupid, and the ThemeRegistry test, which beejeebus said he might be able to look into today (and hopefully the exception in ThemeTest is related).

David_Rothstein’s picture

The ExtensionHandler deals only with modules, so system_list() has been left in place to deal with themes.

Given that, does it make sense to still call it "ExtensionHandler"? If we're adding new terminology, it should mean something new also.

I think it could work (based on the potential for future additions of non-module things to the same system), but if so, I think it's important to make sure (in this patch) that all methods which are specific to modules have the word "module" somewhere in the method name, or otherwise things will get very very confusing. For example:

+  public function loadAll() {
...
+  public function loadAllIncludes($type, $name = NULL) {

This really leaves me wondering whether or not these methods are supposed to work for things other than modules.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.147.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
142.73 KB

Fixes the ThemeRegistry fail and the ThemeTest exception. The slippery upgrade path test fail is back, more slippery than ever because I can't even get it to fail locally now. Also, MTimeProtectedFastFileStorageTest passes for me locally :-/

Re #148 - agreed, I will change the method names to include the word module once the patch is green.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.150.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.150.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review

the contextual links test fails because we haven't loaded contextual.module in to memory before _theme_process_registry() for node_theme runs.

so, although 'contextual' is checked as a candidate prefix in _theme_process_registry(), function_exists() fails:

          foreach ($prefixes as $prefix) {
            // Only use non-hook-specific variable processors for theming hooks
            // implemented as templates. See theme().
            if (isset($info['template']) && function_exists($prefix . '_' . $phase)) {    <--- this fails with patch
              $info[$phase_key][] = $prefix . '_' . $phase;
            } 
            if (function_exists($prefix . '_' . $phase . '_' . $hook)) {
              $info[$phase_key][] = $prefix . '_' . $phase . '_' . $hook;
            } 
          } 
Anonymous’s picture

so #154 is rubbish. the problem is this in _theme_process_registry():

          // HEAD
          if ($type == 'module') {
            // Default variable processor prefix.
            $prefixes[] = 'template';
            // Add all modules so they can intervene with their own variable
            // processors. This allows them to provide variable processors even
            // if they are not the owner of the current hook.
            $prefixes += module_list();
          }

         // PATCH
          if ($type == 'module') {
            // Default variable processor prefix.
            $prefixes[] = 'template';
            // Add all modules so they can intervene with their own variable
            // processors. This allows them to provide variable processors even
            // if they are not the owner of the current hook.
            $prefixes += array_keys(drupal_container()->get('extension_handler')->getModuleList());
         }

head returns an array keyed by module name, array_keys doesn't. so $prefixes[] = 'template'; causes the first module to be discarded. nice and quiet like. the fix is just:

            $prefixes = array_merge($prefixes, array_keys(drupal_container()->get('extension_handler')->getModuleList()));
Anonymous’s picture

attached patch with change from #155.

Status: Needs review » Needs work

The last submitted patch, 1331486-156-extension_handler.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
142.98 KB

sigh. so we had a variable called $m, and we used a variable called $module. renamed it so we might not do that again. and because i wanted more than 5 characters for the last 2 hours of effort :-\

diff --git a/core/includes/module.inc b/core/includes/module.inc
index 3f9de34..a8be3ee 100644
--- a/core/includes/module.inc
+++ b/core/includes/module.inc
@@ -305,9 +305,9 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
 
       $sorted_modules = $module_config->get('enabled');
       $sorted_with_filenames = array();
-      foreach (array_keys($sorted_modules) as $m) {
-        $filename = isset($module_filenames[$m]) ? $module_filenames[$m] : drupal_get_filename('module', $module);
-        $sorted_with_filenames[$m] = $filename;
+      foreach (array_keys($sorted_modules) as $sorted_module) {
+        $filename = isset($module_filenames[$sorted_module]) ? $module_filenames[$sorted_module] : drupal_get_filename('module', $sorted_module);
+        $sorted_with_filenames[$sorted_module] = $filename;
       }

will the bot have pity on us?

katbailey’s picture

@beejeebus I owe you a very large whiskey. Thank you so much for figuring this out. (To think it was down to such sloppiness on my part... ugh.)

I've made the change suggested by David in #148, i.e. including the word "module" in each of the method names for clarity.

Also reverted a bad "fix" I had made to BundleTest - the real fix was done later on and the change here served only to cover up the underlying problem.

aspilicious’s picture

+++ b/core/lib/Drupal/Core/ExtensionHandler.phpundefined
@@ -0,0 +1,539 @@
+ * Definition of Drupal\Core\ExtensionHandler.

Contains ...

+++ b/core/lib/Drupal/Core/ExtensionHandler.phpundefined
@@ -0,0 +1,539 @@
+class ExtensionHandler implements ExtensionHandlerInterface {

Can use a docblock

+++ b/core/lib/Drupal/Core/ExtensionHandler.phpundefined
@@ -0,0 +1,539 @@
+   * Constructor.

I'm not sure this is the correct docs block for constructors. You should check node/1354

+++ b/core/lib/Drupal/Core/ExtensionHandler.phpundefined
@@ -0,0 +1,539 @@
+   * Implements Drupal\Core\ExtensionHandlerInterface::loadModule().

\Drupal\Core...

msonnabaum’s picture

First time looking at this patch, couple of quick observations.

If ExtensionHandler depends on a state k/v object, that's what it should get. ExtensionHandler doesn't need to know how to get a state object from a KeyValueFactory. If this is being injected as a dependency, we probably need a State object.

/**
 * Class representing an ExtensionHandler that tests can use.
 *
 * Overrides all methods in the ExtensionHandler class that attempt to talk to
 * a database.
 */
class ExtensionHandlerMinimal extends ExtensionHandler {

The comment clearly describes what this classes intention is, but the name does not. Can we come up with an intention-revealing name for this class?

Also, we're calling these classes ExtensionHandler*, yet all but one method has the "module" noun in it. This is perhaps a sign that this interface has too much responsibility or is not appropriately named. ExtensionHandler->loadAllModules() is not as clear as ModuleHandler->loadAll().

Berdir’s picture

If ExtensionHandler depends on a state k/v object, that's what it should get. ExtensionHandler doesn't need to know how to get a state object from a KeyValueFactory. If this is being injected as a dependency, we probably need a State object.

Agree, that's a general problem, not just with state, but all services that come from a factory. Currently, we don't have a 'state' service, we just have a keyvalue services with a state collection. Not sure about a general solution though, for frequently used things, we can add it directly to the container, For state, it will be "solved" as a side effect of #1786490: Add caching to the state system.

msonnabaum’s picture

Well, if it's not possible to pass "state" as an argument to the k/v factory when defining the ExtensionHandler service (I have nfi if that is possible), then we just need to create a new State class.

Berdir’s picture

We can pass state as an argument if we define it explicitly in the container. That is easily possible with factoryService + factoryMethod. And the caching issue does this already. However, it doesn't really scale because every argument needs to be defined like that in the container (e.g. config objects).

g.oechsler’s picture

Status: Needs review » Needs work

I took time to review the whole thing. I could not find too much to complain about. So for the rest of this comment: nitpickery++!

--- a/core/includes/bootstrap.inc
+++ b/core/includes/bootstrap.inc
@@ -2460,6 +2475,45 @@ function drupal_container(Container $new_container = NULL) {
+function module_implements($hook) {
+function module_invoke_all($hook) {
+function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+function module_exists($module) {

I'm not entirely sure, but it feels like these functions are deprecated in favor to their ExtensionHandler method equivalent. Maybe this should be stated in the respective docblocks.

--- a/core/includes/module.inc
+++ b/core/includes/module.inc
@@ -509,21 +300,35 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
-      // Update the kernel to include it.
+
+      // Update the kernel to include the new module

Gotcha, missing punctuation. ;)

--- a/core/includes/schema.inc
+++ b/core/includes/schema.inc
@@ -79,13 +79,13 @@ function drupal_get_complete_schema($rebuild = FALSE) {
-      if (function_exists('module_load_all_includes')) {
+      if (function_exists('system_list_reset')) {
...
         system_list_reset();
-        module_load_all_includes('install');
+        drupal_container()->get('extension_handler')->loadModuleIncludes('install');
       }

Before we were relying on system_list_reset() to be there anyway - now we check on it. So the code before was either terribly broken or this if-statement is obsolete.

--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -400,7 +400,7 @@ function system_requirements($phase) {
     );
 
     // Check installed modules.
-    foreach (module_list() as $module) {
+    foreach (drupal_container()->get('extension_handler')->getModuleList() as $module => $filename) {
       $updates = drupal_get_schema_versions($module);
       if ($updates !== FALSE) {
         $default = drupal_get_installed_schema_version($module);

Bonus nitpick: When looking at the full context of this, you'll find that $filename is not used in the loop. This can be found a few times in the patch. Surely we could go with foreach (array_keys(...) as $module) {...} but I doubt that it will make the code any more readable.

chx’s picture

> Before we were relying on system_list_reset() to be there anyway - now we check on it. So the code before was either terribly broken or this if-statement is obsolete.

Neither, before we checked whether module.inc was ever included by checking on one function in that file, now that things got moved around we check on another.

katbailey’s picture

Status: Needs work » Needs review
FileSize
69.82 KB
142.74 KB

OK, this takes care of the class/method naming issues, hopefully to everyone's satisfaction: using ModuleHandler instead of ExtensionHandler and removing "module" from most of the method names (I left getModuleList and setModuleList because meh).

Also added the 'state' service, I think it's perfectly ok to special case this particular k/v collection.

Took care of the nitpicks too.

When looking at the full context of this, you'll find that $filename is not used in the loop

Does that really matter? I'd prefer to leave it as is but if it really needs to be changed to use array_keys() I don't feel that strongly about it. Have left it as is for now.

it feels like these functions are deprecated in favor to their ExtensionHandler method equivalent. Maybe this should be stated in the respective docblocks.

Well, I haven't converted all of core to use the ExtensionHandler method equivalents because I wanted to keep the patch size down and keep it relatively easy to rebase. Not sure what the rule is about deprecating functions like this. Anyone else know?

tim.plunkett’s picture

I agree with not using the array_keys() for that, it's an extra function call for only marginally better readability.

As far as the procedural functions, we now have #1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function, so you can put @deprecated on them until the conversion follow-up is done, hopefully mitigating any further additions of them to core.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.167.patch, failed testing.

Crell’s picture

Status: Needs work » Needs review

There's discussion of allowing @deprecated in docblocks, which looks like it will happen: #1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function I'd say go ahead and use 'em.

I think the foreach() is fine as is; there's a couple places we do that, and since there's no performance difference either way as far as I know, meh.

Further comments, all should be relatively minor:

+++ b/core/includes/bootstrap.inc
@@ -2476,6 +2491,45 @@ function drupal_container(Container $new_container = NULL) {
+function module_invoke_all($hook) {
+  $args = func_get_args();
+  // Remove $hook from the arguments.
+  unset($args[0]);
+  return drupal_container()->get('module_handler')->invokeAll($hook, $args);
+}

I don't know if it matters that this leaves the array 1-based, not 0-based. I think we normally array_shift() here to avoid that. (If it works, though, I'm fine to ignore it since we'll be removing this function anyway.)

+++ b/core/lib/Drupal/Core/InstallModuleHandler.php
@@ -0,0 +1,141 @@
+        // Since module_hook() may needlessly try to load the include file again,
+        // function_exists() is used directly here.

Comment should be updated, since I presume module_hook() is now another method here.

+++ b/core/lib/Drupal/Core/InstallModuleHandler.php
@@ -0,0 +1,141 @@
+          module_load_include('inc', $module, "$module.$group");

Shouldn't module_load_include() also turn into a method, rather than a function?

+++ b/core/lib/Drupal/Core/ModuleHandler.php
@@ -0,0 +1,531 @@
+      module_load_include($type, $module, $name);

As above, I think module_load_include() should move into this class. And its parameter order should change to not be stupid. :-)

+++ b/core/lib/Drupal/Core/ModuleHandlerInterface.php
@@ -0,0 +1,201 @@
+  /**
+   * Returns whether all modules have been loaded.
+   *
+   * @return bool
+   *   A Boolean indicating whether all modules have been loaded. This means all
+   *   modules; the load status of bootstrap modules cannot be checked.
+   */
+  public function isLoaded();

isLoaded() to me suggests "is this object loaded", not "have we loaded all the things". That may be too subtle a distinction here, but the method name seems odd to me.

+++ b/core/lib/Drupal/Core/ModuleHandlerInterface.php
@@ -0,0 +1,201 @@
+   * @param $files
+   *   The array of filesystem objects used to rebuild the cache.

What's a file system object? SplFile? That would be my guess, but it could also be an array structure of some sort. Doc needs improvement.

+++ b/core/lib/Drupal/Core/ModuleHandlerInterface.php
@@ -0,0 +1,201 @@
+  public function buildModuleDependencies($files);

$files should be type hinted to array for completeness.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
@@ -76,11 +85,9 @@ public function rebuild() {
     // We need to manually call each module so that we can know which module
     // a given item came from.
-    // @todo Use an injected Extension service rather than module_list():
-    //   http://drupal.org/node/1331486.
-    foreach (module_list() as $module) {
+    foreach ($this->ModuleHandler->getModuleList() as $module => $filename) {

This is probably a follow-up, but this "we need to know which module said what" problem comes up a couple of times. We ought to have an invokeAll() version that does track that for us. Probably not worth doing here unless it's really easy to do, but something to think about for later.

katbailey++!

effulgentsia’s picture

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.167.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#1356170: Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep() changed the following:

@@ -921,7 +922,7 @@ function module_hook_info() {
         if (function_exists($function)) {
           $result = $function();
           if (isset($result) && is_array($result)) {
-            $hook_info = array_merge_recursive($hook_info, $result);
+            $hook_info = NestedArray::mergeDeep($hook_info, $result);
           }
         }
       }
@@ -1002,7 +1003,7 @@ function module_invoke_all($hook) {
     if (function_exists($function)) {
       $result = call_user_func_array($function, $args);
       if (isset($result) && is_array($result)) {
-        $return = array_merge_recursive($return, $result);
+        $return = NestedArray::mergeDeep($return, $result);
       }
       elseif (isset($result)) {
         $return[] = $result;

with use Drupal\Component\Utility\NestedArray; at the top

katbailey’s picture

Thanks @tim!

Latest patch:

  • adds @deprecated to the procedural wrappers
  • converts module_hook into the implementsHook method in the ModuleHandler class because I had left it out inadvertently
  • removes getHookInfo from the interface as it is only used internally
  • adds a loadInclude() method BUT I have to leave the module_load_install and module_load_include functions in place because the ModuleHandler currently only deals with enabled modules, but these functions know how to load the files of disabled modules. It just needs some thinking through but I'd rather leave that for a follow-up if that's ok. Added a todo in module_load_include()
  • changes the parameter name in buildDependencies from $files to array $modules (and adds a better explanation of what it is) as well as changing all the local variable names in there as they were very confusing.
  • takes care of the rest of the outstanding nitpicks I think

Let's see what testbot thinks...

msonnabaum’s picture

Naming changes look very good to me.

Crell’s picture

The interdiff from #174 looks OK to me.

Status: Needs review » Needs work

The last submitted patch, 1331486.extension_handler.174.patch, failed testing.

tim.plunkett’s picture

The fatal is:

exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "module_handler" does not exist.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:799
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php(423): Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition('module_handler')
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc(2553): Symfony\Component\DependencyInjection\ContainerBuilder->get('module_handler')
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(602): module_hook('system', 'schema')
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/schema.inc(284): module_invoke('system', 'schema')
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/schema.inc(223): drupal_get_schema_unprocessed('system')
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Cache/DatabaseBackendUnitTest.php(39): drupal_install_schema('system')
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.php(116): Drupal\system\Tests\Cache\DatabaseBackendUnitTest->setUpCacheBackend()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(702): Drupal\system\Tests\Cache\GenericCacheBackendUnitTestBase->setUp()
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(381): Drupal\simpletest\TestBase->run()
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('59', 'Drupal\system\T...')
#10 {main}FATAL Drupal\system\Tests\Cache\DatabaseBackendUnitTest: test runner returned a non-zero error code (1).
katbailey’s picture

This is now completely broken since #1792536: Remove the install backend and stop catching exceptions in the default database cache backend went in :-(
I just spent hours fixing the installer only to find WebTests are OOMing (somewhere in list_themes() as far as I can tell).
Not putting up a new patch in that state but the fix for the installer is in the branch in the sandbox in case anyone else feels like going nuts: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/modu...

sun’s picture

The changes visible in the sandbox commit are hinting at the topic of: Installer + System module.

#1798732: Convert install_task, install_time and install_current_batch to use the state system streamlines and removes a lot of special-casing that's currently going on there, and based on your debugging commit, you seem to struggle with exactly that (and this is not the only issue that runs into the problem). So it would probably make sense to get that patch over there finally in.

Aside from that, I briefly glanced over this patch. Looks relatively good. I'm not sure what the overall status is, but I can try to do a more in-depth review over the weekend and help to complete and polish it.

Also, there's one particular change contained, which I don't really like though, and it's unrelated to this issue here: The introduction of DrupalWebTestBase::$install (next to ::$modules). If that's required for this patch for some reason, then we should move it into a separate issue and discuss the changes first.

Crell’s picture

Issue tags: +Avoid commit conflicts

Tagging so we don't break it again.

katbailey’s picture

Issue tags: -Avoid commit conflicts

Thanks @sun - appreciate anything you can do to help move this along. I must say the idea of blocking it on another issue sickens me though - it's been so painful to chase head on and it was green just the other day.

To explain the change to DUTB, i.e. the addition of the $install property, this was necessary for scenarios where system module was being enabled in a DUTB test using $this->enableModules() in setUp() when the test's parent class had already enabled some modules in the same way. The act of installing system module wipes the system.module.enabled config (during config_install_default_config()) so the module handler loses the previously installed modules. So with this change, we can at least make sure that if system module is going to get installed, this will happen before any other module gets installed.

I can understand your wanting this particular change to be pulled out into a separate issue, but if you could accept it (or some version of it) as part of this patch, it would be great to avoid stalling the progress here.

katbailey’s picture

Issue tags: +Avoid commit conflicts

Oops

sun’s picture

FWIW... I've merged this patch on top of #1798732: Convert install_task, install_time and install_current_batch to use the state system, and the installer seems to work pretty fine with that. Sent it for testing in http://drupal.org/node/1766036#comment-6933768

sun’s picture

So applying this patch on top of #1798732: Convert install_task, install_time and install_current_batch to use the state system gets us back to 1 fail, and 201 exceptions.

Note: I reverted most of @katbailey's latest commit during the course of that merge, since the commit tried to work around the actual cause/problem of System module not getting installed correctly.

I'll have to look into the exceptions, which should no longer occur with the installer-services patch, so they must be caused by some legacy/BC code of the new code here. I'd recommend to get the installer-services patch in ASAP, so we can move on here. Since I already merged these two monsters, we can extract the necessary conflicts/changes from there.

katbailey’s picture

Status: Needs work » Needs review
FileSize
30.64 KB
146.62 KB

I decided to spend some time reworking the ModuleHandler classes (based on some discussion with msonnabaum) - there was a lot of duplicated code between the InstallModuleHandler and the regular ModuleHandler. At first I tried to use a CacheDecorator to wrap a basic ModuleHandler with no knowledge of caching, but that seems to be fairly impossible mainly due to the way the hook implementations are incrementally discovered. I couldn't come up with a satisfactory set-up using a decorator anyway. So now I've made a CachedModuleHandler that extends the ModuleHandler, and the latter is used during installation. It tidies up the methods quite a bit.

I'm leaving my fix for the installer in place because I am very resistent to the idea of holding this up because of installer lameness.

I still haven't figured out the problem with the db backend cache test, but want to at least find out if there are any more fails now.

sun’s picture

Hm. Why didn't you simply inject the Cache\MemoryBackend and KeyValue\MemoryStorage?

That's what we're doing with everything else in the installer and really the most simple way.

Status: Needs review » Needs work

The last submitted patch, 1331486.module_handler.186.patch, failed testing.

katbailey’s picture

Why didn't you simply inject the Cache\MemoryBackend and KeyValue\MemoryStorage?

Because nobody suggested it and I didn't think of it :-P Actually I quite like having the caching logic separated out from the main logic of the module handler, but maybe your suggestion is more consistent with what we're doing elsewhere so I don't feel that strongly about it. Anyone else care to weigh in?

I'll try and fix that pesky test.

katbailey’s picture

Status: Needs work » Needs review
FileSize
783 bytes
147.38 KB

We can't call drupal_install_schema() without a 'module_handler' service having been registered to the container.

msonnabaum’s picture

I agree that the current design with the caching logic separated is much better. It's intent is clearer and it decomposes some of those methods that were huge to begin with.

Status: Needs review » Needs work

The last submitted patch, 1331486.module_handler.190.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review
FileSize
591 bytes
147.96 KB

Doh! I missed that exception before. Easy fix...

chx’s picture

Status: Needs review » Reviewed & tested by the community

Rerolling this monster should stop.

However, the following minor things needs to be done in a followup:

drupal_container()->get('module_handler')->setModuleList($module_list); and load() should chain

drupal_load runs drupal_container()->get('module_handler') twice

the doxygen drupal_container()->get('module_handler')->alter($hook). is incorrect it's >alter($type, $data, $context1, $context2);

->addArgument('%container.modules%') we need to give katbailey an award for this

information discovered during a Drupal\Core\SystemListing scan.

needs to be \Drupal.

loadInclude could use a more modern syntax that doesnt make me go bonkers

chx’s picture

Anyone else reviewing this: consider we are at #195 and the patch works and does not eat babies. Is there anything that really needs to be done and can't be done in a followup? This is the one of the most important patches in bringing sanity to the module data build scenarios -- currently Drupal *only* works because we fill modules in first in system_list and themes second. You change that, it breaks. That's madness.

sun’s picture

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

FileSize
147.84 KB

First of all, re-rolled against HEAD; didn't apply anymore (and the branch in the wscci sandbox doesn't contain latest changes).

Status: Needs review » Needs work

The last submitted patch, module.handler.197.patch, failed testing.

sun’s picture

Just a quick note: Still working on this. The current patch contains some logic that isn't right. I've resolved it for the most part, but I'm still debugging remaining problems. Need to run now, but will follow up with a new patch tonight. Will also look into those two test failures.

chx’s picture

Status: Needs work » Reviewed & tested by the community

Thanks for fixing the new failures! However, deciding what logic is right, I feel the need to remind you of #1784312-52: Stop doing so much pre-kernel bootstrapping. It's not just me, again, who is worried what is happening to this patch. Also, you still did not add back all the bugfixes you removed from the {system} removal patch despite I filed and assigned them to you close to half a year ago. Let's not repeat that here, okay?

chx’s picture

Status: Reviewed & tested by the community » Needs work

Eh, it's CNW for the bot fails.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
750 bytes
0 bytes

I fixed the two tests. What DrupalUnitTestBase is doing probably falls under the "not right" category but that's regardless of this patch and definitely a separate issue -- how can you enable a module first and install it later? Boggles the mind.

plach’s picture

@chx:

The patch is empty :)

chx’s picture

FileSize
0 bytes

Second attempt.

chx’s picture

FileSize
3.46 KB
150.21 KB

Blargh. (this patch was uploaded by registering a new user.)

sun’s picture

Assigned: sun » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
80.66 KB
140.91 KB

~6 hours of debugging and fixing later, I'm feeling pretty exhausted. Here's the slightly corrected and also cleaned up patch:

  1. Fixed interactive installer is broken due to bogus fixed module list in _drupal_maintenance_theme().
  2. Fixed module_enable() + module_set_weight() lose fixed module list. Added tests.
  3. Simplified code logic in ModuleHandler classes. Added documentation.
  4. Fixed $moduleHandler property names in request/routing classes.
  5. Reverted unnecessary test changes.
  6. Simplified schema.inc changes.
  7. Restored deprecated module_list() for DX and to not break dozens of current other patches.
  8. Moved ModuleHandler into Extension\ModuleHandler.

The most noteworthy changes happened to module_enable(), module_disable(), and module_set_weight(). Those were previously operating on the current/active module list of ModuleHandler, but that wasn't exactly right. A module is only enabled when it is in the system.module:enabled configuration. A module is not inherently enabled, just because it is loaded and in the active module list. Due to various reasons (install.php, update.php, tests, etc) we need to be able to operate with a fixed module list, but still be able to enable/install a module, without a hard binding between these two lists.

This is also what previously "broke" the DrupalUnitTests. Especially for those, we actually want to operate with merely loaded (but not installed) modules most of the time. Installing modules is actually discouraged for DUTB tests - by the time of the D8 release, I hope we got rid of most of the install operations in DUTB tests. Furthermore, DUTB::enableModules() actually contained a @todo that referenced exactly this issue, since module_enable() is expected to be able to install a module with the module_handler service. And it is. :) Therefore, all of the test changes have been reverted. And instead, new test coverage for loading/enabling/disabling/weight-changing/etc of modules in DUTB has been added.

What's slightly concerning with this overall patch and change proposal is the double-housekeeping of the active module list, once in DrupalKernel (%container.modules%) and once more in ModuleHandler. While it is true that both should be the same, because ModuleHandler gets the list from DrupalKernel injected, experience tell us that a "should" in combination with "duplicated data" is guaranteed to not be true, and to break badly. As long as everything runs through module_enable(), and every other code does the same as module_enable(), that's not an issue, because it ensures to rebuild/reboot the kernel appropriately. However, that's a very bold assumption, which is invalidated in this very patch already; drupal_install_system() does NOT call into module_enable() and does NOT perform the required kernel rebuild. I did not change or correct that, since #1798732: Convert install_task, install_time and install_current_batch to use the state system will change that function to call module_enable() instead. Anyhow, long story short, we should revisit whether we can eliminate the duplicate tracking and management of the module list in a follow-up issue.

The previous two test failures in #197 were caused by new calls to module_list() in core. I must say that I consider this to be a serious DX regression:

- module_list();
+ array_keys(drupal_container()->get('module_handler')->getModuleList());

For now, I restored it as @deprecated in order to decrease the chance for conflicting commits introducing new instances only. But personally, I don't think it makes sense to remove module_list() (just yet). I also considered to rename getModuleList() into getModuleFilenames(), and introduce a new getModuleList() that would return what module_list() returned before (i.e., module names both as keys and values). I think we should consider to do that in a follow-up issue, since the module list is needed very often (especially in contrib), and the filenames are needless, unexpected, and getting in everyone's way.

Anyway, all in all, I'm happy with this patch now. Let's see what the testbot has to say.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Avoid commit conflicts

The last submitted patch, module.handler.200.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

#207: module.handler.200.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Avoid commit conflicts

The last submitted patch, module.handler.200.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
141.12 KB
  1. Merged HEAD (conflicts).
  2. Fixed testing themes and bootstrap hooks are not refreshed in module_enable(). Added docs.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API clean-up, -Avoid commit conflicts

The last submitted patch, module.handler.211.patch, failed testing.

katbailey’s picture

Status: Needs work » Needs review

#211: module.handler.211.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Avoid commit conflicts

The last submitted patch, module.handler.211.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
1.01 KB
142.13 KB

Fixed LocaleUpdateTest.

The test failure in Drupal\rest\Tests\Views\StyleSerializerTest seems to be a random test failure — potentially caused by not accounting for sanitized strings.

sun’s picture

podarok’s picture

#215 looks good

katbailey’s picture

Status: Needs review » Reviewed & tested by the community

It was with great trepidation that I opened that 80K interdiff last night... but what I found were a lot of sensible changes that amount to a great refinement of the patch. So yay! back to RTBC :-D

Let's put this beast to bed already!

Dries’s picture

Issue tags: +needs profiling

While I like this in principle, this requires performance profiling and benchmarking before it can be committed. This could add quite a bit of overhead to Drupal.

Not sure this is truly 'critical' but I'll leave it as 'critical' for now.

Dries’s picture

Status: Reviewed & tested by the community » Needs work
Dries’s picture

It sounds like we also want to discuss the proper way to name services. It seems like '_handler' is inconsistent with how we name other services. Sometimes we use nothing (e.g. 'request' for the request handler), something we use '*_manager' (e.g. paths). Sometimes we use dots, sometimes we use underscores.

drupal_container()->get('request');
drupal_container()->get('path.alias_manager');
drupal_container()->get('plugin.manager.editor');

Maybe for now just drop the '_handler' part?

Lars Toomre’s picture

Not sure where in the current core process the 'documentation gate' should be addressed. There are missing @param, @return directives and one line descriptions for class properties missing from the last patch. Should that be addressed now or be put off to some as yet follow-up issue?

katbailey’s picture

In general, we've been using underscores when it takes multiple words to describe what the thing is - e.g. 'alias manager': it's not an alias, it's not just a manager, it's an alias manager, hence alias_manager (mirroring the class name AliasManager). The 'path.' prefix indicates it's a service provided by the path subsystem. So in the case of the ModuleHandler, calling the service simply 'module' would seem to fall short of describing what it actually is.

msonnabaum’s picture

I agree with Dries, we should pick a separator and use it consistently. However, that would require changing all the previous ones, so I think it's best handled in a follow up.

Crell’s picture

Per #223, this is named consistently with what we've been doing to date, which is inspired by Symfony's de facto conventions, and it's a discussion we already had a few months ago. If we want to rethink our overall approach to service naming, this is not the place to do it.

catch’s picture

Yes I agree that's a follow-up. Moving back to needs review for profiling. This /ought/ to be low impact (i.e. if there's a big performance hit it'll hopefully be because of something silly).

One thing we can potentially get out of it though is lazy-loaded modules which would be really nice for lightweight router items etc.

catch’s picture

Status: Needs work » Needs review
msonnabaum’s picture

Issue tags: -needs profiling

Tested with the apc loader, showing a slight performance improvement. Here's the summary for 50 samples both before an after the patch:

Before:

ct:
  min: '17,133'
  max: '20,240'
  mean: '17,195'
  median: '17,133'
  95th: '17,133'
wt:
  min: '82,820'
  max: '120,825'
  mean: '87,624'
  median: '86,219'
  95th: '95,422'
cpu:
  min: '77,982'
  max: '117,383'
  mean: '82,724'
  median: '81,160'
  95th: '91,994'
mu:
  min: '7,655,608'
  max: '8,239,176'
  mean: '7,667,279'
  median: '7,655,608'
  95th: '7,655,608'
pmu:
  min: '7,750,184'
  max: '8,336,928'
  mean: '7,761,919'
  median: '7,750,184'
  95th: '7,750,184'

After:

ct:
  min: '16,005'
  max: '16,005'
  mean: '16,005'
  median: '16,005'
  95th: '16,005'
wt:
  min: '78,102'
  max: '89,096'
  mean: '81,443'
  median: '80,765'
  95th: '86,518'
cpu:
  min: '74,161'
  max: '84,601'
  mean: '77,360'
  median: '77,118'
  95th: '82,436'
mu:
  min: '7,499,456'
  max: '7,499,544'
  mean: '7,499,542'
  median: '7,499,544'
  95th: '7,499,544'
pmu:
  min: '7,588,336'
  max: '7,595,528'
  mean: '7,595,384'
  median: '7,595,528'
  95th: '7,595,528'
effulgentsia’s picture

Issue tags: +needs profiling

In terms of profiling, one of my concerns is:

+++ b/core/includes/bootstrap.inc
@@ -2476,6 +2489,82 @@ function drupal_container(Container $new_container = NULL) {
+function drupal_alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
+  return drupal_container()->get('module_handler')->alter($type, $data, $context1, $context2);
+}

Currently, calling drupal_alter() when no modules implement the alter hook is nearly free, which is good, because it means modules can add alter steps liberally without worrying about performance (modules choosing to *implement* rather than *add* an alter hook need to evaluate the performance of doing so, but that places the responsibility where it belongs). Here, we're adding quite a few stack calls, which I fear will result in too much pressure to remove alter steps entirely in some places. Maybe this will get mitigated if all frequently called code is in objects with an injected module handler, though I have my doubts about that being a reality in D8.

The same concern may also apply to module_invoke_all(), module_list(), module_implements(), etc. Even without the procedural wrapper, there's still a lot of extra stack calls in doing drupal_container()->get('module_handler')->method() rather than global_function().

This could possibly be partially mitigated with a module_handler() function that statically caches the result of drupal_container()->get('module_handler')?

Anyway, getting data first before micro-optimizing makes sense, but hopefully this helps identify what scenarios deserve profiling.

effulgentsia’s picture

x-post. If overall performance is looking like an improvement, then I'm ok with #229 being a follow up.

chx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -needs profiling

Profiling showed an improvement there's agreement that service name bikeshedding is a followup.

Re. #229 there always was a little cost to drupal_alter, it's still there, a little more perhaps, but then again, it's just a few function calls that are added (not many!). But we can, again look at this in a followup.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review

It seems like '_handler' is inconsistent with how we name other services.

The class is currently named Drupal\Core\Extension\ModuleHandler. If we want to be consistent with Drupal\Core\Path\AliasManager, then we can rename it to Drupal\Core\Extension\ModuleManager, and name the service 'extension.module_manager'. However, we don't currently have an "extension system" in the Component drop-down of the d.o. core issue queue: we can either add it, or decide it's not really its own system and therefore name the service simply 'module_manager'.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

x-post. #232 can also be followup material.

effulgentsia’s picture

Re. #229 there always was a little cost to drupal_alter, it's still there, a little more perhaps, but then again, it's just a few function calls that are added (not many!).

Yeah, I just grepped core for where drupal_alter() is called, and it looks like the only ones where a couple extra stack calls might matter is:
- drupal_alter('url_outbound') which will likely go away with #1888424: Make Drupal's URL generation logic available to HttpKernel, and minimize code repetition/divergence, or can get $moduleHandler/$moduleManager injected into it.
- drupal_alter('query') which if it remains, I'm assuming Drupal\Core\Database\Query\Select can get $moduleHandler/$moduleManager injected into it.
- drupal_alter('translated_menu_link'), which already has an optimization to only be called conditionally, and _menu_link_translate() will need to move into an OOP system anyway, so can get $moduleHandler/$moduleManager injected into it.
- drupal_alter('user_format_name'): eh, we might just need to bite the bullet on that one

There are probably some contrib cases of drupal_alter() happening on something that gets called very often at runtime, but saying that those cases can be optimized on a case by case basis by moving that code into a proper service is probably not so bad.

webchick’s picture

Assigned: Unassigned » catch

This terrifies me, so assigning to catch. :D

katbailey’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
751 bytes
142.87 KB

Needed a reroll after #1848998: Problems with update_module_enable() landed as that patch introduced a new call to module_list_reset().
The update function introduced by that patch worries me slightly too (I was not aware of the update_module_enable() function's existence before) but SystemUpgradePathTest is passing for me locally so I guess that means everything's hunky-dory..?

katbailey’s picture

Status: Needs review » Reviewed & tested by the community
sun’s picture

Created two follow-up issues:
#1893680: Rename 'module_handler' service and ModuleHandler classes
#1893690: Find a new home and service for module_enable(), module_disable(), module_set_weight(), etc

Would be great to see this in, so we can unblock dependent issues and move on with follow-up issues to clean up the remaining parts.

catch’s picture

Title: Move module_invoke_*() and friends to an Extensions class » Change notice: Move module_invoke_*() and friends to an Extensions class

Thanks for the follow-ups.

I've gone ahead and committed/pushed this to 8.x. This will need a change notice.

jibran’s picture

Issue tags: +Needs change record

tagging

jibran’s picture

Status: Reviewed & tested by the community » Needs work

Status for change notice.

sun’s picture

Btw, work and IRC discussions on this also triggered: #1892574: Remove hook_hook_info_alter() ;)

catch’s picture

Status: Needs work » Needs review

This broke HEAD. Reverted for now.

xjm’s picture

Title: Change notice: Move module_invoke_*() and friends to an Extensions class » Move module_invoke_*() and friends to an Extensions class
xjm’s picture

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API clean-up, +Needs change record, +Avoid commit conflicts

The last submitted patch, 1331486.module_handler.236.patch, failed testing.

sun’s picture

Assigned: catch » sun
sun’s picture

update.php fails with:

Failed: InvalidArgumentException: The entity (user_role) did not specify a controller_class. in Drupal\Core\Entity\EntityManager->getControllerClass() (line 330 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Entity/EntityManager.php).

due to #1479454: Convert user roles to configurables

Furthermore, update_access_allowed() manually loads user.module to call user_access(), which in turn circles back into user roles being converted into config entities, but not being migrated into config yet.

Furthermore, the entire error stack triggers a fatal error in itself:

Fatal error: PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'test_drupal8.simpletest944757cache_config' doesn't exist
Call Stack:
    0.1798    2019408   1. _drupal_exception_handler() core\includes\bootstrap.inc:0
    0.1950    2250480   2. error_displayable() core\includes\bootstrap.inc:2280
    0.1957    2253536   3. Drupal\Core\Config\Config->get() core\includes\errors.inc:156
    0.1957    2253536   4. Drupal\Core\Config\Config->load() core\lib\Drupal\Core\Config\Config.php:211
    0.1957    2253536   5. Drupal\Core\Config\CachedStorage->read() core\lib\Drupal\Core\Config\Config.php:416
    0.1957    2253536   6. Drupal\Core\Cache\DatabaseBackend->get() core\lib\Drupal\Core\Config\CachedStorage.php:62
    0.1957    2253736   7. Drupal\Core\Cache\DatabaseBackend->getMultiple() core\lib\Drupal\Core\Cache\DatabaseBackend.php:46
    0.1957    2254264   8. Drupal\Core\Database\Connection->query() core\lib\Drupal\Core\Cache\DatabaseBackend.php:61
    0.1958    2256000   9. Drupal\Core\Database\Statement->execute() core\lib\Drupal\Core\Database\Connection.php:523
    0.1958    2256032  10. PDOStatement->execute() core\lib\Drupal\Core\Database\Statement.php:58

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'test_drupal8.simpletest944757cache_config' doesn't exist: SELECT cid, data, created, expire, serialized, tags, checksum_invalidations, checksum_deletions FROM {cache_config} WHERE cid IN (:cids_0); Array
(
    [:cids_0] => system.logging
)
 in core\lib\Drupal\Core\Database\Connection.php on line 554

Call Stack:
    0.1798    2019408   1. _drupal_exception_handler() core\includes\bootstrap.inc:0
    0.1950    2250480   2. error_displayable() core\includes\bootstrap.inc:2280
    0.1957    2253536   3. Drupal\Core\Config\Config->get() core\includes\errors.inc:156
    0.1957    2253536   4. Drupal\Core\Config\Config->load() core\lib\Drupal\Core\Config\Config.php:211
    0.1957    2253536   5. Drupal\Core\Config\CachedStorage->read() core\lib\Drupal\Core\Config\Config.php:416
    0.1957    2253536   6. Drupal\Core\Cache\DatabaseBackend->get() core\lib\Drupal\Core\Config\CachedStorage.php:62
    0.1957    2253736   7. Drupal\Core\Cache\DatabaseBackend->getMultiple() core\lib\Drupal\Core\Cache\DatabaseBackend.php:46
    0.1957    2254264   8. Drupal\Core\Database\Connection->query() core\lib\Drupal\Core\Cache\DatabaseBackend.php:61

Conclusion:

1) update.php is completely hosed currently, regardless of how we put it, and regardless of which patches we roll back or forth.

2) We need a custom kernel/bundle for update.php, that overrides a range of services.

Working on this.

sun’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
147.87 KB
  1. Fixed error handler requires config() to exist and class loader to be active.
  2. Fixed update.php requires a UpdateBundle to prevent services from accessing non-existing storages.
  3. Fixed obsolete addition of user roles to filter format configuration calls into module APIs.

The addition of UpdateBundle might be debatable, but it's a fully working solution. I'd suggest to do this for now, and if needed, re-evaluate a better solution in a follow-up issue.

xjm’s picture

Status: Needs review » Needs work
xjm’s picture

Status: Needs work » Needs review

Sorry, xpost.

xjm’s picture

Maybe #249 should be in its own issue, and this one postponed on it? It might help solve a lot of problems.

moshe weitzman’s picture

UpdateBundle looks small and clean to me. +1. Thanks also for fixing that error handler config dependency.

sun’s picture

And we're green again. Hope we can move forward with this.

Ideally, before any other commits — this patch has an exceptionally high hit rate for commit conflicts.

katbailey’s picture

Status: Needs review » Reviewed & tested by the community

sun++
The changes in #249 look entirely reasonable to me - back to RTBC.

sun’s picture

To clarify upfront, because it's probably not obvious:

  1. #1779026: Convert Text Formats to Configuration System initially introduced a $roles property for text $format entities, to control the user role access permissions.
  2. That idea, however, turned out to be a too tight entity reference between a text format object and user role IDs (which are equally entities now) — the user role permissions can be changed without touching the text format configuration at all, so the text format configurations contained stale information about which roles have access to which format.
  3. Therefore, we retained, but scaled down the FilterFormat::$roles property to only have an effect upon initial creation of a new text format. This allows default configuration in installation profiles (and also tests) to declare role access within the configuration, which vastly simplifies the setup of text formats.
  4. However, the $roles are not exported into config for existing formats. The value of $roles is completely ignored for existing formats. Therefore, there's also no point in populating the 'roles' key of text format configuration objects in the upgrade path.

Thanks for listening. :) Decide for yourself whether to put this into the Things I Didn't Want To Know™ or the Good To Know™ drawer ;)

catch’s picture

Title: Move module_invoke_*() and friends to an Extensions class » Change notice: Move module_invoke_*() and friends to an Extensions class

The error_handler changes are duplicate with #1845646: error_displayable() cannot be converted from variable system safely. That's already a major task blocking another critical task, so let's just go with sun's change in this patch for now, and we can continue looking at it in another issue. The problem with setting the error handler later is we'll just be using whatever native PHP error handling until that point.

Committed/pushed to 8.x again.

sun’s picture

Title: Change notice: Move module_invoke_*() and friends to an Extensions class » Move module_invoke_*() and friends to an Extensions class
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record
sun’s picture

jhodgdon’s picture

Sigh. This has probably killed everyone's favorite feature on api.drupal.org -- the links to where the hooks are invoked. I've filed
#1895024: Support Core's 8.x module invoke system
in hopes that maybe we can do something else to continue to have this feature for 8.x code. If you have any implementation suggestions, please put them there.

TwoD’s picture

This patch broke installations not using English. Please see #1898236: Installing without English language leads to Fatal Error.

Status: Fixed » Closed (fixed)

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

alexpott’s picture

Issue tags: -Avoid commit conflicts

Cleaning up tags...

YesCT’s picture

joachim’s picture

The params for invokeAll() are not the same as for module_invoke_all(), but the docs for the new invokeAll() have the old parameters. The change record doesnt state that the parameters have changed for invoking a hook. See #2095219: incorrect params in ModuleHandlerInterface::invokeAll().

joachim’s picture

Issue summary: View changes

Adding pastie draft