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.
Those have just been added by #1893772: Move entity-type specific storage logic into entity classes.
Entity::save() & Entity::delete() should be left as the parent implementations that just delegate to the storage controller. The storage controller then calls [pre|post]Save(), [pre|post]Delete(), etc...
Remaining tasks:
- get green tests
- moving our code to pre/postSave() means we can stop populating $this->original, ConfigStorageController::save() does it for us
Patch should be ready.
Comment | File | Size | Author |
---|---|---|---|
#65 | field-save-delete-2020895-64.patch | 24.12 KB | yched |
#63 | field-save-delete-2020895-63.patch | 24.12 KB | yched |
#56 | field-save-delete-2020895-56.patch | 24.12 KB | yched |
#56 | interdiff.txt | 3.11 KB | yched |
#55 | field-save-delete-2020895-55.patch | 24.03 KB | swentel |
Comments
Comment #1
yched CreditAttribution: yched commentedTemptative patch.
This leaves out FieldInstance::delete() for now, because of the custom $field_cleanup param, this might be a bit tricky :-(.
Comment #2
amateescu CreditAttribution: amateescu commentedI had the same tricky situation in the patch that converted menu links to entities :) I chose to solve it by using a flag on controller that needs to be set prior to calling the delete method. See http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul... and http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul....
Comment #4
yched CreditAttribution: yched commentedReroll after #2018731: Move field_has_data() to Field::hasData().
Fails were because
$storage_controller->loadUnchanged($this->getOriginalID());
in FieldInstance::preSave() return FALSE when we are called from field_entity_bundle_rename().I added a test / workaround for now, but field_entity_bundle_rename() should update the config records directly IMO, rather than re-saving the ConfigEntity. I know we went back and forth on this in the "Field / CMI" issue, but I can't remember why we stayed that way.
Comment #5
yched CreditAttribution: yched commentedre @amateescu #2 / menu links - setPreventReparenting() :
Hm, smart ;-) The tricky bit might be making sure that the flag that gets set on the controller for a specific delete() operation doesn't interfere for the rest of the request. Or I guess each delete operation explicitly sets the state of the flag it needs, so the flag always has the "right" value when performing a delete ?
I probably won't have the time to look into this before I go, though :-/
Comment #6
amateescu CreditAttribution: amateescu commentedYep, in that case, it was always set by the function that called the storage controller. But the proper scenario would be to unset it at the end of the delete() method.
Comment #8
pcambraHere's a reroll of this, I've updated it to the new changes on the field but got several test failures as it seems that in some circumstances, the instance is not saved to config, had to "force it" in a test to demonstrate the problem (see the interdiff).
Didn't push it to the sandbox because I get this git message on status & I don't know if I should push or not :)
Your branch is ahead of 'origin/field-save-delete-2020895' by 510 commits.
Comment #9
yched CreditAttribution: yched commentedThanks ! Will look into this.
The 510 commits are probably the ones you merged from HEAD, it should be fine to push.
Comment #11
yched CreditAttribution: yched commentedThe instance 'id' property was not saved anymore. Caused all sorts of weirdness when loading more than two of them.
My bad :-(.
Reverted the changes in CrudTest.php
Comment #12
yched CreditAttribution: yched commentedReroll effect - the patch reintroduced the CRUD hooks that got removed in #2013939: Remove hook_field_[CRUD]_() in favor of hook_ENTITY_TYPE_[CRUD]() calls.
Comment #13
pcambraOpened #2054699: Ensure config entity id is set for computed composite ids regarding the id save
Comment #14
amateescu CreditAttribution: amateescu commentedThis cleanup looks great to me. I could only find this small nitpick :)
Missing description for the @param argument and needs an empty line before @throws. Also happens for the next method.
Comment #15
pcambraFieldInstance::preDelete() is still TODO
Comment #16
yched CreditAttribution: yched commentedWe might be able to remove the need for the $field_cleanup bool, actually.
It's just there to avoid an infinite loop on:
"delete last instance -> delete field -> delete all instances"
or
"delete field -> delete all instances -> delete field"
But If we move "delete all instances" to Field::postDelete(),
and "if (last instance) { delete field }" to FieldInstance::postDelete(),
then by the time those run, the actual records that might risk "re-delete ad nauseum" have been deleted already, load() will return nothing, and there is nothing to delete, no loop :-)
Comment #17
yched CreditAttribution: yched commentedOops, also, I didn't re-remove "Invoke hook_field_delete_field()" from Field::postDelete() - see #12
Comment #18
pcambraRerolling after #2054699: Ensure config entity id is set for computed composite ids and #2056827: Remove Unused local variable $module_handler from /core/modules/field/lib/Drupal/field/Plugin/Core/Entity/FieldInstance.php, including the comment in #14. Not taken care of #16 & #17 yet, just checking if nothing was broken in the rebase.
Comment #20
pcambraSome load and load multiple fixes on the storage controller
Comment #21
pcambraHere's a initial approach to #16 and #17, I'm getting some warnings on CRUD field tests as it seems to be reloading a instance twice.
Comment #23
yched CreditAttribution: yched commentedThanks @pcambra!
This handles multiple field deletion, so for performance, it would be best to do one single $state->get() before looping, and one single $state->set() after looping ?
+ 80 chars comment wrapping needs to be adjusted after the code is moved around.
Also, this was previously done *after* the "delete all instances" part, and is now done before. Might have an impact on test fails, not sure.
Those two lines should move above the loop.
That's for a second line of optimizations once this is green, but this could probably be optimized by collecting the instance ids for all the fields, and do a single loadMultiples() on all the collected ids.
Also, FieldInstance::delete(), that defined the custom $field_cleanup set to FALSE here, is now gone, so the FALSE is useless now.
Meaning the code could just do:
$instances = $instance_controller->loadMultiple(...);
$instance_controller->delete($instances);
(which is the way entity_delete_multiple does() it)
indentation is broken :-)
Comment #24
yched CreditAttribution: yched commentedAlso note: moving our code to pre/postSave() means we can stop populating $this->original, ConfigStorageController::save() does it for us.
Comment #25
pcambraHere's some progress, rebased against latest HEAD and I've implemented most of #23 & #24 but probably still have some fails.
The only substantial change is that getBundles() wasn't being populated on postDelete of Field, probably because is too late, stored it in the state in preDelete for later use on postDelete.
Comment #27
yched CreditAttribution: yched commented#1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) means we shouldn't need to check for "cannot create a *new* field/instance with the same id", ConfigEntity::save() checks that for us.
But the current code organization in HEAD forces us to have our own check, because it needs to happen before we call the EntityStorageController::on[Field|Instance]Create() - and our current call to parent::save() happens after that.
The new code organization in the patch should hopefully let us remove that.
Added that in a "remaining tasks" section in the OP, along with #24
Comment #28
yched CreditAttribution: yched commentedReroll for now
(I also updated the field-save-delete-2020895 branch in the sandbox)
Comment #30
yched CreditAttribution: yched commented#28: field-save-delete-2020895-28.patch queued for re-testing.
Comment #32
yched CreditAttribution: yched commentedLet's see where this one brings us.
Comment #34
yched CreditAttribution: yched commentedSome dirty parts to polish, but let's see.
Comment #35
yched CreditAttribution: yched commentedReordering a couple things.
Comment #36
yched CreditAttribution: yched commentedPrevious patch seems lost in testbot limbo, I'll cancel it...
Meanwhile, debugged & reordered a bit more.
Comment #37
yched CreditAttribution: yched commentedCleaned up the newly added tests.
I think this is ready for reviews now.
Comment #38
swentel CreditAttribution: swentel commentedKilled the test, it was running for 4 days now ...
Comment #40
yched CreditAttribution: yched commented#37: field-save-delete-2020895-37.patch queued for re-testing.
Comment #41
yched CreditAttribution: yched commentedReroll
Comment #42
swentel CreditAttribution: swentel commentedType in 'deleteing'.
Should we already use getFieldType() ?
Same maybe for $field->name ?
Comment #43
yched CreditAttribution: yched commentedI'd think not in this issue. The Field / FieldInstance classes currently have plenty of code that access their own properties directly, let's not change this here.
Fixed the typo.
Comment #44
yched CreditAttribution: yched commentedAttempt to do #27
Comment #46
yched CreditAttribution: yched commentedHm, #1969698: ConfigEntity::save() should disallow saving ID/UUID conflicts (Field UUID changes can badly corrupt field data) did not take care of that actually, so we need to keep those for now. Reverting the changes from #44.
We should be good here IMO.
Comment #47
yched CreditAttribution: yched commentedReroll + bump. We need to do this, the sooner the better, and it's a bit painful to merge :-/
Comment #49
swentel CreditAttribution: swentel commentedThis is good to go indeed.
Comment #51
swentel CreditAttribution: swentel commented#49: field-save-delete-2020895-49.patch queued for re-testing.
Comment #53
swentel CreditAttribution: swentel commentedpinged jthorson about this one - installation goes fine locally ..
Comment #54
Berdir"The service definition "plugin.manager.entity.field.field_type" does not exist.".
Without "entity" now.
Comment #55
swentel CreditAttribution: swentel commentedOk, that was weird - must have been on the wrong branch.
Comment #56
yched CreditAttribution: yched commentedDoh, thanks for fixing this :-)
Patch just moves the dependencies on the field type manager to standalone vars, as the current code tends to do in Field / FieldInstance pending real injection abilities in entity classes.
Comment #58
swentel CreditAttribution: swentel commentedRight, much nicer :)
Comment #58.0
swentel CreditAttribution: swentel commentedremaining tasks
Comment #59
Xano56: field-save-delete-2020895-56.patch queued for re-testing.
Comment #60
yched CreditAttribution: yched commentedremoved the stale "remaining tasks"
Comment #61
Xano56: field-save-delete-2020895-56.patch queued for re-testing.
Comment #62
yched CreditAttribution: yched commentedCan I haz commit ? :-p
Comment #63
yched CreditAttribution: yched commentedReroll (just offsets)
Comment #64
claudiu.cristea#2143019: Allow field type to specify a 'process settings' step before saving & loading definitions waits for this.
Comment #65
yched CreditAttribution: yched commentedRe-reroll.
Pleaaaase ? :-)
Comment #66
catchCommitted/pushed to 8.x, thanks!