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

CommentFileSizeAuthor
#39 merge-database-storage-controllers-2095399-39.patch136.36 KBBerdir
#39 merge-database-storage-controllers-2095399-39-interdiff.txt4.93 KBBerdir
#37 merge-database-storage-controllers-2095399-37.patch135.62 KBBerdir
#35 merge-database-storage-controllers-2095399-35.patch134.89 KBBerdir
#35 merge-database-storage-controllers-2095399-35-interdiff.txt1.69 KBBerdir
#31 merge-database-storage-controllers-2095399-31.patch134.76 KBBerdir
#29 merge-database-storage-controllers-2095399-29.patch134.84 KBBerdir
#28 merge-database-storage-controllers-2095399-28.patch140.01 KBBerdir
#28 merge-database-storage-controllers-2095399-28-interdiff.txt1.91 KBBerdir
#25 merge-database-storage-controllers-2095399-25.patch142.99 KBBerdir
#23 merge-database-storage-controllers-2095399-23.patch142.98 KBBerdir
#19 merge-database-storage-controllers-2095399-19.patch142.98 KBBerdir
#19 merge-database-storage-controllers-2095399-19-interdiff.txt1.91 KBBerdir
#17 merge-database-storage-controllers-2095399-17.patch141.38 KBBerdir
#17 merge-database-storage-controllers-2095399-17-interdiff.txt2.39 KBBerdir
#15 merge-database-storage-controllers-2095399-15-1.patch140.68 KBBerdir
#15 merge-database-storage-controllers-2095399-15-2.patch140.4 KBBerdir
#12 merge-database-storage-controllers-2095399-12.patch139.98 KBBerdir
#9 merge-database-storage-controllers-2095399-9.patch137.73 KBBerdir
#4 merge-database-storage-controllers-2095399-4.patch137.7 KBBerdir
#4 merge-database-storage-controllers-2095399-4-interdiff.txt10.96 KBBerdir
#1 merge-database-storage-controllers-2095399-1.patch127.09 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Active » Needs review
Issue tags: +Entity Field API
FileSize
127.09 KB

Something like this.

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-1.patch, failed testing.

Berdir’s picture

Oh, 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?

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-4.patch, failed testing.

yched’s picture

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

Berdir’s picture

I already have slightly different changes to those methods in 3 different issues. We'll see what gets in first :)

jhodgdon’s picture

Component: documentation » other

Not a documentation issue.

Berdir’s picture

Component: other » entity system
FileSize
137.73 KB

Yeah, sorry, I always mess up the component.

Re-roll, not sure why it doesn't install.

Berdir’s picture

Status: Needs work » Needs review

Trying again. Can't reproduce on PHP 5.4 or 5.3

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-9.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
139.98 KB

As expected, this is an interesting issue to re-roll.

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-12.patch, failed testing.

jthorson’s picture

From the testbot logs on install:

PHP Fatal error:  Cannot inherit previously-inherited or override constant FIELD_LOAD_CURRENT from interface Drupal\\menu_link\\MenuLinkStorageControllerInterface in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php on line 25
Berdir’s picture

Uh, trying some alternatives to see if they fix the problem.

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-15-2.patch, failed testing.

Berdir’s picture

Ok, so removing the implements from the interface fixes the problem. This should also fix the field attach and the revision related tests.

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-17.patch, failed testing.

Berdir’s picture

Ok, 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 :)

jibran’s picture

Minor doc issues.

  1. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -1199,4 +1274,201 @@ static public function _fieldColumnName(FieldInterface $field, $column) {
    +   * @param boolean $load_revision
    

    This should be bool.

  2. +++ b/core/lib/Drupal/Core/Entity/FieldableDatabaseStorageController.php
    @@ -1199,4 +1274,201 @@ static public function _fieldColumnName(FieldInterface $field, $column) {
    +   * @param array &$entities
    

    This should be \Drupal\Core\Entity\EntityInterfac[] after #1772420: [policy only] Documentation standard for arrays of a certain type (param/return types)

  3. +++ b/core/modules/node/lib/Drupal/node/NodeStorageController.php
    @@ -30,7 +30,7 @@ public function create(array $values) {
    +   * Overrides Drupal\Core\Entity\DatabaseStorageController::attachLoad().
    

    {@inheritdoc}

Berdir’s picture

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

The last submitted patch, merge-database-storage-controllers-2095399-19.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
142.98 KB

Re-roll.

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-23.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
142.99 KB

Another.

Berdir’s picture

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

The last submitted patch, merge-database-storage-controllers-2095399-25.patch, failed testing.

Berdir’s picture

Berdir’s picture

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

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-29.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
134.76 KB

Another re-roll after that issue went in for the second time. Something with revisions is broken, will look into that later.

plach’s picture

You mean revisions are broken with the current patch, I hope :)

Berdir’s picture

Yes! ;)

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-31.patch, failed testing.

Berdir’s picture

Yay for fast entity tests. Lost the code that sets $this->revisionDataTable.

Status: Needs review » Needs work

The last submitted patch, merge-database-storage-controllers-2095399-35.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
135.62 KB

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

msonnabaum’s picture

Issue tags: -sprint

With the exception of invokeFieldItemPrepareCache and invokeFieldMethod which look like they shouldnt be in this patch, this looks great.

Berdir’s picture

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

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

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

msonnabaum’s picture

Looks good to me. Definitely a step forward.

Berdir’s picture

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

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks great. Nice to see the NG classes gone. Committed/pushed to 8.x, thanks!

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