After #2015687: Convert field type to FieldType plugin for taxonomy module , ConfigEntityReferenceItemBase only contains the supporting code for the "entity autocreate"

Looks like there's no reason why this shouldn't be useful for base fields as well. That's only an opt-in on the autocomplete widgets, and API-wise it can be handy to auto create the author when creating a node.

+ it would simplify our inheritance stack a bit.

Patch also fixes a couple inconsistencies about how the various entity ref field types handle "autocreate" (target_id NULL or 0)
Thus, filing as bug.

CommentFileSizeAuthor
#66 interdiff.txt1.1 KBamateescu
#66 remove_ConfigEntityReferenceItemBase-2156337-66.patch12.72 KBamateescu
#61 remove_ConfigEntityReferenceItemBase-2156337-61.patch12.7 KByched
#59 remove_ConfigEntityReferenceItemBase-2156337-59.patch12.02 KByched
#57 interdiff.txt2.06 KBamateescu
#57 remove_ConfigEntityReferenceItemBase-2156337-57.patch12.02 KBamateescu
#56 interdiff.txt904 bytesyched
#56 remove_ConfigEntityReferenceItemBase-2156337-56.patch12.02 KByched
#54 interdiff.txt2.7 KByched
#54 remove_ConfigEntityReferenceItemBase-2156337-54.patch12.02 KByched
#52 funky_interdiff.txt2.16 KBamateescu
#48 interdiff.txt1.2 KBamateescu
#48 remove_ConfigEntityReferenceItemBase-2156337-48.patch11.95 KBamateescu
#38 interdiff.txt777 bytesyched
#38 remove_ConfigEntityReferenceItemBase-2156337-38.patch10.75 KByched
#34 interdiff-30-34.txt1004 bytessmiletrl
#34 remove_ConfigEntityReferenceItemBase-2156337-34.patch9.86 KBsmiletrl
#30 interdiff.txt586 bytessmiletrl
#30 remove_ConfigEntityReferenceItemBase-2156337-30.patch10.51 KBsmiletrl
#22 interdiff.txt2.25 KByched
#21 remove_ConfigEntityReferenceItemBase-2156337-21.patch8.93 KByched
#19 remove_ConfigEntityReferenceItemBase-2156337-19.patch7.15 KByched
#13 interdiff.txt703 bytesyched
#13 remove_ConfigEntityReferenceItemBase-2156337-13.patch7.15 KByched
#11 interdiff.txt1.16 KByched
#11 remove_ConfigEntityReferenceItemBase-2156337-11.patch7.15 KByched
#9 interdiff.txt883 bytesyched
#9 remove_ConfigEntityReferenceItemBase-2156337-9.patch5.99 KByched
#4 remove_ConfigEntityReferenceItemBase-2156337-2.patch5.98 KByched
#2 interdiff.txt928 bytesyched
#2 remove_ConfigEntityReferenceItemBase-2156337-2.patch5.99 KByched
#1 remove_ConfigEntityReferenceItemBase-2156337-1.patch4.77 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
4.77 KB

Patch, let's see what breaks.

yched’s picture

FileItem & ImageItem would benefit from this too.

amateescu’s picture

If nothing breaks, I'm super excited for this patch :)

(Except for that whitespace you have in the interdiff :P)

yched’s picture

Damn. I love PhpStorm, but I *hate* that it doesn't trim whitespaces on the line your cursor is :-p

The last submitted patch, 1: remove_ConfigEntityReferenceItemBase-2156337-1.patch, failed testing.

The last submitted patch, 2: remove_ConfigEntityReferenceItemBase-2156337-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: remove_ConfigEntityReferenceItemBase-2156337-2.patch, failed testing.

yched’s picture

Weird, I can reproduce some of the fails locally, but not all (might be related to the fact that I'm currently running 5.5 :-/)

For those I do reproduce (e.g. CommentCSSTest), looks like the errors lie in EntityReferenceItem::isEmpty(), that fails to properly recognize empty items in e.g. snode->uid, producing NULL values that end up in sql inserts on columns that are not supposed to be nul like 'last_comment_uid'

I don't really plan on pushing this myself for now ... Anyone interested ? ;-)

yched’s picture

Status: Needs work » Needs review
FileSize
5.99 KB
883 bytes

This being said... let's see what this fixes / breaks :-)

The last submitted patch, 9: remove_ConfigEntityReferenceItemBase-2156337-9.patch, failed testing.

yched’s picture

Better :-)

Last remaining fail is entity_ref's AutocompleteWidget:
- it leaves empty values pass through as target_id = ''
- this value isn't filtered by isEmpty() anymore since it now checks === NULL
- it then reaches validation and fails, '' doesn't have the right type.

Attached patch should fix AutocompleteWidget.

