System updates are executed without priority

We have DependencyHookInvocationTest.php to test the order is correct, is it possible that we have a circular dependency?

Modules implementing hook_update_dependencies().

core/modules/block/block.install
core/modules/contact/contact.install
core/modules/overlay/overlay.install

All of these are concerned with getting the users_data sorted so there is no reason why user.module updates should come before the system.module ones

As discussed with @chx, changing the priority of this to normal.

I haven't seen random upgrade path test failures like we used to have frequently in quite a while and while this does look strange, it is IMHO neither critical nor major until we have proof that this actually causes problems with the upgrade path.

Closing this in favor of #211182: Updates run in unpredictable order which has several patches and over 200 comments.

Sorry, I hadn't realized the other issue was committed and that this is a symptom of the problem still existing.

I was experiencing this yesterday and today and suddenly it disappeared. While debugging for #2951279: After 8.5.0 upgrade: Unknown column revision.revision_default it happened multiple times that the system_update_8501 was executed as one of the last instead as one of the first updates. This cased my Drupal update to fail, because it is necessary that this particular system update is executed before performing entity CRUD operations in other updates.

I've cooked something which should further ensure that system updates are executed as soon as possible.

Having a bunch of contrib updates and updating Drupal from one minor to another minor version could be classified as dangerous if we cannot ensure correct update order.

Therefore I think that this issue is at least major even if the failures are random, as we now have a lot of complains in #2951279: After 8.5.0 upgrade: Unknown column revision.revision_default , which for me is enough proof that there is something wrong.

I've finally found something which is reproducible - update of a "standard" profile installation from 8.4.8 to 8.6.x revision "e9dd745e03168b4759f8d3261b3ebd93246cb168".

Case 1:
There are no update dependencies. The updates order as returned by update_resolve_dependencies() looks like this:

1. taxonomy_update_8501
2. taxonomy_update_8502
3. system_update_8501
4. block_content_update_8400
5. comment_update_8600
6. dblog_update_8600
7. field_update_8500
8. menu_link_content_update_8601
9. taxonomy_update_8503
10. views_update_8500

There is absolutely no reason for the taxonomy updates to be executed before the system update, but this is what \Drupal\Component\Graph\Graph::searchAndSort() generates.

Case 2:
If we define through hook_update_dependencies that taxonomy_update_8502 and block_content_update_8400 have to be executed before system_update_8501 then the updates order looks like this:

1. taxonomy_update_8501
2. block_content_update_8400
3. taxonomy_update_8502
4. system_update_8501
5. comment_update_8600
6. dblog_update_8600
7. field_update_8500
8. menu_link_content_update_8601
9. taxonomy_update_8503
10. views_update_8500

With the attached patch to this comment the cases look like this:

Case 1:

1. system_update_8501
2. views_update_8500
3. taxonomy_update_8501
4. taxonomy_update_8502
5. taxonomy_update_8503
6. menu_link_content_update_8601
7. field_update_8500
8. dblog_update_8600
9. comment_update_8600
10. block_content_update_8400

Case 2:

1. taxonomy_update_8501
2. taxonomy_update_8502
3. block_content_update_8400
4. system_update_8501
5. views_update_8500
6. taxonomy_update_8503
7. menu_link_content_update_8601
8. field_update_8500
9. dblog_update_8600
10. comment_update_8600

The new patch ensures that system updates will be executed as soon as possible.

If there are multiple system updates and there is an update defined to run before the first system update, then we should not add regular updates as edges to later system updates otherwise the dependencies will not be executed before the system updates.

What went wrong with the last patch is this case:
Define that taxonomy_update_8502 should run before system_update_8501 and add new update system_update_8502.

With the previous patch the system updates will be executed before the taxonomy updates taxonomy_update_8501 and taxonomy_update_8502, but it should be the other way around and the new patch is making this right.

I've implemented a general test asserting that system updates will be executed first.

Had a look at the patch and it seems to be doing three things. None of which seem inherently problematic, but I'd like to get a better understanding why they are needed and also why all three things are needed. The three things are:

