Apparently #1018602: Move entity system to a module sneaked in this hook but we don't have docs to explain what it does.

This was originally a critical upgrade path issue due to the unpredictability of using hook_update_N() to enable modules during major upgrade. However now that major upgrades are handled by the migration API, we can relax things for minor updates.

Remaning tasks:

Remove the implementations of hook_schema_0()/hook_schema_SCHEMA_VERSION() and all references to it in core.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

That issue doesn't actually even mention that hook, it was just borrowed from #1239370: Module updates of new modules in core are not executed and merged in.

sun’s picture

The fact that #1239370: Module updates of new modules in core are not executed is still open should sufficiently clarify that hook_schema_0() is just a ugly hack at this point. ;)

We can document that it's == hook_schema() but only for the case of when being installed via update_module_enable(), in which case all existing updates that are being defined in the .install file are ignored, and update.php (or a subsequent invocation of update.php) will expose all of those existing updates as updates that haven't been executed yet.

I can think of a dozen of edge-cases in which this will blow up in contrib. We can document all of that, but it will take a novel :)

catch’s picture

Status: Active » Postponed

I'm not sure it's worth adding docs here until #1239370: Module updates of new modules in core are not executed is resolved.

What we really need to do is #1052692: New import API for major version upgrades (was migrate module) instead of piling hack upon hack to make the existing major upgrade system work, but since we're stuck with hacks for another 5 years we should probably sort out what exactly those hacks are going to do.

catch’s picture

Status: Postponed » Active

Meh unpostponing again, we could add stub docs for this.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
2.45 KB

Starter docs for this, since @Wim Leers complained for lacks of docs in #2074193: Add changed time tracking to custom blocks.

jhodgdon’s picture

Status: Needs review » Needs work

Minor grammar point: "This schema is used when a new module is installed in the update process,
it has the same structure as hook_schema()." is a comma splice. The comma needs to be either a ; or a . with a new sentence.

I cannot comment on the accuracy of the docs, so I'll leave others to review that... But I'm not sure after reading the above sentence in the docs what it means. What's the difference between this and hook_schema() exactly? How is a new module installed during updates?

tstoeckler’s picture

I've never understood why hook_schema() isn't sufficient for modules that are installed in the update process. I am absolutely certain there is a reason for that, but I think these docs should (finally! :-)) make it clear to me. In other words, I think we could be a little bit more verbose about why and when this is needed.

effulgentsia’s picture

catch’s picture

@tstoeckler yes that's what's missing from the current docs.

Here's an example of the kind of situation that breaks if you just use hook_schema(), and how hook_schema_0() fixes it.

- modules A installs module B in A_update_8100() - written in 2013. It also migrates some of module C's data to module B's tables in A_update_8101().

- module B changes its schema in 2014 with an upgrade path for the data in 2014, that's in B_update_8101().

If in 2015 someone upgrades their site to Drupal 8 and runs those updates, module B will get installed with the new schema in hook_update_8100(), then the data migration in hook_update_8101() will try to insert data into the old schema (because when it was written that was correct). Tables or columns could have been renamed.

Having hook_schema_0() means that you can install module B in either 2013 and 2014 with exactly the same schema, hook_schema_0(), migrate the data into that schema, then module B's own updates handle getting it up to date.

#1239370: Module updates of new modules in core are not executed is required to actually get those updates to run, but these are all spin-offs of the same critical Drupal 7 bugs where this problem was discovered.

With hook_schema_0(), it should all work as long as you have dependencies that ensure an order of A_update_8100(), A_update_8101(), B_update_8100(), without having to change code in updates that have already been written and were correct at the time.

Having written all this, I've realised hook_schema_0() doesn't help at all if you want to install a module once it's already made a schema change after adding hook_schema_0() - you'd need hook_schema_7101() as well. So we might want to look at having update_module_enable() take a schema array as argument, and then the module doing the update has to specify the schema manually - either calling hook_schema_SCHEMA_VERSION() or copying the definition over from the other module's .install file, rather than trying to figure it out ourselves.

jhodgdon’s picture

When I started reading #9, I came to the same conclusion as the last paragraph: it doesn't seem like it's actually solving the problem completely, because if I write a module A in 2014 I am going to want to install module B with the 2014 schema.

This is one of the reasons that documentation for hooks should be written when the hooks are invented. If you can't write coherent documentation for a hook, it's a good clue that the hook has some conceptual problem in it.

Should we postpone this issue? Because it doesn't seem like hook_schema_0 is going to be the final answer to this problem.

jhodgdon’s picture

Is this still an issue?

If so, webchick's original comment about "put it at major and hope it gets resolved quickly" did not happen. Does this hook exist, and is it documented?

Gábor Hojtsy’s picture

hook_schema_0() is still alive and well in core. Not sure if we want to keep this practice anymore given the switch to migration path though. However as far as core does, there are 2 implementations and no docs for this "hook".

