As pointed out in the development list, all install files should have a consistent layout. yched proposed

1. hook_schema()
2. hook_install()
3. hook_uninstall()
4. hook_update_N()

This order looks reasonable. However, I've also seen hook_enable() and hook_disable() in some install files. Enhancing the above list with them:

1. hook_schema()
2. hook_install()
3. hook_enable()
4. hook_disable()
5. hook_uninstall()
6. hook_update_N()

Changing existent install files will most likely break many PNR patches in the current queue. Thus, delaying this commit might be considerable. On the other hand, contrib module developers might already start to port their modules to 6.x using the current (partially wrong) layout. Furthermore, the (preferred) layout should be documented somewhere, however, I've no idea where. I'm open to create a patch to update existing install files.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KarenS’s picture

And what about hook_requirements() which may also live in the .install file? Are there any others?

And should we go ahead and suggest (or require?) that all these hooks live in the install file rather than module code for consistency?

yched’s picture

Yeah, I forgot enable/disable (can/are meant to ?) live in .install (that's because we have them in content.module for cck ;-) ). When you look at the code in module_enable(), it's not *that* clear.

So, updated proposal :
1. hook_schema()
2. hook_requirements()
3. hook_install()
4. hook_enable()
5. hook_disable()
6. hook_uninstall()
7. hook_update_N()

Suggested places for docs :
- http://api.drupal.org/api/file/developer/hooks/install.php (needs to be updated with a hook_schema too - hook_schema doesn't seem to be currently documented anywhere on a.d.o, btw)
- http://drupal.org/node/114774 (D5-D6 upgrade page), like just below the 'New format for hook_update_N()' item ?

I agree with Karen and sun that the current policy about what hook lives where is not clear; not sure either where to put recommendations about it (and actually *enforcing* hook_enable in .install might be tedious, codewise...)

I'd say sun's point about contrib authors having started their D6 port makes it likely we go ahead rather sooner than later. I'm not sure *that* many patches will get broken (er, the 'document db schema' patch will be for sure...)

Anonymous’s picture

My opinion is that .install should contain hook_install, hook_update_N and hook_uninstall only, hook_schema should remain in .schema and the up to the module maintainer for everything else. It could be possible to use the .schema files in tools that have no reason to require the .install functions. And the sneak change in post freeze (post beta even) isn't really welcome by many. If this change is to remain it should be moved to D.7.

yched’s picture

earnie: this has *already* happened : http://drupal.org/node/150245
This thread is about how to organize things on top of that.

Any concerns about whether hook_schema in .install is a good thing or not belong in the original thread (and would probably have been welcome best while it was CNR :-)

sun’s picture

Priority: Normal » Critical

contrib authors having started their D6 port makes it likely we go ahead rather sooner than later

Thus, marking this critical; also to get some attention by core and core-alike maintainers.

We can certainly discuss this issue until http://drupal.org/node/164983 hits core.

chx’s picture

Priority: Critical » Minor

how could this be critical?? Critical is when some big part does not work. Normal is when something is broken. Minor is something that we survive if it's not fixed. I can't see what's broken here, so minor.

webchick’s picture

Version: 6.x-dev » 7.x-dev
Component: install system » documentation

Yes! Let's do this. In fact, I think I have a duplicate bug somewhere about this.

Moved to documentation even though it is not, because it doesn't affect the underlying subsystem at all, and is mostly of interest to people who crave consistency and ease of explaining things to people. Seems like the docs team is a pretty good match. :)

On a quick scan of this issue, I like yched's proposal in #2.

sun’s picture

Assigned: Unassigned » sun

I'll revisit this after ALPHA1.

sun’s picture

Assigned: sun » Unassigned
duellj’s picture

Status: Active » Needs review

This would be very helpful to standardize.

Here's a patch that reorders *.install files according to #2 (new list updated for hook_field_schema() and hook_update_dependencies()):

  1. hook_schema()
  2. hook_field_schema()
  3. hook_requirements()
  4. hook_install()
  5. hook_enable()
  6. hook_disable()
  7. hook_uninstall()
  8. hook_update_dependencies()
  9. hook_update_N()

The only thing else we might want to standardize private functions within install files.

duellj’s picture

Forgot to attach patch.

jhodgdon’s picture

Status: Needs review » Needs work

The last submitted patch, drupal.consistent_install_181955.11.patch, failed testing.

webchick’s picture

Version: 7.x-dev » 8.x-dev
duellj’s picture

Status: Needs work » Needs review
FileSize
164.38 KB

Updated list to include hook_modules_enabled() and hook_update_last_removed():

  1. hook_schema()
  2. hook_field_schema()
  3. hook_requirements()
  4. hook_install()
  5. hook_enable()
  6. hook_disable()
  7. hook_uninstall()
  8. hook_update_dependencies()
  9. hook_modules_enabled()
  10. hook_update_last_removed()
  11. hook_update_N()

And new patch for 8.x.

jhodgdon’s picture

Component: documentation » other
Issue tags: +Coding standards

This is not really a documentation patch, so moving to "other" and adding the "coding standards" tag, since it's really about coding standards.

So... The first step would be to actually agree on a standard. Has the coding standard this patch is attempting to enforce been sufficiently agreed upon by the community? If so, where is it documented?

thedavidmeister’s picture

Issue summary: View changes
Status: Needs review » Needs work

as per #16 it needs to be clearer that this standard has been discussed, agreed upon and documented somewhere before we can accept patches to change these files.

neetu morwani’s picture

Issue tags: +Needs reroll
jhodgdon’s picture

Title: Consistent .install file layout » [policy, then patch] Consistent .install file layout
Project: Drupal core » Drupal Technical Working Group
Version: 8.0.x-dev »
Component: other » Miscellaneous
Issue tags: -Needs reroll

It is premature to reroll this patch until we have agreed upon a standard. And actually I think this belongs now in the TWG issue queue, where standards are discussed.

tizzo’s picture

Project: Drupal Technical Working Group » Coding Standards
tizzo’s picture

Issue tags: +needs announcement for final discussion

Tagging as I think this is ready for final discussion.

effulgentsia’s picture

Priority: Minor » Normal

I agree that this is ready for an announcement for final discussion, and we'll include it in the first batch of those which is coming in January. Meanwhile, raising the priority to Normal, because I don't see this as any lower a priority proposal than other coding standard proposals.

tizzo’s picture

Status: Needs work » Needs review
Issue tags: -needs announcement for final discussion +final discussion

We believe this issue is ready for final discussion and are announcing and tagging it appropriately. Final ratification will require a sanity check on the full list of hooks. It is worth noting that the proposal made in comment 15 has not been incorporated into the issue summary’s proposal.

pjcdawkins’s picture

This seems unnecessary to me, hard to program an IDE to enforce, and I am guessing it would cause hundreds of contrib modules to be in breach of the standard as soon as it is ratified, which seems to me to defeat the point of a standard.

Perignon’s picture

I'm with @pjcdawkins on this. This seems unnecessary. Additionally, why have a hook if you don't need it.

xjm’s picture

It's unclear to me whether this issue is about which hook implementations should be in the .install file, or what order they should be in. The former makes a difference because it impacts what code is loaded at install versus normal runtime. Specifying a specific order for the functions, on the other hand, seems completely ridiculous to me. There is no expectation anywhere else that functions or methods should be in a particular order, so I see no reason this one file type should have such a tedious and unnecessary restriction.

xjm’s picture

Oh also, the proposed standard does not entirely reflect the relevant hooks for D8 (neither the summary nor comment #15). Edit: and that's another reason not to change this if it's about ordering; we'd have to change the standard every time we change the APIs.

I think maybe the intent of this proposal (if it is about ordering) was to have the update functions easy to locate? If so, I think the relevant standard is actually our use of @defgroup and @ingroup for those, rather than the ordering in the file. (See a core update hook implementation for an example.)

pfrenssen’s picture

I don't think this has any value and will make writing install files more complicated for no gain. -1

dawehner’s picture

In general I think in a Drupal 8 world the install file is much less relevant. You almost never create own custom tables, fields + KV gives you pretty much everything you
can ever imagine. Also hook_install()/hook_uninstall() is much more rare, given that configuration is shipped in its own folder now, which covers stuff, which used
to be in hook_install() like custom fields etc.

It would be nice to actually write into the summary some reasoning why forcing a certain order actually gives you some profit. Its not like you read that file from the top to the bottom
in order to understand what is going on. Also when writing it, enforcing some rule would be just annoying, because there is none in module files. You just add a new function normally done.

In summary it feels like the additional structure would not help really but rather set a higher burden.

yched’s picture

FWIW, back when @sun an I made the original proposal (8 years ago, it was D6 beta !), the .install file was a very telling entry point for understanding a module's intent and data model :
what db schema does it add, what does it do on install.

As @dawehner points, that's very much less the case now that modules rarely define any custom tables at all, and that install steps mostly consist of declarative config files imports rather than imperative API calls.

So yeah, this proposal probably doesn't make much sense 8 years later :-)

pjcdawkins’s picture

Status: Needs review » Closed (works as designed)
Issue tags: -final discussion

Thanks yched, it looks like there's a strong consensus to me so this can be closed.

David_Rothstein’s picture

Status: Closed (works as designed) » Needs review

I don't think this should be an official coding standard (i.e. this issue may be in the wrong queue) but I do think the original goal makes sense, i.e.:

  1. Have a recommended layout, which can be documented in How-To guides such as https://www.drupal.org/node/876250. (This would also serve as a good list of which hooks are supposed to be in that file in the first place, which almost everyone - including, I think, this issue - gets wrong at one time or another.)
  2. Make Drupal core follow the recommended layout.

The reality is we already do have a de facto standard for the most popular hooks like hook_schema(), hook_install(), and especially hook_update_N(). Because of that, every time I want to use one of the less popular hooks (especially hook_requirements()) I always debate with myself where in the file to put it... it never seems obvious where it should go. Having a semi-official pattern to follow would make the decision easy.

David_Rothstein’s picture

From the list in #15:

1. hook_schema()
2. hook_field_schema()
3. hook_requirements()
4. hook_install()
5. hook_enable()
6. hook_disable()
7. hook_uninstall()
8. hook_update_dependencies()
9. hook_modules_enabled()
10. hook_update_last_removed()
11. hook_update_N()

This looks mostly good to me, except for hook_modules_enabled() (that should not be in an .install file). I think it's reasonable for both Drupal 7 and 8 (except two or three of those hooks don't exist in Drupal 8 anymore).

In general I think in a Drupal 8 world the install file is much less relevant. You almost never create own custom tables, fields + KV gives you pretty much everything you can ever imagine. Also hook_install()/hook_uninstall() is much more rare, given that configuration is shipped in its own folder now, which covers stuff, which used to be in hook_install() like custom fields etc.

I don't think this is entirely true.

Using grep I find 16 instances of hook_install() in Drupal 8 core, and 16 in Drupal 7 core (in both cases excluding test modules) - i.e. the exact same amount. Granted there are more modules in Drupal 8 than Drupal 7 but still it is used frequently in both versions.

For hook_schema() there are 28 in Drupal 7 and 16 in Drupal 8, which is a bigger difference, but still a pretty popular hook in Drupal 8.

And of course hook_update_N() (and the various other hooks related to it) are going to be used all the time in Drupal 8 as well.

Artusamak’s picture

Having an official recommendation seems to be a good idea.
Having an obligation to follow this recommendation seems a bad idea. This will be overwhelming for most of the contrib maintainers and as said above there is nowhere else such obligation. For this reason i'm really not in favor or this standard being mandatory to follow.

hugronaphor’s picture

Issue summary: View changes
xjm’s picture

Hmm, I really do not see the value. PHP is not bash. The order of your functions in your file is irrelevant. Any editor, IDE, or documentation that implies it is relevant is misinformation IMO. The "recommended" order of hook implementations in what is, let's be honest, a legacy file that we should deprecate by D9, seems to me to be an example of bikeshed painting. It is trivial, of minimal value, and possible to debate endlessly. It also distracts from what is actually relevant, like using documentation groups that IDEs and api.d.o can parse to create lists of the relevant implementations.

Having a handbook page of what functions go in the file is fine (but does not belong in this queue). Having an issue to decide what order they should be in on that handbook page, or coding standards to enforce that ordering, is not a good idea I think.

xjm’s picture

Make Drupal core follow the recommended layout.

Also, please note that coding standards patches for D8 core are not accepted unless implemented as official standards (with a coder rule, where possible). The long-term goal is to never have a patch marked NW for a coding standard that could be checked by a machine, but isn't, because the style nitpicking for unknown/unclear standards is one of the single most damaging parts of the contributor experience for Drupal core. Therefore, a patch to reorder functions based on a non-standard is not in line with our release management goals for Drupal 8, and we would not commit such a patch.

David_Rothstein’s picture

Even among the current coding standards there are some that are only recommendations already, for example: https://www.drupal.org/coding-standards#quotes

The general principle there (which is basically "when in doubt, be consistent with existing code" - and which perhaps should be a coding standard of its own) is a good one and is primarily what this issue is about also.

Code which is in a consistent, logical order is easier to read. I could write a module that puts hook_help() at the bottom of a .module file rather than the top, orders update functions in non-numerical order, puts the name key in an .info file somewhere other than the first line, etc. - it might not violate any coding standards but that doesn't mean it's a good idea.

I generally wouldn't want someone to mark an issue "needs work" over these kinds of things alone (I try not to even do that with official coding standards myself) but that doesn't mean a patch to make the codebase more self-consistent is a bad idea that shouldn't be committed to Drupal core. I don't see how the two things are related.

I think this issue would be better off in the core documentation queue rather than the coding standards queue.

xjm’s picture

Thanks David. I guess maybe we should let the TWG make the determination on whether this proposal will become a standard or not, and then if the answer is "No", discuss the core policy implications of a "recommended" standard in the core queue.

However, I do feel extremely strongly about this topic (not making core subject to non-official coding standards), and I feel that for the D8 branch we have already made the process decision on how coding standards patches will (and won't) be made. Reference: https://www.drupal.org/core/scope#coding-standards That policy is the result of a lot of work and experience with coding standards cleanup initatives, with myself, @alexpott, @jhodgdon, and countless community members. If we aren't aligned in terms of D8 and D7, we can explore how to solve that. But I'd really prefer to let the decision stay made and not reopen it and set us back by months in that area. So I'd prefer not to have a core issue, because honestly if it were a D8 issue I'd really feel it were a wontfix. But if you think it's important enough to discuss in the core queue, then we can re-discuss in that issue.

tizzo’s picture

Status: Needs review » Closed (won't fix)

The Coding Standards Committee reviewed the recent comments, and it seems to us that this proposal does not have 2 current sponsors (per step 2 of https://www.drupal.org/project/coding_standards) for making this an official coding standard. Therefore, we're marking this "won't fix".

I guess maybe we should let the TWG make the determination on whether this proposal will become a standard or not, and then if the answer is "No", discuss the core policy implications of a "recommended" standard in the core queue.

We feel such a follow-up is outside the scope of the Coding Standards project, and leave it to the community to decide on whether to open a core issue for it.

David_Rothstein’s picture

Title: [policy, then patch] Consistent .install file layout » Make .install file layouts consistent
Project: Coding Standards » Drupal core
Version: » 8.1.x-dev
Component: Miscellaneous » other
Status: Closed (won't fix) » Needs work
Issue tags: -Coding standards

I think it would be best to to continue using this issue since it started off in the core queue originally, and all the discussion/patches are already here (which helps with accurate issue credits).

we have already made the process decision on how coding standards patches will (and won't) be made. Reference: https://www.drupal.org/core/scope#coding-standards That policy is the result of a lot of work and experience with coding standards cleanup initatives

I don't disagree with anything on that page. I'm just saying this isn't a coding standards issue, so it shouldn't apply here. Meaning concretely: This shouldn't be considered a bug fix, I don't think there should be a Coder rule written to enforce a particular .install file layout, etc. I think it's just a regular proposed improvement to make the codebase more self-consistent.

xjm’s picture

So for me, from a release management perspective, this issue is wontfix. Moving functions around in a file is disruptive for no reason, and if it becomes a "secret standard" that is not enforcible as a coding standard, that creates a situation where the supposedly recommended ordering will constantly regress and people in on the "secret" will mark others' patches NW over it (or otherwise ask patch contributors to change it), resulting in bad contributor experience. I would not commit this to D8, and if it were committed I would ask for a revert, because I do think it contradicts the coding standards policy. (I consider "Make X consistent" to be exactly what coding standards are.)

xjm’s picture

Also, I'm not sure about moving any issue out of the coding standards queue in general, because that means it is not possible to use that queue to find out what decisions the TWG has already made.

xjm’s picture

Also also, even if we were to accept the premise that this not a about a pseudo-coding standard (which I don't), most commenters on this issue have already voiced their feedback that they do not think this change is worthwhile.

David_Rothstein’s picture

I don't think most recent commenters said this wasn't worthwhile for core, just that it shouldn't be an official coding standard. And overall when this issue was just filed against core it received more support than opposition.

Good point about being able to use the Coding Standards queue to find that this proposal was already rejected there. I created a related issue there now and marked it "won't fix".

David_Rothstein’s picture

Here's what a patch looks like, for what it's worth.

The first leaves hook_requirements() out (since that is a trickier change for a couple reasons). This one seems like a pretty clear improvement in code organization to me.

The second is the full thing.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

andypost’s picture

Making that suggestion it makes sense to take into account "_module_install_helper_function()" related to install file.
Suppose they should be after hook_uninstall()
Is there related docs pages to update?

catch’s picture

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

@David the patch in #46 no longer applies, because we've removed all but four tables from system_schema() - and there are open patches for at least key/value too. I'd expect us to move on to other core modules once we're done with system. So any time this would have gone in in the past three months, or now, it would have conflicted with patches that are working towards making it irrelevant.

So this is really won't fix for 8.x, moving back to 7.x in case you still think it's useful there.

David_Rothstein’s picture

Version: 7.x-dev » 8.2.x-dev
Status: Needs review » Needs work

This issue is still relevant even for modules that don't implement hook_schema().

However, if it can't get in because the code is changing too much, it could be postponed (or even moved to Drupal 9)...

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.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.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.