Coming from #633332: hook API documentation files have no information about where hooks are to go: It seems clear that implementations of hook_schema_alter() SHOULD live in .install files. But do they have to? Is what hook_schema_alter() does undone on de-installation? If not, should we add a note saying so?

Comments

jhodgdon’s picture

I don't think that hook_schema_alter() belongs in a .install file, and as things are now I doubt it would work.

The thing is, this hook alters *other* modules' hook_schema implementations. My understanding of the module .install file is that it is for functions related to installing/enabling/disabling *this* module. Not everyone agrees -- see #1308610: move the location of hook_modules_uninstalled() to install files -- maybe this issue should be combined with that one, as it's kind of related in my mind at leat?

sven.lauer’s picture

Title: Document that hook_schema_alter() should (must?) live in the module's .install file » Improve hook_schema_alter() documentation.
Issue tags: +Needs backport to D7

Ah, you are right. Apparently, I was not thinking clearly (somehow I was thinking of schema_alter()ing as an on-install task. But the hook is called whenever the schema information is rebuilt, so it likely needs to be in the .module file.

Leaving this open because:

1. Should we document that hook_schema_alter() needs to / should go into the .module file? I know this is the silent default, but given hook_schema_alter()s close connection to hook_schema() (which MUST be in the .install file), an additional note might not hurt?

2. The precise use and effect of hook_schema_alter() could be made clearer. The current version says:

When a module modifies the database structure of another module (by changing, adding or removing fields, keys or indexes), it should implement hook_schema_alter() to update the default $schema to take its changes into account.

Implicit in this is that just implementing hook_schema_alter() will NOT change the database (at least for already-installed modules). Maybe we should make this explicit?

In general, it is a little unclear (to me) how one would use hook_schema_alter() in conjunction with hook_(un)install(): I.e. how is the following scenario to be handled:

Module A alter()s module B's schema and, in A_install(), it makes the corresponding changes to the database.

Now suppose that A is installed before B. A's schema_alter() will be available and called when schema information is collected. However, since A's hook_install() was called before B even was there, the database will go unaltered. This would mean that the schema information and the database are out of sync, right?

Perhaps the intention is that a module should only schema_alter() parts of the schema of modules it depends on (i.e. in the above example, A should depend on B, preventing the scenario from happening). If so, this should be documented.

jhodgdon’s picture

RE #2 - I see no problem with idea #1.

Idea #2... I don't know the answer to your question, but presumably if module B is altering module A's schema, module B would have module A as a dependency, so you would never be able to have module B enabled/installed without module A also being enabled, right? Also, just consider that API doc is not where to write a tutorial on how to write a module. The place for that is the online drupal.org/documentation, or the Examples for Developers project. All of that said, if you can think of some short snappy updates to the doc that would clarify it, I'm all for it. :)

sven.lauer’s picture

Well, I was not suggest to provide a full explanation of this use case (or a tutorial for using this function), but rather I was tryting to illustrate how I don't quite understand what the hook does / is supposed to do after reading the current doc. Minimally if this

presumably if module B is altering module A's schema, module B would have module A as a dependency, so you would never be able to have module B enabled/installed without module A also being enabled, right?

is actually an assumption that drupal makes, it should be documented in the API doc (and this is not obvious---think about module A that providing optional integration with B if it is present, then you would not have a dependency, but A might still do something to B's schema (though perhaps it should not).

dave reid’s picture

I'm not quite sure I understand why this hook needs to move from .install profiles. I can see clearly that drupal_get_complete_schema() from drupal_get_schema() loads all module's .install files. It's always been my assumption that if you are altering another schema, you also need to create your fields/indexes manually in your module's hook_install() (with checks like db_field_exists() to ensure you're not attempting to add it if it exists) as well as remove them in your hook_uninstall(). Maybe we just need to document this?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jp.stacey’s picture

Issue summary: View changes
Status: Active » Closed (won't fix)
Issue tags: -Needs backport to D7 +SprintWeekend2017

As I think the thread is in agreement that (a) hook_schema_alter() has to be in .module, (b) .module is the default and (c) changing this behaviour would be a code issue, not a documentation issue; and as this has been untouched for five years: I'm going to close it.