Follow-up of #2004244-105: Move entity revision, content translation, validation and field methods to ContentEntityInterface.
Critical because I think we want to get rid of the NG notion and this is the last one of those after the other issue.
My plan from there:
That's my idea/plan on that topic, which would also work quite well with the current class hierarchy, which consists of just two points:
- Merge DatabaseStorageController and DatabaseStorageControllerNG into FieldableDatabaseStorageController
- Introduce a new SimpleDatabaseStorageController or something like that basically only supports to save plain entity objects, no fields, (we could support revisions, but maybe even leave that out?)
Comments
Comment #1
BerdirSomething like this.
Comment #2
BerdirExplanation for the patch, which might look a bit interesting:
DatabaseStorageController is basically changed twice:
- First is the simplification into the non-fieldable DatabaseStorageController, with all field/revision stuff removed.
- Then there's the removed NG controller
- Then you can see the all the additions from the NG storage controller controller, merging the result into FieldableDatabaseStorageController.
Comment #4
BerdirOh, menu link save override had still calls for the field stuff.
Move those two field methods down to fieldable entity storage controller base/interface, why are they even on the interface?
Comment #6
yched CreditAttribution: yched commentedI separately opened #2095255: Remove BC code from field iterators for the simplifications on invokeFieldMethod() / invokeFieldItemPrepareCache().
Do you want to mark that other one as duplicate, or do we get those simplifications in separately ?
Comment #7
BerdirI already have slightly different changes to those methods in 3 different issues. We'll see what gets in first :)
Comment #8
jhodgdonNot a documentation issue.
Comment #9
BerdirYeah, sorry, I always mess up the component.
Re-roll, not sure why it doesn't install.
Comment #10
BerdirTrying again. Can't reproduce on PHP 5.4 or 5.3
Comment #12
BerdirAs expected, this is an interesting issue to re-roll.
Comment #14
jthorson CreditAttribution: jthorson commentedFrom the testbot logs on install:
Comment #15
BerdirUh, trying some alternatives to see if they fix the problem.
Comment #17
BerdirOk, so removing the implements from the interface fixes the problem. This should also fix the field attach and the revision related tests.
Comment #19
BerdirOk, found the problem, we didn't delete stuff from the data table.
And that test was the only one that failed and only due to a big (not re-indexing the search data when a node gets deleted), so I added better tests for that :)
Comment #20
jibranMinor doc issues.
This should be bool.
This should be \Drupal\Core\Entity\EntityInterfac[] after #1772420: [policy only] Documentation standard for arrays of a certain type (param/return types)
{@inheritdoc}
Comment #21
Berdir#19: merge-database-storage-controllers-2095399-19.patch queued for re-testing.
Comment #23
BerdirRe-roll.
Comment #25
BerdirAnother.
Comment #26
Berdir#25: merge-database-storage-controllers-2095399-25.patch queued for re-testing.
Comment #28
BerdirRe-roll.
Comment #29
BerdirFun re-roll. But I think I all issues that I was waiting for are now in, so adding to sprint.
If this is still green, then I need reviews :)
I'd suggest to not make too many changes to the FieldableDatabaseStorageController here as it's probably much easier to review if we have a specific issue to optimize the code flow there.
Comment #31
BerdirAnother re-roll after that issue went in for the second time. Something with revisions is broken, will look into that later.
Comment #32
plachYou mean revisions are broken with the current patch, I hope :)
Comment #33
BerdirYes! ;)
Comment #35
BerdirYay for fast entity tests. Lost the code that sets $this->revisionDataTable.
Comment #37
BerdirFound it. Missed one change from the sensible storage issue, buildQuery() has a change that no longer made sense and the result was that the data from the node_revision table wasn't loaded. The weirdest thing is that there weren't more fails due to that.
No useful interdiff as I also moved a few things around to debug this and trying to result in a better diff/grouping of methods.
Comment #38
msonnabaum CreditAttribution: msonnabaum commentedWith the exception of invokeFieldItemPrepareCache and invokeFieldMethod which look like they shouldnt be in this patch, this looks great.
Comment #39
BerdirRemoved invokeFieldItemPrepareCache(), that was originally moved and then the original got removed in the meantime.
invokeFieldMethod() is still necessary but it should be a protected helper, does not need to be on the interface, that makes no sense. So removed it from there and made it protected. Some for the translation hook helper, moved that down too, was already protected. Also changed the type hint on both to ContentEntityInterface, because they rely on those methods.
Comment #40
amateescu CreditAttribution: amateescu commentedKeeping a stripped-down version of the database storage controller that is not fieldable will impact a bit this issue we discussed in Prague: #2100343: Remove 'fieldable' key in entity definitions in favour of 'field_ui_base_route'. Basically, you can set the (new) ‘extensible_storage’ flag as much as you want, but without the storage controller to back that up, it won't be extensible. Maybe that flag should be added automatically when parsing the entity type annotations.
Anyway, this is still very good progress and I also think it's ready to go.
Comment #41
msonnabaum CreditAttribution: msonnabaum commentedLooks good to me. Definitely a step forward.
Comment #42
Berdir@amateescu: Yes, I think that's by design, only fieldable entity storage controllers can be extensible. config entities aren't either. There will also be other types of entities, e.g. (read-only) external entities.
That said, right now we need the simple version of the database storage controller for menu links. When they're converted, then we could still remove it if we want to.
Comment #43
catchLooks great. Nice to see the NG classes gone. Committed/pushed to 8.x, thanks!