#1535868: Convert all blocks into plugins renumbered some module update numbers of User module.

1) Updates have dependencies; @see hook_update_dependencies().

2) Renumbering existing updates complicates matters for #1867972: Make HEAD2HEAD updates work and related issues.

3) Updates should never be renumbered. The consequences of doing so are simply not worth the effort.

CommentFileSizeAuthor
drupal8.block-updates-revert.0.patch1.01 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Blocks-Layouts
xjm’s picture

Status: Needs review » Reviewed & tested by the community

I remember wondering about this too, but I don't remember if there was a reason it was done or if I even brought it up.

Regarding update renumbering--this probably isn't the place to discuss this, but what about refactoring or adding to updates? E.g., updating every single variable to config in 20000 hooks seems suboptimal, and separate updates that (say) add and then delete a db field are a waste of time during the upgrade process. I thought we normally cleaned up upgrades before we started rolling unstables.

sun’s picture

This is indeed the wrong issue to discuss, but to give a very quick reply to #2 in order of the questions raised:

1) 20,000 module updates are not an issue at all, since all updates run in a batch. Batch API is well designed for the task and executes them in the optimal way. That's the least thing to worry about.

2) As long as updates are iterative, we should simply add new updates. Anything else and going out of line adds complexity only. In almost all cases, complexity leads to unexpected bugs. Especially in case of updates, which require us to travel back and forth in time, with different environments, configurations, and data sets.

3) The exception to the rule is when we're changing our minds. For instance, when introducing a {config} database table for storing configuration and replacing that with a file-based storage later. In such cases, we want to delete the previously added update - and - potentially add a new one. However, we should generally try to avoid replacing an existing update with a different one. (E.g., in the concrete case of the update that added {config}, we replaced that to add a {cache_config} table, which is an entirely different thing.) In any case, these are well-known exceptions to the rule and only natural for something that undergoes major changes. These kind of updates can only be resolved by a specialized head2head upgrade script, which is essentially capable of "rewriting history" (and correctly determining when history needs to be rewritten).

4) Yes, we rewrote updates in the past, but we stopped doing so. Apparently there are still some open D7 issues in the queue that wanted to completely rewrite the updates of various modules. Those rewrite/clean-up actions never really made any sense, because it's a tedious and very complex task to get right and which involves a large risk of unintentionally breaking the upgrade path due to missing test coverage (whereas a 100% test coverage is close to impossible to reach for upgrade path tests, since there's an infinite amount of scenarios). Lastly, sorta needless to say, upgrading is a one-time operation, and no one really cares how well or clean the code is written, as long as it works. The only exception to that are performance reasons, but fixing performance issues normally means to touch the code within a single update only - not other updates.

In summary:

  • KISS: Iterative.
  • Accept that there are exceptions to the rule and know them very well.

Technically, the above list should be amended with the following, but since it's less objective, I left it out:

5) Each time you have to reinstall HEAD (and each time you had to reinstall HEAD in the past year), because you can't recover via update.php, you can be dead sure that the upgrade path is broken. Reality is that we could make all of our lives much easier we'd stick 100% to the iterative update/upgrade procedure, even when changing our minds later. Each time we go out of bands and rewrite history, we're opening up a new parallel dimension that is factually untested, except for the very limited upgrade path test coverage we have in core. If you take a (large) step back and consult any uninvolved person nearby, he/she is guaranteed to ask you why we're masochists like that. Frankly, there's no reasonable answer to that, except for the typical engineering yadayada of "we want clean", but that's pure idealism/perfectionism, and thus the cost/benefit ratio of that argumentation is beyond logical/human sense. The additional cost is a lack of head2head updates, complete loss of real world upgrade path testing (with varying environments and configurations), an exceptional amount of buried/wasted developer/contributor time for testing and debugging, and ultimately, a long-delayed release date.

(And almost needless to say, strictly iterative head2head updates would be much easier to test continuously and automatically for every single patch in the queue: git checkout 8.x; install.php; git apply patch; update.php - the "upgrade path" is before vs. after the patch, by design.)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yeah it's not inconceivable that a contrib module already added hook_update_dependencies() based on these, so best to leave as is unless there's a pressing reason to change.

xjm’s picture

Issue tags: +Block plugins

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