Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
- _update_7000_field_create_field to _update_8000_field_create_field
- _update_7000_field_create_instance to _update_8000_field_create_instance
- Removes _update_7000_field_read_fields(), _update_7000_field_delete_field() and _update_7000_field_delete_instance() as there are no use case in core right now that use it. Modules in contrib that want to perform such actions don't exist as far as we know, plus you should really be careful anyway in case some module does that.
- Does some fiddling with hook_update_dependencies to move field conversion to CMI as early as possible, after {file.usage}.id conversion, but before any other fields in core are created, e.g. user picture block custom.
Original report
We need equivalents for the existing _update_7000_field_*() helper functions, that run on top of CMI storage.
Then, #1953418: Run "Field API : CMI" upgrade as early as possible. can remove the old functions, and push existing upgrades after the CMI conversion.
Comment | File | Size | Author |
---|---|---|---|
#59 | 1969662_59.patch | 15.46 KB | swentel |
#57 | 1969662_57.patch | 15.45 KB | amateescu |
#57 | interdiff.txt | 3.15 KB | amateescu |
#49 | 1969662_49.patch | 17.9 KB | amateescu |
#49 | interdiff.txt | 6.49 KB | amateescu |
Comments
Comment #1
yched CreditAttribution: yched commentedtitle cleanup
Comment #2
swentel CreditAttribution: swentel commentedTagging (for the rocketship site)
Comment #3
andypostAlso this functions needs comment to figure out which kind of functions to use depending on hook_update_dependencies()
Comment #4
yched CreditAttribution: yched commentedIn #625958-172: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute, posted a hotfix for the newly added file_update_8002() that was using field_CRUD_*() functions.
Comment #5
swentel CreditAttribution: swentel commentedTaking this up (unless someone else is busy ?) - the sooner we tackle this, the better.
Comment #6
swentel CreditAttribution: swentel commentedLet's see, this seemed to be fine locally ...
This only converts _update_7000_field_create_field() and _update_7000_field_create_instance() so far. I'm wondering if we really need to keep the rest ?
I had to play a little with the update dependencies (some numbers were just wrong). The most important one is now that the file column is updated ASAP, the rest really doesn't matter.
Comment #8
swentel CreditAttribution: swentel commentedHmm, most of those tests pass locally, asking for a re-test.
Comment #9
swentel CreditAttribution: swentel commented#6: 1969662-field-update-crud-cmi-6.patch queued for re-testing.
Comment #11
swentel CreditAttribution: swentel commentedThis should do it I think.
Also opened #1985488: Consider renaming translation_entity module, because boy, that was funny ..
Comment #13
swentel CreditAttribution: swentel commentedjthorsen did a re-test and which now is green.
The error was in FieldInstance (Unsupported Operand Type in FieldInstance.php on line 368), smells like a random failure now, maybe due to the patch.
Haven't seen any locally though, even running 10 times. Going to trigger another re-test, just to see what happens.
Comment #14
swentel CreditAttribution: swentel commented#11: 1969662-field-update-crud-cmi-11.patch queued for re-testing.
Comment #16
swentel CreditAttribution: swentel commentedSo, looks like we're really hitting a random failure, see #13. Leaving to needs review to get potentially more eyes on it.
Comment #17
swentel CreditAttribution: swentel commentedThis is the order of the upgrades now. Field CMI runs after file update, after user_update_8000 and before user_update_8011 and other update functions that create fields and instances (like block).
Should be green and stable. Converted the other upgrade functions as well.
Comment #18
swentel CreditAttribution: swentel commentedSome doc updates.
Comment #19
yched CreditAttribution: yched commentedCool !
Couple remarks:
- the new _udpate_7000_field*() should be renamed 8000 ;-)
-
That function and the others Will need a type hints for their params I guess...
Also, I'd suggest naming the param $field_config rather than $field, like we do in other places, since this is not a Field object ?
IMO we could stop accepting $field['field_name'] here, and accept only $field['id'] (and thus not fill 'id' in the defaults).
If $p1 and $p2 are irrelevant, then just list(,,$entity_type, $bundle...) ?
if ($field_name == $field_name) { : Heh, this looks wrong :-).
Comment doesn't seem relevant ?
Same here, only accept $instance['field_id'] ?
Comment #21
swentel CreditAttribution: swentel commentedHah, the 7000 is indeed stupid. Addressed other things too and tests pass locally, so here goes :)
One weird thing is that the block upgrade still called _update_7000_field_sql_storage_write(), but that didn't even exist anymore ..
The failure from #18 is random imo and has nothing todo with this patch.
Comment #23
swentel CreditAttribution: swentel commentedforgot one rename
Comment #24
yched CreditAttribution: yched commentedThanks !
A problem with _update_8000_field_read_fields() is it also returns deleted fields from state.
Then the temptation is great for consuming code to just iterate on the results, and save them back to config, not paying attention whether they are deleted or not --> deleted fields ending up in CMI + also in state, ooops.
Maybe we should just drop _update_8000_field_read_fields() ? There is currently no similar function for instances after all...
Updates functions willing to read / update fields or instances can hit raw config, that's not too difficult. Will leave out deleted fields if they don't take the extra step to read them from state, but I'd say that's what most cases actually intend to anyway.
We just provide helpers for create and delete for both fields and instances, that are a bit more involved, and that's it ?
Comment #25
swentel CreditAttribution: swentel commentedAgreed, also removes _update_8000_field_delete_field() and _update_8000_field_delete_instance() after talk with yched in IRC.
Comment #26
yched CreditAttribution: yched commentedLooks good. Thanks !
Comment #28
swentel CreditAttribution: swentel commentedToo early, sorry, redundant function in update.inc :)
Comment #29
yched CreditAttribution: yched commentedRight.
Comment #29.0
swentel CreditAttribution: swentel commentedUpdated issue summary
Comment #31
swentel CreditAttribution: swentel commented#28: 1969662-field-update-crud-cmi-28.patch queued for re-testing.
Comment #32
swentel CreditAttribution: swentel commentedC'mon testbot!
Comment #33
yched CreditAttribution: yched commentedLeft a note on #1965208-21: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem - patch over there contains:
Comment #34
catchThe idea with the _update_7000_*() functions is so that updates can rely on them and not have to be changed when the API does.
So the existing updates should continue using those, and the functions themselves should be left as is, then an _update_800*_() can be added each time the API changes. This way it's not necessary to retrospectively go back into the upgrade path and change all the dependencies each time.
Comment #35
swentel CreditAttribution: swentel commentedThis patch also makes sure that those functions for field api run on top of CMI and not a mix of both, because that was causing headaches in all sorts of ways (hook_update_dependencies is really not funny, but I guess everyone knows that). See #33, #625958: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute It feels redundant to have two _upgrade_create_field() functions, because contrib is never going to use the old one anyway as we're on top of CMI then.
We had an separate issue first for the early run (see #1953418: Run "Field API : CMI" upgrade as early as possible.), but decided to merge this together.
#17 contains a dump of the order of updates with this patch, where the field api updates now run very early before any, just after file_update_8001.
Unless I'm missing something of course, but I'd rather go for this patch, because it's a weird situation we're in now.
Comment #36
yched CreditAttribution: yched commented@catch:
Yeah, really, please let's do this :-)
- Having updates dealing with fields being a mix of "some are coded to run on the old storage / some are coded to run on the new storage" is a confusing mess - we've had several opportunities to see that when polishing #1735118: Convert Field API to CMI and since then.
It also adds rigidity to the upgrade order, which has many chances of biting us later.
The "field to CMI" update itself has little dependencies, so a situation where this one runs asap, and all the other updates dealing with fields consistently run after that one, is much more preferable, The final polishing phase .
- As @swentel says, the _update_7000_field_*() helper functions would be of no use for contrib, and would thus just muddy the water.
By having every core update run on the CMI field storage and the 8000 helpers, we provide more consistent examples for D8 contrib to code their own upgrades.
Comment #37
catchCan we at least rename the _8000_ updates to reflect the schema version when the fields change to config - that's 8003 not 8000.
Also assigning to chx since he's the upgrade path maintainer (again). What I think this issue is saying is use hook_update_dependencies() until we get to beta/rc and only have more than one update_8* function prior to that for the full upgrade path.
Comment #38
yched CreditAttribution: yched commentedAFAIK, update helper functions are typically named just "7000" / "8000", their name doesn't refer to a specifc 70xx / 80xx update #.
Comment #39
chx CreditAttribution: chx commentedI rerolled this and it's good to go IMO. We call these function update*8000 because they are used during the 7->8 update, used by many updates and not just a specific one. It is possible although never was necessary that we need to add a 8001 but even that's unlikely.
Comment #41
swentel CreditAttribution: swentel commented#39: 1969662_38.patch queued for re-testing.
Comment #42
swentel CreditAttribution: swentel commentedRTBC as per #39
Comment #44
swentel CreditAttribution: swentel commented#39: 1969662_38.patch queued for re-testing.
Comment #45
swentel CreditAttribution: swentel commentedRTBC as per #39
Comment #46
catch@yched the naming is defined by http://api.drupal.org/api/drupal/modules%21system%21system.api.php/group...
Comment #47
yched CreditAttribution: yched commentedOK, strictly speaking, according to those standards, _update_8000_field*() should be _update_8003_field*().
I've never actually seen such a case of and update helper being named after a specific N00X update number rather than just N000, Since this is so uncommon, I'm slightly worried that it might make the functions appear like helpers for field_update_8003 (which they are not).
OTOH, maybe having 8003 in the name makes it clearer for updates that will try to use them that they need to make sure they run after field_update_8003().
If we had a possibility of forcing field_update_8003() to run before anything else, then naming the helpers 8000 would be fine, but since this currently requires each update dealing with fields to explicitly add a dependency on field_update_8003, a reminder is probably useful.
And since we have explicit standards on this case, well, sure, let's apply them...
Can't reroll myself for now, though.
Comment #48
yched CreditAttribution: yched commentedAlso:
- It might be useful to add something like "Upgrade using this function need to use hook_update_dependencies() to ensure they get executed after field_update_8003()." in the phpdocs for the helpers ?
- In Portland, @chx pointed that field updates should not specify a dependency on file updates, since file.module is optional. It should be file_update_dependencies() that references field_update_8003, rather than the other way around.
Comment #49
amateescu CreditAttribution: amateescu commentedUpdated the function names, reversed the field -> file update dependency and fixed
file_update_8002()
to work with config.Comment #50
yched CreditAttribution: yched commented@amateescu: thanks!
file_update_8002() is being removed in #625958: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute, and so should probably be left untouched here.
The important thing that file_update_dependencies() should say is :
Comment #51
chx CreditAttribution: chx commentedNo, no, no, no! That's wrong, let's put the function names back. We will add 8001 if and when there is a necessity for new ones. If the docs says otherwise then they are worded poorly. It's not like the 8000 schema changes (or even could change) before a 8.x release. #48 is correct.
Comment #52
yched CreditAttribution: yched commented@chx: I'm fine either way, but @catch insists on _update_8003_field*().
https://api.drupal.org/api/drupal/modules%21system%21system.api.php/grou... states:
So, strictly speaking, even though I initially wanted to go with 8000, I have to concede @catch seems right...
The new helper functions are only valid after field_update_8003() has played. I very much wish there was a way we could *enforce* the code currently in field_8003 to run before most other upgrades, in which case we could make a case that "the helpers are valid for the whole 8000 range". But as it currently stands it's just a regular update, other updates dealing with field/instance definitions need to state an explicit dependency on it, and the helpers are only valid after field_8003 has run.
So - that back and forth on naming doesn't rejoice me either :-). How do we move forward here ?
Comment #53
catchThe docs are right, I was involved with the original issue that added these (not a fun one), then the issue that tried to add the docs (also not fun).
It's incredibly confusing but it just reflects the reality of what the major upgrade path is like when some updates change schema and others change data based on either the old or new schema (not fun). If someone has a better idea on either how to cope with this issue or the naming that's great but it should be a follow-up rather than revisiting that entire discussion here.
Comment #54
amateescu CreditAttribution: amateescu commentedFair enough, that's critical and RTBC, while this one is just 'normal' so let's leave it alone for the sake of saving a re-roll.
Nope, that's:
But since we're removing
file_update_8002()
, why do we still need this dependency?Comment #55
yched CreditAttribution: yched commentedfile_8001 needs to run before field_8003, and this has to be specified in file.install rather than field.install.
So,
$dependencies['field'][8003] = array(
'file' => 8001,
);
in file_update_dependencies() seems like the way to go ?
+ the fate of file_8002 is not related ?
Comment #56
amateescu CreditAttribution: amateescu commentedEdit: removed stupid comment :)
About "+ the fate of file_8002 is not related ?". It is very much related because if we remove it in #625958: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute there is no other update function in file.install that deals with fields. file_update_8001() is just updating a db column in {file_usage}. So there's nothing else that needs to depend on field_update_8003().
Comment #57
amateescu CreditAttribution: amateescu commented@alexpott told me why field_update_8003() must run after file_update_8001(), sorry for the noise..
Comment #59
swentel CreditAttribution: swentel commentedThere we go
Comment #60
yched CreditAttribution: yched commentedThanks guys !
Looks good if green.
Comment #61
chx CreditAttribution: chx commentedWhatever. I was involved with the originals too and this is not how I remember; and I still think it's a bad idea -- but, whatever.
Comment #62
catchIf you have an update that runs before field_update_8003(), which is quite possible to do with hook_update_dependencies(), it's got nowhere to go now. Calling the update _8000_ would misleadingly imply that it can run any time after field_update_8000() which is not the case.
This is why I originally suggested leaving the _update_7000_ in there and adding the new one, but since we don't support head2head updates yet retrospectively rewriting old update handlers is less risky than otherwise. Calling things 'a bad idea' and 'not how I remember' isn't an argument so without any genuine discussion I don't know how this is supposed to be clarified, I've explained more than once my understanding of this and no-one's actually argued against it.
Either way, committed/pushed to 8.x.
Comment #63.0
(not verified) CreditAttribution: commentedUpdated issue summary.