amateescu’s picture

I don't really plan on pushing this myself for now

lol :)

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -149,4 +149,30 @@ public function getMainPropertyName() {
+    if (empty($this->target_id) && ($entity = $this->entity) && $entity->isNew()) {

Shouldn't this do the same $target_id === NULL check like isEmpty()?

yched’s picture

;-)

Yeah, most probably.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Yay, thanks!

The last submitted patch, 13: remove_ConfigEntityReferenceItemBase-2156337-13.patch, failed testing.

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

Oops..

Berdir’s picture

#597236: Add entity caching to core also has a fix for ConfigEntityReferenceItemBase, looks like this is better and the other fix can be removed after this..

yched’s picture

Well, this time, help welcome to fix those new fails ;-)

Looks like there's still some autocreate code that doesn't comply to the "target_id === NULL rather that 0" convention introduced in #2015687: Convert field type to FieldType plugin for taxonomy module ?

yched’s picture

Status: Needs work » Needs review
FileSize
7.15 KB

Reroll

yched’s picture

Title: merge ConfigEntityReferenceItemBase up into EntityReferenceItem ? » merge ConfigEntityReferenceItemBase up into EntityReferenceItem, and fix inconsistencies
Category: Task » Bug report
Issue summary: View changes
yched’s picture

Should fix the fails ?

yched’s picture

FileSize
2.25 KB

forgot the interdiff

The last submitted patch, 19: remove_ConfigEntityReferenceItemBase-2156337-19.patch, failed testing.

yched’s picture

Green :-p

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

More green :p

Nice cleanup.

xjm’s picture

The last submitted patch, 21: remove_ConfigEntityReferenceItemBase-2156337-21.patch, failed testing.

The last submitted patch, 21: remove_ConfigEntityReferenceItemBase-2156337-21.patch, failed testing.

yched’s picture

smiletrl’s picture

Rerolled, also I just found image_entity_presave(EntityInterface $entity, $type) has an extra param $type.

As stated in issue summary, this deleted "ConfigEntityReferenceItemBase" is only for "entity autocreate". While this patch passed, I feel the functions, i.e, presave() and isEmpty(), shouldn't be used for fileitem and imageitem, which two items are not meant for autocreate. It may confuse people a bit -- could it be possible to "autocreate" a file or image with some autowidget ?:)

Also, it's not related much, but can we move image_entity_presave(EntityInterface $entity) to imageitem:presave()?

yched’s picture

Thanks for the reroll @smiletrl.

I feel the functions, i.e, presave() and isEmpty(), shouldn't be used for fileitem and imageitem, which two items are not meant for autocreate

The "autocreate" feature is an API-level feature provided by the field type. Some widgets happen to trigger it, but it's valid feature nonetheless API-wise (save the file entity by just saving a node that uses it in an image field).

So, no, I see no need to *add* code to artificially *remove* the feature for file & image fields. We should not couple the featured offered by the field type to what kind of widgets exist.

can we move image_entity_presave(EntityInterface $entity) to imageitem:presave() ?

image_entity_presave() reacts to "an entity (here a Field or FieldInstance) being saved". So no, it's completely different from imageitem:presave().

amateescu’s picture

Auto-creating an entity should be possible for all entity types and entity reference-like fields, which includes file and image, so I don't see why we shouldn't allow them to do that.

Edit: Yeah, @yched said it better :)

The last submitted patch, 30: remove_ConfigEntityReferenceItemBase-2156337-30.patch, failed testing.

smiletrl’s picture

Yeah, sorry, that function is working on the entity~ Also, it sounds logic to add the support here for contrib code to make use of this auto-create feature.

