Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#66 | interdiff.txt | 1.1 KB | amateescu |
#66 | remove_ConfigEntityReferenceItemBase-2156337-66.patch | 12.72 KB | amateescu |
#61 | remove_ConfigEntityReferenceItemBase-2156337-61.patch | 12.7 KB | yched |
#59 | remove_ConfigEntityReferenceItemBase-2156337-59.patch | 12.02 KB | yched |
#57 | interdiff.txt | 2.06 KB | amateescu |
Comments
Comment #1
yched CreditAttribution: yched commentedPatch, let's see what breaks.
Comment #2
yched CreditAttribution: yched commentedFileItem & ImageItem would benefit from this too.
Comment #3
amateescu CreditAttribution: amateescu commentedIf nothing breaks, I'm super excited for this patch :)
(Except for that whitespace you have in the interdiff :P)
Comment #4
yched CreditAttribution: yched commentedDamn. I love PhpStorm, but I *hate* that it doesn't trim whitespaces on the line your cursor is :-p
Comment #8
yched CreditAttribution: yched commentedWeird, 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 ? ;-)
Comment #9
yched CreditAttribution: yched commentedThis being said... let's see what this fixes / breaks :-)
Comment #11
yched CreditAttribution: yched commentedBetter :-)
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.
Comment #12
amateescu CreditAttribution: amateescu commentedlol :)
Shouldn't this do the same
$target_id === NULL
check like isEmpty()?Comment #13
yched CreditAttribution: yched commented;-)
Yeah, most probably.
Comment #14
amateescu CreditAttribution: amateescu commentedYay, thanks!
Comment #16
amateescu CreditAttribution: amateescu commentedOops..
Comment #17
Berdir#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..
Comment #18
yched CreditAttribution: yched commentedWell, 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 ?
Comment #19
yched CreditAttribution: yched commentedReroll
Comment #20
yched CreditAttribution: yched commentedComment #21
yched CreditAttribution: yched commentedShould fix the fails ?
Comment #22
yched CreditAttribution: yched commentedforgot the interdiff
Comment #24
yched CreditAttribution: yched commentedGreen :-p
Comment #25
BerdirMore green :p
Nice cleanup.
Comment #26
xjm21: remove_ConfigEntityReferenceItemBase-2156337-21.patch queued for re-testing.
Comment #29
yched CreditAttribution: yched commented21: remove_ConfigEntityReferenceItemBase-2156337-21.patch queued for re-testing.
Comment #30
smiletrl CreditAttribution: smiletrl commentedRerolled, 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()?Comment #31
yched CreditAttribution: yched commentedThanks for the reroll @smiletrl.
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.
image_entity_presave() reacts to "an entity (here a Field or FieldInstance) being saved". So no, it's completely different from imageitem:presave().
Comment #32
amateescu CreditAttribution: amateescu commentedAuto-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 :)
Comment #34
smiletrl CreditAttribution: smiletrl commentedYeah, 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.
Comment #35
BerdirYou can't delete that, that is necessary.
Comment #37
yched CreditAttribution: yched commentedYes, #34 is a no-go. We need to find why #30 has issues with image fields, it worked earlier.
Comment #38
yched CreditAttribution: yched commentedThat'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).
Comment #40
Berdir38: remove_ConfigEntityReferenceItemBase-2156337-38.patch queued for re-testing.
Comment #41
BerdirWeird unserialize errors (unserialize(): Error at offset 867267 of 1187046 bytes), re-testing.
Comment #44
BerdirApparently not random then.
Comment #45
yched CreditAttribution: yched commentedWoah, 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.
Comment #46
yched CreditAttribution: yched commentedProbably another occurrrence of serialization subtleties between 5.3 / 5.4+
(my localhost is 5.5 :-/)
Comment #47
smiletrl CreditAttribution: smiletrl commentedThe 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 :
The views I just created has this:
Comment #48
amateescu CreditAttribution: amateescu commentedThis fixes it for me.
I guess we should really consider moving entity_reference to Core instead of a module..
Comment #49
smiletrl CreditAttribution: smiletrl commentedamateescu++
Comment #50
yched CreditAttribution: yched commented@amateescu: awesome - thanks !
Any idea why, though ? It looks weird for those tests to suddenly require entity_ref, we must be doing something wrong...
Comment #51
BerdirPHP 5.5.3 segfaults for me when running these tests without that dependency in zend_hash_find, within serialization stuff. No idea why.
Comment #52
amateescu CreditAttribution: amateescu commented@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
toDrupal\entity_reference\Plugin\Field\FieldType\ConfigurableEntityReferenceFieldItemList
. The funny thing is that evenDrupal\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 implementsConfigFieldItemListInterface
...This led me to
Drupal\Core\Entity\FieldableEntityStorageControllerBase
, and apparently the issue comes from iterating on entity fields inloadFieldItems()
. 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
Comment #53
yched CreditAttribution: yched commentedIndeed, 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...
Comment #54
yched CreditAttribution: yched commentedFound 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...
Comment #56
yched CreditAttribution: yched commentedSilly yched.
Comment #57
amateescu CreditAttribution: amateescu commentedPhew, that was fun! (not) :) Fixed some small doc issues and this is RTBC.
Comment #58
yched CreditAttribution: yched commented57: remove_ConfigEntityReferenceItemBase-2156337-57.patch queued for re-testing.
Comment #59
yched CreditAttribution: yched commentedReroll just in case
Comment #60
yched CreditAttribution: yched commented59: remove_ConfigEntityReferenceItemBase-2156337-59.patch queued for re-testing.
Comment #61
yched CreditAttribution: yched commentedReroll after #2160575: Option widgets integration is broken for the entity reference field + Bump :-)
Comment #63
yched CreditAttribution: yched commented61: remove_ConfigEntityReferenceItemBase-2156337-61.patch queued for re-testing.
Comment #64
yched CreditAttribution: yched commentedBack to RTBC
Comment #65
amateescu CreditAttribution: amateescu commentedI think you meant this :)
Comment #66
amateescu CreditAttribution: amateescu commentedI 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.
Comment #68
yched CreditAttribution: yched commented66: remove_ConfigEntityReferenceItemBase-2156337-66.patch queued for re-testing.
Comment #70
amateescu CreditAttribution: amateescu commented66: remove_ConfigEntityReferenceItemBase-2156337-66.patch queued for re-testing.
Comment #71
amateescu CreditAttribution: amateescu commentedBot acting crazy..
Comment #72
alexpottCommitted f5ba64c and pushed to 8.x. Thanks!