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

Files: 
CommentFileSizeAuthor
#39 merge-database-storage-controllers-2095399-39.patch136.36 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,947 pass(es).
[ View ]
#39 merge-database-storage-controllers-2095399-39-interdiff.txt4.93 KBBerdir
#37 merge-database-storage-controllers-2095399-37.patch135.62 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,806 pass(es).
[ View ]
#35 merge-database-storage-controllers-2095399-35.patch134.89 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,884 pass(es), 3 fail(s), and 2 exception(s).
[ View ]
#35 merge-database-storage-controllers-2095399-35-interdiff.txt1.69 KBBerdir
#31 merge-database-storage-controllers-2095399-31.patch134.76 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,792 pass(es), 23 fail(s), and 10 exception(s).
[ View ]
#29 merge-database-storage-controllers-2095399-29.patch134.84 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,202 pass(es), 320 fail(s), and 117 exception(s).
[ View ]
#28 merge-database-storage-controllers-2095399-28.patch140.01 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,916 pass(es).
[ View ]
#28 merge-database-storage-controllers-2095399-28-interdiff.txt1.91 KBBerdir
#25 merge-database-storage-controllers-2095399-25.patch142.99 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 merge-database-storage-controllers-2095399-23.patch142.98 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 merge-database-storage-controllers-2095399-19.patch142.98 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#19 merge-database-storage-controllers-2095399-19-interdiff.txt1.91 KBBerdir
#17 merge-database-storage-controllers-2095399-17.patch141.38 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,552 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#17 merge-database-storage-controllers-2095399-17-interdiff.txt2.39 KBBerdir
#15 merge-database-storage-controllers-2095399-15-1.patch140.68 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,399 pass(es), 111 fail(s), and 195 exception(s).
[ View ]
#15 merge-database-storage-controllers-2095399-15-2.patch140.4 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,910 pass(es), 248 fail(s), and 987 exception(s).
[ View ]
#12 merge-database-storage-controllers-2095399-12.patch139.98 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 merge-database-storage-controllers-2095399-9.patch137.73 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 merge-database-storage-controllers-2095399-4.patch137.7 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 merge-database-storage-controllers-2095399-4-interdiff.txt10.96 KBBerdir
#1 merge-database-storage-controllers-2095399-1.patch127.09 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Active» Needs review
Issue tags:+Entity Field API
StatusFileSize
new127.09 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Something like this.

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.

Status:Needs work» Needs review
StatusFileSize
new10.96 KB
new137.7 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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 ?

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

Component:documentation» other

Not a documentation issue.

Component:other» entity system
StatusFileSize
new137.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-9.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Yeah, sorry, I always mess up the component.

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

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.

Status:Needs work» Needs review
StatusFileSize
new139.98 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new140.4 KB
FAILED: [[SimpleTest]]: [MySQL] 57,910 pass(es), 248 fail(s), and 987 exception(s).
[ View ]
new140.68 KB
FAILED: [[SimpleTest]]: [MySQL] 58,399 pass(es), 111 fail(s), and 195 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.39 KB
new141.38 KB
FAILED: [[SimpleTest]]: [MySQL] 58,552 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
new142.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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}

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

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

Status:Needs work» Needs review
StatusFileSize
new142.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new142.99 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch merge-database-storage-controllers-2095399-25.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another.

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

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

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
new140.01 KB
PASSED: [[SimpleTest]]: [MySQL] 58,916 pass(es).
[ View ]

Re-roll.

Issue tags:+sprint
StatusFileSize
new134.84 KB
FAILED: [[SimpleTest]]: [MySQL] 57,202 pass(es), 320 fail(s), and 117 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new134.76 KB
FAILED: [[SimpleTest]]: [MySQL] 58,792 pass(es), 23 fail(s), and 10 exception(s).
[ View ]

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

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

Yes! ;)

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
new134.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,884 pass(es), 3 fail(s), and 2 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new135.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,806 pass(es).
[ View ]

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.

Issue tags:-sprint

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

StatusFileSize
new4.93 KB
new136.36 KB
PASSED: [[SimpleTest]]: [MySQL] 58,947 pass(es).
[ View ]

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.

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: Rename 'fieldable' key in entity definitions. 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.

Looks good to me. Definitely a step forward.

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

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.