#1097100: Remove all 7xxx update functions and tests (D6 to D7 upgrade path) shows that no-one really gets the point of the _update_7000_foo_bar() functions apart from about three people.
So we need proper documentation explaining why they're there, what they do, how to use them, what to do when a new one needs to be added.
This is the current documentation, which is a bit lacking - http://api.drupal.org/api/drupal/includes--update.inc/group/update-api-6...
We need to get the two critical issues in first before changing the docs (since for a start, those should not be named update-api-6.x-7.x), but I'm opening this as a marker.
Here's a rough example:
function comment_update_8000() {
foreach (_update_node_get_types_0() as $type) {
// Do something.
}
// Get a list of node types and change the name of a comment_node_type_$type() variable.
}
function node_update_8000() {
// Change the name of the node_type table to something else.
db_rename_table('node_type', 'node_types');
}
function comment_update_8010() {
// Change the name of another comment_node_type_$type() variable.
// Now comment_update_8000() needs to query the old table name, and
// comment_update_8010() needs to query the new table name. This means
// we'd need two _update_7/8/xxxx_node_get_types() functions to handle the
// two different schema versions.
foreach (_update_node_get_types_8000() as $type) {
// Do something else.
}
}
function node_update_8020() {
// Rename {node_types}.type to {node_types}.bundle
}
function comment_update_8056() {
foreach (_update_node_get_types_8020() as $types) {
// Do something else again.
}
}
Comment | File | Size | Author |
---|---|---|---|
#45 | 1134088-45.patch | 9.82 KB | star-szr |
#40 | 1134088-40.patch | 8.87 KB | star-szr |
#36 | 1134088-36.patch | 8.59 KB | star-szr |
#36 | interdiff.txt | 5.35 KB | star-szr |
#34 | 1134088-34.patch | 8.73 KB | star-szr |
Comments
Comment #1
catchEasy example:
Comment #2
plachI even tried to smell #1 but I'm still not sure I fully grok it :(
(subscribe)
Comment #3
quicksketchI was one of the ones that misunderstood the system, and after several discussions with catch finally understand why the utility update functions are named the way they are. Since now I'm one of four people that now understand the system, I'll take on this issue.
Comment #4
jhodgdonAs a note, there are currently no functions listed at http://api.drupal.org/api/drupal/includes--update.inc/group/update-api-6... -- so it looks like there are no functions that are members of that group? That also needs to be fixed -- use @ingroup... there's information on how to do grouping on http://drupal.org/node/1354.
It would probably also be good to make a see also link between that topic and hook_update_N, which seems like it might be related?
And as I'm also one of the people who doesn't have a clue what this is about, I'm glad quicksketch has taken on this issue. Thanks!
Comment #5
catchBumping this back to major, see the long and painful discussion at #1164852: Inconsistencies in field language handling.
Comment #6
xjmIs quicksketch still working on this? :)
In a sentence (this is catch on IRC):
Also:
+1
Comment #6.0
catchUpdating summary
Comment #7
catchThis is closely related to #1239370: Module updates of new modules in core are not executed.
Comment #8
jhodgdonOK... So what are we trying to do here on this issue?
Comment #9
catchIn a hurry this morning, but here's new pseudo-code for #1 since this has come up yet again and apparently #1 is impenetrable.
Comment #10
jhodgdonOK, that's a nice example, but where are we proposing to put this, and what are we trying to document here on this issue? I'm still confused.
Comment #11
catchI think we could put something like that in an @code inside the update-api group documentation, which is at http://api.drupal.org/api/drupal/includes%21update.inc/group/update-api-... - this would also need to explain why schema dependent and/or hook-invoking API functions can't be used in updates - i.e why this confusing situation exists at all.
Another example of how vague this is currently is #1439692: Rename field language properties to langcode which just renamed an update_7000_*() function to update_8000_*() despite there not being a schema change.
Comment #12
clemens.tolboomI'm missing hook_update_dependencies as that should dictate the order for update_N invocations.
I think I grok that each update_8000 must contain the full monty from upgrading a D7 site into D8 site?
Comment #13
xjmAlright, after reading #9 a couple times I think I finally am starting to understand the problem space.
hook_schema()
or simply writes a plain select query, doesn't necessarily match the current database structure at that point during the upgrade?Is this correct?
Comment #14
catchYes #13 is dead on. the update_xxx_*() functions deal with #1, and hook_update_dependencies() deal with #2 - we should probably add hook_update_dependencies() to the example here too.
Comment #15
xjmComment #16
star-szrComment #17
chx CreditAttribution: chx commented#13, choose the correct answer is 2. You can't fire a hook plain and simple because it might trigger a query in a module expecting a certain schema and it's not there yet. Nasty. (Why do you think I took off the upgrade maintainer badge for D8?)
Comment #18
xjmExample of this problem space in action: #1292470-115: Convert user pictures to Image Field
Comment #19
star-szrHere's a first draft.
This does not yet cover hook_update_dependencies().
Comment #20
star-szrOops! I just realized the example code should probably use function names like _update_7000_bar_get_types(), not update_7000_bar_get_types(). Revised.
Comment #21
jhodgdonThis looks really good, and I think it makes the idea much clearer, thanks!
A few thoughts:
a) I'm wondering if we could/should use the standard term "invoking" rather than "firing" a hook -- or is "firing" something different?
b) This documentation refers to the "schema version" twice, and while I personally know what that means, I'm not sure everyone would, especially since hook_update_N() does not use that term. So how about the first time the "schema version" is referred to, we say something like:
...create a new utility function named _update_N_my_function() where N is the schema version the function acts on (the schema version is the number N from the hook_update_N() where this schema was introduced, or a number following the same numbering scheme) , and my_function is...
c) I am also a bit confused about how the foo() update hooks would know which bar() schema version is in effect, and hence which bar update function should be called. That wasn't made clear by the example or the text.
d) I think the example code needs to tell which file each function should be placed in (foo.install, bar.install, ???). They should probably be grouped by file instead of mixing them up.
Comment #22
star-szrAll good points, I've made changes accordingly. I think C is where hook_update_dependencies() comes in, so I've added an implementation of hook_update_dependencies() to foo.install. Not sure yet how to incorporate an explanation of hook_update_dependencies() into the text, suggestions are welcome.
Should we have a @see to hook_update_dependencies() as well?
Comment #23
star-szrWhitespace fix.
Comment #24
jhodgdonLooking better!
I have a couple of suggestions:
a) Yes, it seems like we should have an @see to hook_update_dependencies().
b)
- second line - let's lose the word "given" (doesn't add any meaning).
- Also, this applies to not only invoking hooks, but to any other operation involving the schema, such as drupal_write_record(). And I think just saying "impossible to judge the consequences" is too limited -- really, you don't even know if it will work. So this whole sentence just needs to be rewritten (and maybe the following sentence too, which actually says more about the problems than this first sentence). And the final sentence of this paragraph about "not invoke any hooks" also probably needs to be broadened.
c) Given that you have put in hook_update_dependencies, I think in this:
the (for whatever reason) remark should be removed.
Comment #25
jhodgdonComment #26
star-szrHere's another update.
For this round I compared the update versions of a few of the functions side by side. The following are probably the clearest examples to look at, and at least the function signatures remain more or less the same.
_update_7000_field_create_field() vs. field_create_field()
and
_update_7000_field_delete_field() vs. field_delete_field()
field_create_field() also shows a specific example where drupal_write_record() is converted to db_insert() in the update version, so I used that to get more specific than "not invoke any hooks", and provided a link to the Database abstraction layer group as well.
One more idea in parting: what about linking the update versions of API functions to their "normal" counterparts, or just being more consistent with the naming? I had trouble finding field_sql_storage_field_storage_write() and node_type_get_types() because the update versions were not named consistently.
Comment #27
star-szrSorry, testbot! Corrected the punctuation after the @link as per http://drupal.org/node/1354.
Comment #28
jhodgdonThis is looking pretty good, thanks! I have a few suggestions:
a)
I think we shouldn't say "update hooks" here. Maybe "update functions (hook_update_N() implementations)" would be better? This terminology should also be used throughout the patch (later references could just say "update functions").
b) [EDIT: Fixed code tag here]
The Drupal project officially uses the serial comma (so we need a comma before "and" here).
c)
I think these code examples could be introduced better. I would add "that follows" to the first line here, and then make a better introduction to the code samples, like:
For example, file foo.install could contain:
And bar.install could contain:
d) See #4 above... What functions do we have that belong in this group? They need to get @ingroup added to them.
Comment #29
jhodgdonOne other suggestion: I think hook_update_N() needs a paragraph added that says something about this and points to this documentation for more details. This patch adds an @see, but it doesn't say anything about why. Maybe a slightly adapted version of this paragraph from the current patch:
with a line that says "See .... for details."
Comment #30
star-szrThanks @jhodgdon! I'll work on these revisions.
Regarding point D in #28: It looks like the update-api functions are already grouped with @ingroup in D7 and D8, or are there other functions that need to be added to the group?
Comment #31
jhodgdonYou are right -- some functions have been added... probably someone should look through the .install files and see if there are any more lurking.
The 8.x list looks suspicious to me though. Although it's supposed to be listing 7.x to 8.x functions, quite a number of them have 7000 in the list. I think probably there need to be two groups defined in the 8.x branch: one for 6-7 updates (left over from 7.x) and one for 7-8 updates (new functions). And the group header for the 6-7 updates should probably be short -- referencing the 7-8 updates page that will have all this nice documentation we've been working on for this issue.
Either that or we should change the group's ID so that it doesn't have 6.x-7.x or 7.x-8.x in the URL/group ID.
Comment #32
catchThe 7000 updates in 8.x are actually fine - as long as the schema hasn't changed since they were added, those functions should be used to update data in the tables they deal with.
Comment #33
jhodgdonGood point (#32/catch).
So let's change the @defgroup/@ingroup machine name to take the 6.x-7.x version parts out entirely. This will let the topics be linked on api.drupal.org (because they will have the same name), and better reflect that the update functions can be useful past a specific version's lifetime.
Comment #34
star-szrNotes and a summary of changes:
Would the naming conventions for these utility functions be slightly different in some cases for contrib? For example, if I'm creating a contrib module that uses a function from the core field module that doesn't have a utility version, I can't just add a utility function to the field module's .install. If I needed a utility version of field_update_field(), would I name my utility function _update_N_mymodule_field_update_field() to prevent clashes?
Also… interdiff_1000.txt :)
Comment #35
jhodgdonThanks, this is getting close!
One thing: we don't split up @link ... @endlink into separate lines, so this section needs a change in wrapping:
(move the whole @link/@endlink to a new line)
Also, can we remove the two-space indentation in the @code section?
I realize I may have suggested it sometime back but we're removing those now (sorry!) -- too much empty space.
Also, just a minor suggestion: I think these two paragraphs could/should be combined in system.api.php:
And I'm also wondering why the update-api group is in update.inc. Maybe it should be moved to system.api.php instead, so it's closer to the docs for hook_update_N(), hook_schema(), etc.?
Comment #36
star-szrHere's a new patch with those changes. I haven't moved the group to system.api.php just yet, I think it makes more sense where it is but I'm ready to be convinced otherwise :)
Comment #37
jhodgdonMy thinking on this being in system.api.php is that the hook_update_N() documentation is there, plus hook_schema(), etc. (the other hooks that belong in the module.install file). update.inc doesn't seem to contain functions related to this at all. But I'm also willing to be convinced -- why do you think it belongs in update.inc?
Anyway, other than that, this looks good. Thoughts?
Comment #38
jhodgdonChanging status until I'm convinced it belongs where it is in the patch. :)
Comment #39
star-szrAfter looking at this again I'm with you, especially once you brought up the .install file. I'll roll a new patch in the next day or two.
Comment #40
star-szrMoved the update_api @defgroup to the bottom of system.api.php. No other changes.
Comment #41
jhodgdonGreat work! I think this is ready to go. I'll commit it tomorrow unless someone else gets to it first (and I guess a documentation issue shouldn't be "major"?).
Comment #42
jhodgdonThanks again! Committed to 8.x. Let's make a version of this for 7.x too -- which will replace a different group there.
Comment #43
star-szrI'll roll a patch for D7, likely over the weekend. I'm guessing the main change for the D7 version would be to update the schema numbers in the examples from 8xxx to 7xxx and 7xxx to 6xxx.
Comment #44
jhodgdonThat would probably be good. Also, the current group ID that the API functions are in for D7 is different, and presumably the actual update functions that are in the group are a different set too.
Comment #45
star-szrHere's a patch for D7. Added the missing @ingroup to
_update_7000_field_read_fields()
, everything else was straightforward.One convention I wasn't sure about - should there be a blank line before @ingroup, i.e. after comments or a @return? I added a blank line before the @ingroup I added.
Comment #46
jhodgdonYes, there should usually be a blank line between @return and @ingroup. The @ingroup can be part of a group of @see though without a blank line.
Anyway, this patch looks good, thanks! I'll get it committed shortly.
Comment #47
jhodgdonThanks all! This is now committed to 7.x.
Comment #48.0
(not verified) CreditAttribution: commentedUpdated issue summary.