$ git grep "_schema_0" *
core/lib/Drupal/Core/Extension/UpdateModuleHandler.php:      $function = $module . '_schema_0';
core/modules/block/custom_block/custom_block.install: * Implements hook_schema_0().
core/modules/block/custom_block/custom_block.install:function custom_block_schema_0() {
core/modules/views/views.install:function views_schema_0() {
catch’s picture

We discussed this at Prague in the context of migrate in core. It may not be an issue, or it may be a sufficiently small issue that we could punt on it.

The main reason this is a problem in Drupal 7 and earlier, is because we've not been able to reliably distinguish between hook_update_N() run more or less in isolation during minor upgrades, compared to hook_update_N() which could be run as part of a major upgrade at any point within a 5 year cycle (i.e. upgrading from Drupal 6 to Drupal 7 now vs. four years ago with the same contrib modules could have a very different upgrade path).

With migrate, that problem has completely disappeared, which leaves us only with the following edge case:

During a minor update, module A enables module B which is in a separate and unrelated project, and module B's schema is updated either before or after that update is written, with its own set of updates. Since the two projects could be upgraded at different times, there's no guarantee of compatible schemas.

For modules in the same project, we can expect the module author to handle this - since we know the code is going to be in the same state for both modules - since they're included in the same project anyway.

I often write custom hook_update_N() with module_enable() in, but in those cases the entire site is in git with known versions of contrib modules, so any issues get resolved by the person writing the hook_update_N() for that custom site.

Enabling a module in a completely separate contrib project is enough of an edge case, and one that can probably be handled with update dependencies and careful monitoring, that I think we could happily delete all references to hook_schema_0(), both from core and hopefully one day from our own memories.

Gábor Hojtsy’s picture

I guess the more concrete question here is if the existing hook_schema_0() implementation should be removed. If not, we need to document it :)

catch’s picture

Title: Missing docs for hook_schema_0() » Remove all references to hook_schema_0()
Category: Bug report » Task
Priority: Major » Critical
Issue summary: View changes
Status: Needs work » Active
Issue tags: -D8 upgrade path +Novice

Pinged chx in irc and he also thinks this is no longer necessary.

So re-purposing this - we should remove the implementations and any references.

jhodgdon’s picture

Title: Remove all references to hook_schema_0() » Remove all references to/implementations of hook_schema_0()
Component: documentation » other

Not exactly documentation any more.

So, just checking... There are definitely only two implementations of this hook in core, in core/modules/block/custom_block/custom_block.install and in core/modules/views/views.install.

But there is one place it is actually being invoked, in core/lib/Drupal/Core/Extension/UpdateModuleHandler.php in the install() method:

      // Check for initial schema and install it. The schema version of a newly
      // installed module is always 0. Using 8000 here would be inconsistent
      // since $module_update_8000() may involve a schema change, and we want
      // to install the schema as it was before any updates were added.
      module_load_install($module);
      $function = $module . '_schema_0';
      if (function_exists($function)) {
        $schema = $function();
        foreach ($schema as $table => $spec) {
          db_create_table($table, $spec);
        }
      }

All of this needs to be removed, right? And we're sure it's OK?

catch’s picture

That'll need to change to hook_schema().

We may in another issue want to see if it's possible to go back to enabling API calls in hook_update_N(), which would require wider changes all round.

JStanton’s picture

Assigned: Unassigned » JStanton

@ Boston Core Sprint

JStanton’s picture

Status: Active » Needs review
FileSize
5.66 KB

Removed all instances of hook_schema_0 in core and the calling of it.

jhodgdon’s picture

I think this is the right fix, and the tests are passing, so it probably is.

However, @catch in #17 said the spot in UpdateModuleHandler would need to be changed to use hook_schema() instead. I do not understand why, since presumably the schema is installed elsewhere normally for modules via hook_schema()? ?? I think this is fine... but will leave to catch to decide.

webchick’s picture

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

Let's do this.

catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs work

I do not understand why, since presumably the schema is installed elsewhere normally for modules via hook_schema()? ?? I think this is fine... but will leave to catch to decide.

No during updates, UpdateModuleHandler completely takes over the installation of modules - so I think this means that modules will be installed with no schema at all. I'm surprised tests don't fail though.

jhodgdon’s picture

Issue tags: -Novice +Needs tests

So it sounds like we need a test that does a module install during an update, and we need to demonstrate that it would fail without the invoke of hook_schema or hook_schema_0 there. Um. No idea how that would come about?

martin107’s picture

Issue tags: +Needs reroll

removing_hook_schema_0-1929816-19.patch not longer applies.

reszli’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.76 KB

I re-rolled the patch

sun’s picture

Status: Needs review » Reviewed & tested by the community

Looks good - views_schema_0() no longer exists in HEAD.

catch’s picture

Status: Reviewed & tested by the community » Needs work

I think we still have the issue described in #22.

reszli’s picture

If you can describe a bit more in detail what needs to be done, I may try.
Note: I'm new to D8.

catch’s picture

The first thing to do would be to manually test with the latest patch.

1. Install minimal profile

2. Add system_update_8001()

3. Inside that update, enable a module that defines hook_schema() (such as tracker)

4. Run the update

5. Check whether the schema is created.

From that, we'll either know we have a bug to fix, or need to figure out why removing the schema creation code from UpdateModuleHandler doesn't break anything.

penyaskito’s picture

FileSize
6.08 KB
1.4 KB

Tried with #1929816-25: Remove all references to/implementations of hook_schema_0(), and adding:

function system_update_8001() {
  module_install(array('book'));
}

The update.php process failed, and attached there is a fix.
However, after finishing the schema is not there still, and the schema_version is set to 0, which is reported to be from a previous version of Drupal so no updates are feasible anymore.

lokapujya’s picture

FileSize
2.71 KB

Now that UpdateModuleHandler is removed in #2233403: Let update.php invoke hooks again, I think we just need to remove the one hook_schema_0().

catch’s picture

Issue tags: -Needs tests +Novice
lokapujya’s picture

Status: Needs work » Needs review

Kick off a a test of the last patch.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Grepping for "schema_0" did not match anything else.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 31: 1929816-31.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
3.87 KB

The previous patch did not delete the whole function.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Oops, yeah, I overlooked that as well :-) Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Exxxxcellent. :)

Committed and pushed to 8.x. Thanks!

  • Commit e48eab8 on 8.x by webchick:
    Issue #1929816 by lokapujya, penyaskito, reszli, JStanton, Gábor Hojtsy...

Status: Fixed » Closed (fixed)

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