That getValue function introduces an $value['entity'] = $this->entity. It breaks image field. Image field produces its entity in its own way(I don't know where it is yet^). Removing that works fine for image field, lets see if it breaks other fields.

Berdir’s picture

You can't delete that, that is necessary.

The last submitted patch, 34: remove_ConfigEntityReferenceItemBase-2156337-34.patch, failed testing.

yched’s picture

Yes, #34 is a no-go. We need to find why #30 has issues with image fields, it worked earlier.

yched’s picture

That's because of #2042773: Change #items within a theme_field() render array from an *array* to the same $items *object* used throughout the rest of the formatter pipeline, not sure why it didn't fail earlier.

Meanwhile, this should fix it, and is needed anyway as long as we have Map::getValue($include_computed).

The last submitted patch, 38: remove_ConfigEntityReferenceItemBase-2156337-38.patch, failed testing.

Berdir’s picture

Berdir’s picture

Weird unserialize errors (unserialize(): Error at offset 867267 of 1187046 bytes), re-testing.

The last submitted patch, 38: remove_ConfigEntityReferenceItemBase-2156337-38.patch, failed testing.

The last submitted patch, 38: remove_ConfigEntityReferenceItemBase-2156337-38.patch, failed testing.

Berdir’s picture

Status: Reviewed & tested by the community » Needs work

Apparently not random then.

yched’s picture

Woah, this is *weird*.

- I have no clue how these fails are related. They test stuff like
core/modules/action/tests/action_bulk_test/config/views.view.test_bulk_form.yml

- I can't reproduce the "unserialize(): Error at offset X of Y bytes in DatabaseStorageExpirable.php line 70" notices on Drupal\node\Tests\Views\BulkFormTest, test is green locally.

- Drupal\action\Tests\BulkFormTest does break locally, but with actual fails and no "unserialize(): Error" notices.
Basically, the drupalGet() of the test_bulk_form View URL just returns 0 bytes, and the rest of the test chokes.

yched’s picture

Probably another occurrrence of serialization subtleties between 5.3 / 5.4+
(my localhost is 5.5 :-/)

smiletrl’s picture

The two tests failed in my computer. I'm using php 5.4.13.

I just manually created a view with fields "action_bulk_form:" and "node_bulk_form:" , and the views page looks fine.

I just fond one intersting thing about the test views.
views.view.test_bulk_form.yml
contains :

          plugin_id: action_bulk_form
          provider: action

The views I just created has this:

          plugin_id: action_bulk_form
          provider: views
amateescu’s picture

Status: Needs work » Needs review
FileSize
11.95 KB
1.2 KB

This fixes it for me.

I guess we should really consider moving entity_reference to Core instead of a module..

smiletrl’s picture

amateescu++

yched’s picture

@amateescu: awesome - thanks !
Any idea why, though ? It looks weird for those tests to suddenly require entity_ref, we must be doing something wrong...

Berdir’s picture

PHP 5.5.3 segfaults for me when running these tests without that dependency in zend_hash_find, within serialization stuff. No idea why.

amateescu’s picture

FileSize
2.16 KB

@yched, no idea.. I just tried by instinct and it worked.

I looked more into it for about 6-7 hours this evening and it seems there's some weird issue with the list classes.

Basically, the interdiff in #48 fixes the failures because it replaces the list class from Drupal\Core\Field\FieldItemList to Drupal\entity_reference\Plugin\Field\FieldType\ConfigurableEntityReferenceFieldItemList. The funny thing is that even Drupal\Core\Field\ConfigFieldItemList works as a replacement, the "fix" is not tied to the configurable e_r list class, but to the fact that ConfigFieldItemList implements ConfigFieldItemListInterface...

This led me to Drupal\Core\Entity\FieldableEntityStorageControllerBase, and apparently the issue comes from iterating on entity fields in loadFieldItems(). If you apply the attached interdiff to the patch from #38, those two tests will pass. I've no effin` clue what might go wrong in there, so I hope either one of you will be able to crack it.

And a small shameless plug.. while investigating this stuff, I also found 3 unrelated bugs:
#2171753: entity_reference_field_info_alter() overrides 'list_class' twice
#2171757: Drupal\views\Form\ViewsForm should extend DependencySerialization
#2171759: Drupal\Core\Field\ConfigFieldItemList tries to use an inexistent FieldInstanceInterface

yched’s picture

Indeed, commenting out the lines in interdiff #52 fixes the tests.
More specifically, it's the
foreach ($translation as $field_name => $items) {
line in FEStorageControllerBase::loadFieldItems() that triggers the bug. Just leaving it with the full loop body commented out triggers the fails again.

Mind bending...

yched’s picture

Found it :-)

Well, not sure what's the exact flow to the fails, but this was (again) caused by a slightly different way to check "autocreate entity" : target_id === NULL vs. empty($target_id)...

Moved that logic to a unique internal helper, so that we stop writing this wrong...

The last submitted patch, 54: remove_ConfigEntityReferenceItemBase-2156337-54.patch, failed testing.

yched’s picture

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
12.02 KB
2.06 KB

Phew, that was fun! (not) :) Fixed some small doc issues and this is RTBC.

yched’s picture

yched’s picture

Reroll just in case

yched’s picture

yched’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 61: remove_ConfigEntityReferenceItemBase-2156337-61.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
yched’s picture

Back to RTBC

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

I think you meant this :)

amateescu’s picture

I just noticed that the reroll from #61 removed AllowedValuesInterface from the list of interfaces implemented by ConfigurableEntityReferenceItem. It's a minor fix so leaving at RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 66: remove_ConfigEntityReferenceItemBase-2156337-66.patch, failed testing.

yched’s picture

The last submitted patch, 66: remove_ConfigEntityReferenceItemBase-2156337-66.patch, failed testing.

amateescu’s picture

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

Bot acting crazy..

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f5ba64c and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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