1. Add +1 to all weights of non-system functions
2. Add edges to "grandparents" of system updates
3. Add edges to system updates to updates without any system update edges

Notes on the actual patch:

1. +++ b/core/includes/update.inc
   @@ -428,6 +431,45 @@ function update_resolve_dependencies($starting_updates) {
   + * The weights of updates in a group of the same weight containing system
   + * updates will be adjusted, so that we ensure that system updates are executed
   + * as soon as possible.
   

   As far as I can tell the code here is correct, but I think this could use an example with a bunch of update functions and their weight before and after calling this function.

2. +++ b/core/includes/update.inc
   @@ -544,6 +586,60 @@ function update_build_dependency_graph($update_functions) {
   +    return $parents;
   

   Should we not do a array_unique() on this? I think there could be multiple paths from one update function to another.

3. +++ b/core/includes/update.inc
   @@ -544,6 +586,60 @@ function update_build_dependency_graph($update_functions) {
   +  // If an update U2 has an edge to a system update S then add S as an edge to
   +  // all updates having U2 as an edge. This ensures that the chain U1->U2->S
   +  // will be executed in that particular order.
   

   This is a mix between describing the behavior generally and providing an example. I think that makes it less understandable than just giving the example. Also I think the fact the U1 has an edge to U2 is kind of missing in the explanation. I suggest:
   

   If a given non-system update U1 has an edge pointing to another non-system update update U2 and U2 has an edge pointing to the a system update S, add an edge to U1 pointing to S. This ensures that the chain U1->U2->S will be preserved in that order despite the system updates being moved before any other updates below.

4. +++ b/core/includes/update.inc
   @@ -544,6 +586,60 @@ function update_build_dependency_graph($update_functions) {
   +  foreach ($graph as $non_system_func => $data) {
   +    if ($data['module'] !== 'system') {
   

   The code here is correct, but it is strange to read foreach ($graph as $non_system_func) since the graph contains system functions as well. What do you think about doing a $non_system_graph = array_filter($graph, ...); before this?

5. +++ b/core/includes/update.inc
   @@ -544,6 +586,60 @@ function update_build_dependency_graph($update_functions) {
   +            $graph[$parent_edge]['edges'] = empty($graph[$parent_edge]['edges']) ? $sys_updates : array_merge($graph[$parent_edge]['edges'], $sys_updates);
   

   I think we should do an array_unique() on the result of the array_merge()

6. +++ b/core/includes/update.inc
   @@ -544,6 +586,60 @@ function update_build_dependency_graph($update_functions) {
   +  foreach ($graph as $system_function => &$system_data) {
   +    if ($system_data['module'] === 'system') {
   

   Same question here: Could we use a array_filter() here?

7. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
   @@ -435,4 +443,46 @@ protected function doSelectionTest() {
   +        else {
   +          $system_updates_finished = TRUE;
   +        }
   

   I don't think this is being used

Had a look at the patch and it seems to be doing three things. None of which seem inherently problematic, but I'd like to get a better understanding why they are needed and also why all three things are needed. The three things are:

1. Add +1 to all weights of non-system functions
2. Add edges to "grandparents" of system updates
3. Add edges to system updates to updates without any system update edges

1. Add +1 to all weights of non-system functions is needed to ensure that system updates will run before the other updates when in the same group of weights. So if the graph outputs something like -

weight 0 - system_update_1, example_update_1
weight 1 - example_update_2, system_update_2

then this weight reordering would ensure that system updates will be the first to run in a group, because we put them in a group before the group they initially belonged to.

2. Add edges to "grandparents" of system updates
During my manual tests I've noticed that somehow the order U1=>U2=>S1 might break if not both U1 and U2 get an edge to S1. I think that it is not wrong at doing so when it increases the stability.

3. Add edges to system updates to updates without any system update edges
This is one more step into increasing the stability and is similar to 1. by ensuring that everything depends on the system updates.

One might argue that 1. and 3. are doing the same thing, but I thought that the more safety features we have the better.

About the patch:

1. I've added a simple example. Or do we need a more complex one?

2. Should we not do a array_unique() on this? I think there could be multiple paths from one update function to another.

   I've thought that it is not a problem to set the same update multiple times as an edge :). But I guess you are right, that it is better to return only the unique entries. Done.

3. Thanks! Done.
4. I am not sure that this really makes the code more easy to parse. And we'll be creating one more array. We are only iterating over the array currently and do not need a copy of it. If you insist on this, then I'll do it :).
5. Right. This is similar to 2. Done.
6. See answer for 4. Only if you insist on it :).

7. I don't think this is being used

   This is used when we come to a function, which is not a system update. Consider the forbidden case S1->U1->S2 (given they don't have dependencies between each other), now when we arrive at U1, we'll set $system_updates_finished to TRUE and when we reach S2, we'll assert that this variable is still FALSE, which will then fail as expected.

Re
1. ...
2. ...
3. ...

So I think if 2. is in some way required that seems like a separate bug. And while you're right that doing both 1. and 3. isn't a problem in and of itself, but I do think adding code to an already complex subsystem comes at a high maintenance cost. So I would prefer we only do either 1. or 3. I personally think 3. is a bit more elegant than 1. but I'll leave that up to you. If I understood correctly the test wouldn't need any changes.

Re patch fixes:
1. I think that's already helpful, thanks! (Although we might have to update/remove it per the above.)
2. ✔️
3. ✔️
4. No, that's fine.
5. ✔️
6. See 4 😉
7. Yup, you're right. I had the logic messed up in my head.

Leaving at needs review because my only criticism is a conceptual issue and it wouldn't hurt to get more eyes/opinions on this.

This needed a re-roll, but Git was able to handle it.

Plain re-roll.

Simple reroll.

This should fix the failing tests complaining about vfs. The other one does not fail locally. Let's give it another try.

-----

I must've got confused - the test still fails locally and therefore I've re-opened the issue where it was introduced and posted an additional test for demonstrating the problem - https://www.drupal.org/project/drupal/issues/3031128#comment-13349092

According to the referenced issue we are not allowed do this check in this particular test and therefore I am simply overriding the method with an empty body.

Patch doesn't apply anymore on HEAD of 8.9.x and 8.8.x, needs a re-roll.

I've rerolled this patch for 8.8.x, 8.8.5, 8.9.x, 9.0.x and 9.1.x.
It needs review, please.

@gnunes if a patch applies to more that one branch then there is no need for multiple patches. So we only need on 8.x patch and one 9.x patch.

The 8.8 patches are failing because of the committed #3063912: Move UpdatePathTestBase::runUpdates() to a trait.
The 8.9.x branch passes because of the committed #3098475: Add more strict checking of hook_update_last_removed() and better explanation, which has not yet been committed to 8.8.x.

Just to ensure that #3098475: Add more strict checking of hook_update_last_removed() and better explanation fixes the 8.8.x fails.

1. +++ b/core/includes/update.inc
   @@ -327,14 +327,6 @@ function update_get_update_list() {
   -        $ret[$module]['warning'] = '<em>' . $module . '</em> module cannot be updated. Its schema version is ' . $schema_version . '. Updates up to and including ' . $last_removed . ' have been removed in this release. In order to update <em>' . $module . '</em> module, you will first <a href="https://www.drupal.org/upgrade">need to upgrade</a> to the last version in which these updates were available.';
   
   +++ b/core/modules/system/system.install
   @@ -1295,6 +1295,40 @@ function system_requirements($phase) {
   +          'description' => t('The installed version of the %module module is too old to update. Update to an intermediate version first (last removed version: @last_removed_version, installed version: @installed_version).', [
   

   This bugfix shouldn't be changing the text.

2. +++ b/core/modules/system/tests/src/Functional/Update/WarmCacheUpdateFrom8dot6Test.php
   @@ -96,4 +96,12 @@ protected function initConfig(ContainerInterface $container) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function assertSystemUpdatesPriorityExecution() {
   +    // Don't check the system updates priority before running the updates as
   +    // this invokes listing modules.
   +  }
   

   This is a worry. I think the code in assertSystemUpdatesPriorityExecution should be able to run with broken stuff. Because otherwise contrib tests could hit this too. Or future core tests.

Re #44.1 hmmm actually I don't get the changes to system_requirements... ah I see it included #3098475: Add more strict checking of hook_update_last_removed() and better explanation. Let's get a new set of patches on this issue.

Here's the same patches as #37

I just wanted to note that this patch also fixes #3131523: Running update path with core 8.8 and pathauto breaks

Given the importance of running system updates first and in-wild issues that would benefit from this I'm promoting this to critical as I think it will help the overall stability of our update eco system.

Maybe new function should start with underscore or it is supposed to be new API?

+++ b/core/includes/update.inc
@@ -432,6 +435,74 @@ function update_resolve_dependencies($starting_updates) {
+function update_prepare_graph_for_sort(array &$graph) {

Good idea @andypost start with an underscore and have @internal. This is definitely not API.

Here's a fix for #49 and CS

Valid patch for 8.9

Knowing this is a critical bug, trying to give this a thorough review. I haven't (yet) read any of the comments in the thread. Just trying to make sense of the patch on its own. Having some trouble with that. ;) Hopefully this is useful feedback. I know the db update system is confusing and hard. But I'd love to try to help keep it accessible for a wider swath of the core contributor community to make sense of.

1. +++ b/core/includes/update.inc
   @@ -432,6 +435,76 @@ function update_resolve_dependencies($starting_updates) {
   + * The weights of updates in a group of the same weight containing system
   + * updates will be adjusted, so that we ensure that system updates are executed
   + * as soon as possible.
   

   Reading this whole method a few times, I'm not totally clear what "group" means here. It's new terminology to me for the DB update system. Does a group just mean "all updates with the same weight value"? Do we need to introduce and use "group" here for this?

2. +++ b/core/includes/update.inc
   @@ -432,6 +435,76 @@ function update_resolve_dependencies($starting_updates) {
   + * $graph = [
   + *   'node_update_8xxx' => [
   + *     'weight' => 0,
   + *     'module' => 'node',
   + *   ],
   + *   'system_update_8xxx' => [
   + *     'weight' => 0,
   + *     'module' => 'system',
   + *   ].
   + * ];
   

   Given the complications below, can we get a slightly more complex/realistic example to help make sense of the algorithm this function is implementing? This example is so trivial, it doesn't add much to understanding the function.

3. +++ b/core/includes/update.inc
   @@ -432,6 +435,76 @@ function update_resolve_dependencies($starting_updates) {
   +  $new_weights = array_combine($weights, $weights);
   

   The example from above says we're going to start with 2 updates with weight 0. So we're going to have:

   $new_weights = array_combine([0,0], [0,0]);

   Due to the collision on 0, isn't that just going to give us:

   $new_weights = [0 => 0]? Is that intentional? Seems a bit wonky and magical. If that's intentional, can we document it? If it's a bug, I'm shocked, but then we should fix it. ;)

4. +++ b/core/includes/update.inc
   @@ -432,6 +435,76 @@ function update_resolve_dependencies($starting_updates) {
   +  foreach ($system_updates as $system_update_weight) {
   +    foreach ($new_weights as $old_weight => &$new_weight) {
   +      if ($old_weight >= $system_update_weight) {
   +        $new_weight++;
   +      }
   +    }
   +  }
   

   This nested foreach could use some comments. I still don't fully understand why it's doing this in this way. The $old_weight might be much higher than the $system_update_weight, but we only increment $new_weight by 1.

   If the goal is to ensure that everything *not* a system update has a weight higher than all the system updates, I'd expect we'd keep track of the max() of $system_updates and then in the final loop, we just check each update, and if it's not a system update, we simply add $max_system_update_weight to the defined weight. That seems more efficient, more obvious, and more clear. But it's probably the wrong behavior. ;) I don't know why. Please comment this to help me/everyone understand what's going on. ;)

5. +++ b/core/includes/update.inc
   @@ -432,6 +435,76 @@ function update_resolve_dependencies($starting_updates) {
   +  // Increment the weight of all updates in a group with a system update but
   

   And here... don't know what "in a group with a system update" actually means.

6. +++ b/core/includes/update.inc
   @@ -548,6 +621,62 @@ function update_build_dependency_graph($update_functions) {
   +    foreach ($graph as $func => $data) {
   ...
   +        $parents[] = $func;
   +        $parents = array_merge($parents, $get_all_parents($func));
   

   instead of $func, can we call this $update_name for consistency with the above code, to avoid confusion that it's some magic PHP syntax inside a lambda (anonymous) function? ;)

7. +++ b/core/includes/update.inc
   @@ -548,6 +621,62 @@ function update_build_dependency_graph($update_functions) {
   +  // If a given non-system update U1 has an edge pointing to another non-system
   +  // update update U2 and U2 has an edge pointing to the a system update S, add
   +  // an edge to U1 pointing to S. This ensures that the chain U1->U2->S will be
   

   A) Wants a comma after the first "U2".

   B) "pointing to the a system update S," doesn't parse. I think we can remove "the a" and have "pointing to system update S". Maybe "S1" is better for consistency with the U* examples?

8. +++ b/core/includes/update.inc
   @@ -548,6 +621,62 @@ function update_build_dependency_graph($update_functions) {
   +  foreach ($graph as $non_system_func => $data) {
   

   Why is everything in this graph $non_system_func? I don't get it just from reading the patch. Oh, because of the line right below it:

   +    if ($data['module'] !== 'system') {
   

   Hrm. I guess this works. Not sure there's a cleaner alternative.

9. +++ b/core/includes/update.inc
   @@ -548,6 +621,62 @@ function update_build_dependency_graph($update_functions) {
   +            $graph[$parent_edge]['edges'] = empty($graph[$parent_edge]['edges']) ? $sys_updates : array_unique(array_merge($graph[$parent_edge]['edges'], $sys_updates));
   

   Very efficient and clever. And also a lot to try to make sense of in a ~160 char line. ;) Probably I'll be outvoted and this should stay as-is. But for mere mortals trying to understand this code, maybe it's worth unwinding this a bit? /shrug

10. +++ b/core/includes/update.inc
    @@ -548,6 +621,62 @@ function update_build_dependency_graph($update_functions) {
    +  foreach ($graph as $system_function => &$system_data) {
    +    if ($system_data['module'] === 'system') {
    +      foreach ($graph as $non_system_function => $non_system_data) {
    +        if ($non_system_data['module'] !== 'system') {
    

    A) It's slightly confusing that these loops have the keys as system_function vs. non_system_function in advance, but then immediately test data['module']. More of the same from 2 points up. But I don't see a cleaner way to do this. /shrug

    B) Trying to make sense of this whole block of code, it seems like thanks to the outer loop, for a given non-system update U1 that has no edges pointing to a system update, we're going to add U1 as an edge to *every* system update. Right? If so, I think the comment above should say that's the intended behavior.

11. +++ b/core/includes/update.inc
    @@ -548,6 +621,62 @@ function update_build_dependency_graph($update_functions) {
    +          $contains_system_update_edge = FALSE;
    +          $edges = empty($non_system_data['edges']) ? [] : array_keys($non_system_data['edges']);
    +          foreach ($edges as $edge) {
    +            if ($graph[$edge]['module'] === 'system') {
    +              $contains_system_update_edge = TRUE;
    +              break;
    +            }
    +          }
    

    We re-compute all this many times. It's not clear why. Can't we do a single pass through the entire graph, figure out if each update has a system_update_edge once, and then re-use that information inside the nested loops? Might make the whole thing easier to understand, and perhaps more efficient overall?

12. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -293,6 +296,14 @@ protected function runUpdates() {
    +    // The update test might be retrieving the schema version of a module
    +    // before the updates are executed, which will now lead to
    +    // update_get_update_list() retrieving the schema version prior to the
    +    // update. To prevent this we reset the static cache of
    +    // drupal_get_installed_schema_version() so that more complex update path
    +    // testing works.
    

    A) The whole first sentence seems like "test might retrieve schema version before updates are executed, which will lead to (something else) will retrieve schema version before updates are executed". ;) So what? I don't know why we need to prevent that. Maybe we don't need to explain, but it seems a bit circular.

    B) "To prevent this, we ..." (wants a comma).

13. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -335,4 +346,46 @@ protected function replaceUser1() {
    +    // Ensure install files are loaded. We reset the static cache as if the
    +    // update functions for retrieving update information have been called they
    +    // might contain an old data.
    

    2nd sentence doesn't parse: "... update information have been called they might..." -- huh? I'm not going to pretend I understand this enough to suggest better wording. ;)

14. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -335,4 +346,46 @@ protected function replaceUser1() {
    +    // @see drupal_get_schema_versions().
    

    I thought we didn't do periods at the end of @see comments like this.

15. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -335,4 +346,46 @@ protected function replaceUser1() {
    +    $get_module_updates = function () {
    +      return $this->getModuleUpdates();
    +    };
    +    $get_module_updates = $get_module_updates->bindTo($db_update_controller, $db_update_controller);
    +    $starting_updates = $get_module_updates();
    

    I have very little idea what this code block is doing. Clearly my PHP lambda function fu is lacking. Why pass $db_update_controller twice to bindTo()? Reading https://www.php.net/manual/en/closure.bindto.php only partly explains. I guess it's not the job of inline comments to teach readers about such intricacies of PHP closures, but ugh.

    Naive Derek asks: "Why not just call $db_update_controller->getModuleUpdates() directly?" ;) WTF do we need the anonymous function and double bindTo() for? Help?

16. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -335,4 +346,46 @@ protected function replaceUser1() {
    +      $system_updates_finished = !$first_update['module'] === 'system';
    

    !$a === 'b' seems harder to mentally parse than $a !== 'b'. Maybe that's just me. Also seems to rely too much on operator precedence. We don't want to negate $a and then compare to $b. We want to negate the comparison of $a and $b. If I understand correctly, let's use !== for this.

17. +++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
    @@ -335,4 +346,46 @@ protected function replaceUser1() {
    +      foreach ($updates as $function => $update) {
    

    Elsewhere in this patch, we use:
    foreach ($updates as $update_function => $update_data) which seems a bit more self-documenting than this.

Thanks/sorry!
-Derek

p.s. Not changing the status, since I don't want to hold up a critical bug for mostly minor cosmetic commenting stuff. We could open a follow-up. But if anyone wanted to re-roll to address any/all of this, I'd feel a lot more comfortable RTBC'ing.

+++ b/core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php
@@ -335,4 +346,46 @@ protected function replaceUser1() {
+    $updates = update_resolve_dependencies($starting_updates);

The number of updates this function returns is only two (system_update_8901 and update_test_semver_update_n_update_8001). This is so, because the number of installed modules is very small (system and update_test_schema). We should add a new test with all or almost all core modules installed.

Gonna have to let this one go since, I really don't grok most of the logic in this patch.

Tagging this as a possible target for the #BugSmash initiative...

OK, the idea of the latest patch is to run updates with modules dependencies in mind. Naturally, it puts core (system) things first. Tested this approach with Open Social based project upgrade with 220 updates. It worked.

Let me know if we should move in this direction.

Possible pitfalls: this patch might create a circular dependency ...

@Taran2L: Your patch needs testing.

@daffie, yes definitely. But before investing time into it I would like to see some pre-approves of the taken direction.

Opened a thread on the MR.

@Taran2L when you provide a whole new branch to review on an issue it is on you to provide information to the previous branch author and other following the issue as to how your new work differs from past efforts. The comment #68 needs to explain way way more - like why it might create circular dependencies. And it needs to add the test coverage from the previous work on this issue because just saying this fixes things is not enough in this area. FWIW when I've looked that this issue before #53 was a good solution and all we needed to do was to address #55 much of which was cosmetic. Can't we go back to trying to do that.

@alexpott, OK, fair points, but I'm not sure the language is. Isn't one of the crucial feature of the forks is to allow of different opinions ?

Anyway, the main difference in #68 from previous work in #53 is that it changes order for every pending update (not only core ones), forcing an update subsystem to take into the account dependencies of the modules. This actually broads the scope of the issue, but also (naturally) makes system updates to run as soon as possible (just because most of the code in the end depends on system module)

Technically it makes each first pending update of each module to depend on the last updates of all the modules that this particular one depends on

This approach might create a circular dependency in conjunction with hook_update_dependencies() when calling code is setting expectation on the order. Personally to me, this should not be an issue in the real world, because it's just wrong to expect updates from the things that a module does not explicitly depends on.

Also, as I asked in #68

Let me know if we should move in this direction.

If this is a direction that is interesting, I can provide more details/description/create a followup etc, but I don't think investing my time into a direction that won't be considered/committed at the first place worth it.

@Taran2L thank you for #74 - I was not saying that alternate approaches are not welcome I was saying that when an alternate approach is offered it needs take into account the previous state of the issue. Given the prior approach was close to rtbc given @dww's review that means completely new need come a bit more fully formed than #68. I'm sorry that I did not thank you for working on this complex issue. It will be great to have this solved.

I'm not sure that the logic as explained in #74 is correct. It's perfectly possible for a module to needs it's update to come in-between another module's (that it is dependent on) update. This is why hook_update_dependencies() is bi-directional - i.e. you can set the dependencies of your own updates but you can also set dependencies of other updates.

@alexpott, no worries ..

So, tests from previous version do pass with the new approach without any adjustments. Is it worth to explore the thing more ?

@Taran2L as far as I understand from your description one of the documented purposes of hook_update_dependencies() will be broken by the patch. Ie...

I'm not sure that the logic as explained in #74 is correct. It's perfectly possible for a module to needs it's update to come in-between another module's (that it is dependent on) update. This is why hook_update_dependencies() is bi-directional - i.e. you can set the dependencies of your own updates but you can also set dependencies of other updates.

So I'm not sure that it is worth exploring - unless I'm wrong it does not break that.

@alexpott, the proposed solution works like that:

1. updates sorted first by update number within each module
2. then updates are sorted between modules with modules dependencies - this extra step is added by this patch
3. then updates are sorted explicit hook_update_dependencies() rules

anyway, in theory ... I think core lacks such complex test coverage, so can't tell for sure.

@alexpott, my updates does preserve hook_update_dependencies() only when there is no dependency between modules.

So, it needs some work. I've spent some time on it and was not able to achieve significant progress. I guess, we need to get back to #53 and (maybe) create a follow up to fix the order for every single module.

The idea is to:

1. order by number
2. order by explicit dependency via the hook
3. order by dependencies

@Taran2L I think the explicit dependency needs to be the last thing - it's there to resolve super gnarly issues.

Unpublishing the second MR (and also hiding the previous patches before the first MR for clarity). Thanks all!

I tried to grok the patch and I am not sure I understand all of it.
But reading the description of _update_prepare_graph_for_sort it seems to me that a much simpler algorithm could be used to change the weights: double all the weights and subtract 1 if it is from `system` (or add 1 if it is not from system)

ie

function _update_prepare_graph_for_sort(array &$graph) {
  foreach ($graph as $update_name => &$update_data) {
    $update_data['weight'] =  $update_data['weight'] * 2;
    if ($update_data['module'] === 'system') {
      $update_data['weight'] = $update_data['weight'] - 1;
    }
  }
}

I am not sure I can say something about the rest of the patch without digging in more.

The suggestion is to revert back to #53, so here is a reroll of that for D10.

Now to address the feedback in #55

Patch at #85 was never reviewed

Tagging for issue summary update, but likely will be 'address feedback in #55'

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.