Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The "autocreate entity" code from ER is a bit outdated, we need to bring it up to speed with the new Entity Field API. This has been found while working on #1969728: Implement Field API "field types" as TypedData Plugins.
Comment | File | Size | Author |
---|---|---|---|
#28 | 2016177-followup-28.patch | 5.49 KB | amateescu |
#28 | interdiff.txt | 1.46 KB | amateescu |
#26 | 2016177-followup-26-test-only.patch | 1.24 KB | amateescu |
#26 | 2016177-followup-26.patch | 4.96 KB | amateescu |
#24 | EntityReferenceFieldTest.php_.txt | 2.5 KB | agentrickard |
Comments
Comment #1
amateescu CreditAttribution: amateescu commentedHere's the combined patch of #1992138-44: Helper issue for "field types as TypedData Plugins" from @Berdir and #48 from @yched.
Comment #2
yched CreditAttribution: yched commentedComment #3
amateescu CreditAttribution: amateescu commentedAnd straight to RTBC since I'm the reviewer here, I didn't write any code for the patch above :)
Comment #4
yched CreditAttribution: yched commentedThanks @amateescu !
RTBC +1 (although I did write part of the code)
Comment #5
yched CreditAttribution: yched commented- add an explicit 'target_id' => 0 in autocreate items, according to what has been settled in #2012662: Constraints on 'target_id' / 'tid' properties break autocomplete if applied
- The formatters need to be adjusted accordingly for the new convention (there is no 'target_id' = 'autocreate' anymore)
Comment #6
yched CreditAttribution: yched commentedEr, wrong interdiff.
Comment #7
amateescu CreditAttribution: amateescu commentedLooks great, thanks @yched!
Comment #8
yched CreditAttribution: yched commentedtestbot seems a bit lazy - reuploading
Comment #9
alexpottCommitted cd8a251 and pushed to 8.x. Thanks!
Comment #10
yched CreditAttribution: yched commentedOoops - trivial coding style followup.
Comment #11
alexpottCommitted 82d44b7 and pushed to 8.x. Thanks!
Comment #12
tstoecklerCan someone explain the following, it looks weird to me:
the count() == 1 check here seems wrong. If multiple target bundles are specified, the chosen one should still be one of them, and not a random one, right?
Comment #13
BerdirRe-opening this for now as a reminder, fine with handling this in a follow-up.
@agentrickard asked in IRC about notices/fatal errors on entity_reference.module:123, which this patch changes for domain.module. I can reproduce on both Node and Term, not sure why our tests aren't failing?
To reproduce:
- Add a reference field on nodes pointing to nodes. Tested with both autocomplete and options list widgets
- Try to create a node with a refererence.
- Fatal error called to method isNew on non-object.
Comment #14
amateescu CreditAttribution: amateescu commentedYep, we had a faulty check there. Fixed that, the count() problem found by @tstoeckler in #12 and a bunch of other things that were using $this->field and $this->instance instead of the new merged array.
Comment #15
agentrickardConfirming that the last patch fixes my failing tests ;-).
Comment #16
BerdirThats great, but we need a regression test and understand why our current tests are not failing and while we're at it, also understand why entity isn't there.. it should be IMHO :)
Comment #17
amateescu CreditAttribution: amateescu commentedWould it make sense to wait on #1969728: Implement Field API "field types" as TypedData Plugins for the tests? Basically, we would need to test hook_field_presave() and it will conflict with that patch :/
Comment #18
BerdirFine with me, I think that issue might get in soon :)
I think the tag was removed accidently?
Comment #19
agentrickardThat main issue #1969728: Implement Field API "field types" as TypedData Plugins is in. Are there followups we care about here?
Comment #20
amateescu CreditAttribution: amateescu commentedMaybe on #2015701: Convert field type to typed data plugin for entity reference module , but I'm not really sure.. the hour is too late :P
Comment #21
agentrickardHm. Something changed in the last week, as my original tests no longer fail, nor does the similar test I wrote for this issue.
Comment #22
BerdirThat means the field types conversion fixed it. Wondering if we can close this without having to add test, as we still don't know why exactly the tests didn't fail...
Comment #23
amateescu CreditAttribution: amateescu commentedWe can close it.. but the patch from #14 stil has some cleanups for the patch that was originally committed here, so I think it should go in even without tests (since we cannot test what we can't reproduce now..) :/
Comment #24
agentrickardHere's the new test file, if we want to include it anyway.
Comment #25
amateescu CreditAttribution: amateescu commentedOhhh, after looking at your patch I just remembered that we actually have another patch in the queue that adds more tests for ER: #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted.
Comment #26
amateescu CreditAttribution: amateescu commentedI managed to test the current broken state of Entity reference with a small addition to an existing test :)
Comment #27
BerdirLet's make this a "positive" test, by verifying that the referenced thing is actually displayed, not that no error is there.
Comment #28
amateescu CreditAttribution: amateescu commentedSure thing, added an assertion for the referenced node label too.
Comment #29
agentrickardTests for core (and my contrib) are green locally, so this is RTBC when the bot catches up.
Comment #30
BerdirYes, this looks good.
Comment #31
alexpottCommitted 82d9d6f and pushed to 8.x. Thanks!