• _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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Title: Provide new _update_8000* CRUD helpers for Field and Instance definitions » Provide new _update_8000_*() CRUD helpers for Field and Instance definitions

title cleanup

swentel’s picture

Issue tags: +Field API

Tagging (for the rocketship site)

andypost’s picture

Also this functions needs comment to figure out which kind of functions to use depending on hook_update_dependencies()

yched’s picture

In #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.

swentel’s picture

Assigned: Unassigned » swentel

Taking this up (unless someone else is busy ?) - the sooner we tackle this, the better.

swentel’s picture

Status: Active » Needs review
FileSize
8.61 KB

Let'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.

Status: Needs review » Needs work

The last submitted patch, 1969662-field-update-crud-cmi-6.patch, failed testing.

swentel’s picture

Hmm, most of those tests pass locally, asking for a re-test.

swentel’s picture

Status: Needs work » Needs review
Issue tags: -Field API

Status: Needs review » Needs work
Issue tags: +Field API

The last submitted patch, 1969662-field-update-crud-cmi-6.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
8.17 KB

This should do it I think.

Also opened #1985488: Consider renaming translation_entity module, because boy, that was funny ..

Status: Needs review » Needs work

The last submitted patch, 1969662-field-update-crud-cmi-11.patch, failed testing.

swentel’s picture

jthorsen 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.

swentel’s picture

Status: Needs work » Needs review
Issue tags: -Field API

Status: Needs review » Needs work
Issue tags: +Field API

The last submitted patch, 1969662-field-update-crud-cmi-11.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

So, looks like we're really hitting a random failure, see #13. Leaving to needs review to get potentially more eyes on it.

swentel’s picture

...
system_update_8013
user_update_8000
system_update_8014
system_update_8015
...
system_update_8025
file_update_8001
system_update_8027
system_update_8028
file_update_8002
field_update_8001
system_update_8029
field_update_8002
system_update_8030
field_update_8003
system_update_8031
user_update_8011
...

This 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.

swentel’s picture

Some doc updates.

yched’s picture

Cool !

Couple remarks:
- the new _udpate_7000_field*() should be renamed 8000 ;-)
-

+++ b/core/modules/field/field.installundefined
@@ -9,77 +9,46 @@
 function _update_7000_field_create_field(&$field) {

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 ?

+++ b/core/modules/field/field.installundefined
@@ -9,77 +9,46 @@
+    'id' => $field['field_name'],

IMO we could stop accepting $field['field_name'] here, and accept only $field['id'] (and thus not fill 'id' in the defaults).

+++ b/core/modules/field/field.installundefined
@@ -102,18 +71,20 @@ function _update_7000_field_delete_field($field_name) {
+    list($p1, $p2, $entity_type, $bundle, $field_name) = explode('.', $id);
+    if ($field_name == $field_name) {

If $p1 and $p2 are irrelevant, then just list(,,$entity_type, $bundle...) ?
if ($field_name == $field_name) { : Heh, this looks wrong :-).

+++ b/core/modules/field/field.installundefined
@@ -162,88 +129,85 @@ function _update_7000_field_delete_instance($field_name, $entity_type, $bundle)
+  // Ditch UUID keys, we will iterate through deleted fields using a numeric
+  // index.

Comment doesn't seem relevant ?

+++ b/core/modules/field/field.installundefined
@@ -162,88 +129,85 @@ function _update_7000_field_delete_instance($field_name, $entity_type, $bundle)
+    'id' => $instance['entity_type'] . '.' . $instance['bundle'] . '.' . $instance['field_name'],

Same here, only accept $instance['field_id'] ?

Status: Needs review » Needs work

The last submitted patch, 1969662-field-update-crud-cmi-18.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
9.47 KB
17.16 KB

Hah, 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.

Status: Needs review » Needs work

The last submitted patch, 1969662-field-update-crud-cmi-21.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
9.69 KB
17.27 KB

forgot one rename

yched’s picture

Thanks !

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 ?

swentel’s picture

Agreed, also removes _update_8000_field_delete_field() and _update_8000_field_delete_instance() after talk with yched in IRC.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Thanks !

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1969662-field-update-crud-cmi-25.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
5.22 KB
14.73 KB

Too early, sorry, redundant function in update.inc :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Right.

swentel’s picture

Issue summary: View changes

Updated issue summary

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API

The last submitted patch, 1969662-field-update-crud-cmi-28.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Field API
swentel’s picture

Status: Needs review » Reviewed & tested by the community

C'mon testbot!

yched’s picture

Left a note on #1965208-21: Convert TaxonomyTermReferenceItem to extend EntityReferenceItem - patch over there contains:

+++ b/core/modules/taxonomy/taxonomy.installundefined
@@ -335,3 +347,38 @@ function taxonomy_update_8006() {
+  // @todo Switch to the update-safe version of this function when we have one.
+  if (!$fields = field_read_fields(array('type' => 'taxonomy_term_reference'), array('include_inactive' => 1))) {
+    return;
catch’s picture

Status: Reviewed & tested by the community » Needs work

The 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.

swentel’s picture

This 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.

yched’s picture

Status: Needs work » Reviewed & tested by the community

@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.

catch’s picture

Assigned: swentel » chx
Status: Reviewed & tested by the community » Needs work

Can 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.

yched’s picture

Can we at least rename the _8000_ updates to reflect the schema version when the fields change to config - that's 8003 not 8000

AFAIK, update helper functions are typically named just "7000" / "8000", their name doesn't refer to a specifc 70xx / 80xx update #.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.84 KB

I 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.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API

The last submitted patch, 1969662_38.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Field API

#39: 1969662_38.patch queued for re-testing.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as per #39

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API

The last submitted patch, 1969662_38.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Field API

#39: 1969662_38.patch queued for re-testing.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as per #39

catch’s picture

Status: Reviewed & tested by the community » Needs work
yched’s picture

OK, 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.

yched’s picture

Also:
- 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.

amateescu’s picture

Status: Needs work » Needs review
FileSize
6.49 KB
17.9 KB

Updated the function names, reversed the field -> file update dependency and fixed file_update_8002() to work with config.

yched’s picture

@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 :

  $dependencies['field'][8003] = array(
    'file' => 8001,
  );
chx’s picture

Assigned: chx » Unassigned

No, 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.

yched’s picture

@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:

The utility function should be named _update_N_mymodule_my_function(). N is the schema version the function acts on (the schema version is the number N from the hook_update_N() implementation where this schema was introduced, or a number following the same numbering scheme)

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 ?

catch’s picture

The 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.

amateescu’s picture

file_update_8002() is being removed in #625958: [Follow up] Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute, and so should probably be left untouched here.

Fair 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.

The important thing that file_update_dependencies() should say is :

  $dependencies['field'][8003] = array(
    'file' => 8001,
  );

Nope, that's:

  $dependencies['file'][8001] = array(
    'field' => 8003,
  );

But since we're removing file_update_8002(), why do we still need this dependency?

yched’s picture

file_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 ?

amateescu’s picture

Edit: 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().

amateescu’s picture

FileSize
3.15 KB
15.45 KB

@alexpott told me why field_update_8003() must run after file_update_8001(), sorry for the noise..

Status: Needs review » Needs work

The last submitted patch, 1969662_57.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
15.46 KB

There we go

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks guys !
Looks good if green.

chx’s picture

Whatever. 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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

If 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.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.