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.
Comment | File | Size | Author |
---|---|---|---|
#46 | install-file-layout-181955-46.patch | 53.35 KB | David_Rothstein |
#46 | install-file-layout-181955-46-no-hook-requirements.patch | 15.39 KB | David_Rothstein |
Comments
Comment #1
KarenS CreditAttribution: KarenS commentedAnd 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?
Comment #2
yched CreditAttribution: yched commentedYeah, 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...)
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedMy 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.
Comment #4
yched CreditAttribution: yched commentedearnie: 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 :-)
Comment #5
sunThus, 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.
Comment #6
chx CreditAttribution: chx commentedhow 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.
Comment #7
webchickYes! 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.
Comment #8
sunI'll revisit this after ALPHA1.
Comment #9
sunComment #10
duellj CreditAttribution: duellj commentedThis 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()):
The only thing else we might want to standardize private functions within install files.
Comment #11
duellj CreditAttribution: duellj commentedForgot to attach patch.
Comment #12
jhodgdon#11: drupal.consistent_install_181955.11.patch queued for re-testing.
Comment #14
webchickComment #15
duellj CreditAttribution: duellj commentedUpdated list to include hook_modules_enabled() and hook_update_last_removed():
And new patch for 8.x.
Comment #16
jhodgdonThis 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?
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commentedas 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.
Comment #18
neetu morwani CreditAttribution: neetu morwani as a volunteer commentedComment #19
jhodgdonIt 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.
Comment #20
tizzo CreditAttribution: tizzo commentedMoving this issue to the Coding Standards queue per the new workflow defined in #2428153: Create and document a process for updating coding standards.
Comment #21
tizzo CreditAttribution: tizzo commentedTagging as I think this is ready for final discussion.
Comment #22
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI 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.
Comment #23
tizzo CreditAttribution: tizzo commentedWe 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.
Comment #24
pjcdawkins CreditAttribution: pjcdawkins commentedThis 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.
Comment #25
Perignon CreditAttribution: Perignon commentedI'm with @pjcdawkins on this. This seems unnecessary. Additionally, why have a hook if you don't need it.
Comment #26
xjmIt'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.
Comment #27
xjmOh 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.)
Comment #28
pfrenssenI don't think this has any value and will make writing install files more complicated for no gain. -1
Comment #29
dawehnerIn 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 usedto 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.
Comment #30
yched CreditAttribution: yched commentedFWIW, 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 :-)
Comment #31
pjcdawkins CreditAttribution: pjcdawkins commentedThanks yched, it looks like there's a strong consensus to me so this can be closed.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.:
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.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedFrom the list in #15:
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).
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.
Comment #34
ArtusamakHaving 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.
Comment #35
hugronaphor CreditAttribution: hugronaphor as a volunteer and at Dennis for Dennis commentedComment #36
xjmHmm, 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.
Comment #37
xjmAlso, 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.
Comment #38
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedEven 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.
Comment #39
xjmThanks 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.
Comment #40
tizzo CreditAttribution: tizzo commentedThe 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".
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.
Comment #41
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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).
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.
Comment #42
xjmSo 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.)
Comment #43
xjmAlso, 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.
Comment #44
xjmAlso 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.
Comment #45
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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".
Comment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHere'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.
Comment #48
andypostMaking 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?
Comment #49
catch@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.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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)...