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.

Files: 
CommentFileSizeAuthor
#28 2016177-followup-28.patch5.49 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es).
[ View ]
#28 interdiff.txt1.46 KBamateescu
#26 2016177-followup-26-test-only.patch1.24 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 57,990 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#26 2016177-followup-26.patch4.96 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 57,992 pass(es).
[ View ]
#24 EntityReferenceFieldTest.php_.txt2.5 KBagentrickard
#14 2016177-followup.patch3.72 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 58,001 pass(es).
[ View ]
#10 minor-followup-2016177.patch788 bytesyched
PASSED: [[SimpleTest]]: [MySQL] 57,590 pass(es).
[ View ]
#8 e_r-better_autocreate-2016177-5.patch13.32 KByched
PASSED: [[SimpleTest]]: [MySQL] 58,307 pass(es).
[ View ]
#6 interdiff.txt5.46 KByched
#5 e_r-better_autocreate-2016177-5.patch13.32 KByched
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#5 interdiff.txt641 bytesyched
#1 2016177-better_autocreate.patch8.51 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 56,487 pass(es).
[ View ]

Comments

Title:Refactor 'autocreate' handling from entity_referenceRefactor 'autocreate entity' handling from entity_reference
Status:Active» Needs review
StatusFileSize
new8.51 KB
PASSED: [[SimpleTest]]: [MySQL] 56,487 pass(es).
[ View ]

Here's the combined patch of #1992138-44: Helper issue for "field types as TypedData Plugins" from @Berdir and #48 from @yched.

And straight to RTBC since I'm the reviewer here, I didn't write any code for the patch above :)

Status:Needs review» Reviewed & tested by the community

Thanks @amateescu !
RTBC +1 (although I did write part of the code)

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new641 bytes
new13.32 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new5.46 KB

Er, wrong interdiff.

Status:Needs review» Reviewed & tested by the community

Looks great, thanks @yched!

StatusFileSize
new13.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,307 pass(es).
[ View ]

testbot seems a bit lazy - reuploading

Status:Reviewed & tested by the community» Fixed

Committed cd8a251 and pushed to 8.x. Thanks!

Status:Fixed» Reviewed & tested by the community
StatusFileSize
new788 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,590 pass(es).
[ View ]

Ooops - trivial coding style followup.

Status:Reviewed & tested by the community» Fixed

Committed 82d44b7 and pushed to 8.x. Thanks!

Can someone explain the following, it looks weird to me:

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Plugin/field/widget/AutocompleteWidgetBase.php
@@ -120,4 +124,39 @@ protected function getLabels(array $items) {
+    // Get the bundle.
+    if (!empty($this->instance['settings']['handler_settings']['target_bundles']) && count($this->instance['settings']['handler_settings']['target_bundles']) == 1) {
+      $bundle = reset($this->instance['settings']['handler_settings']['target_bundles']);
+    }
+    else {
+      $bundles = entity_get_bundles($target_type);
+      $bundle = reset($bundles);

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?

Category:task» bug
Priority:Normal» Major
Status:Fixed» Active

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

Status:Active» Needs review
StatusFileSize
new3.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,001 pass(es).
[ View ]

Yep, 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.

Status:Needs review» Reviewed & tested by the community

Confirming that the last patch fixes my failing tests ;-).

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

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

Issue tags:-Needs tests

Would 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 :/

Issue tags:+Needs tests

Fine with me, I think that issue might get in soon :)

I think the tag was removed accidently?

That main issue #1969728: Implement Field API "field types" as TypedData Plugins is in. Are there followups we care about here?

Maybe 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

Hm. Something changed in the last week, as my original tests no longer fail, nor does the similar test I wrote for this issue.

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

We 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..) :/

StatusFileSize
new2.5 KB

Here's the new test file, if we want to include it anyway.

Ohhh, 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 instance settings when referenced entity bundles are renamed or deleted.

Title:Refactor 'autocreate entity' handling from entity_referenceRegression: Refactor 'autocreate entity' handling from entity_reference
Priority:Major» Critical
Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new4.96 KB
PASSED: [[SimpleTest]]: [MySQL] 57,992 pass(es).
[ View ]
new1.24 KB
FAILED: [[SimpleTest]]: [MySQL] 57,990 pass(es), 1 fail(s), and 2 exception(s).
[ View ]

I managed to test the current broken state of Entity reference with a small addition to an existing test :)

+++ b/core/modules/entity_reference/lib/Drupal/entity_reference/Tests/EntityReferenceAutoCreateTest.phpundefined
@@ -110,5 +113,9 @@ public function testAutoCreate() {
+    $this->assertNoText('Exception');

Let's make this a "positive" test, by verifying that the referenced thing is actually displayed, not that no error is there.

StatusFileSize
new1.46 KB
new5.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es).
[ View ]

Sure thing, added an assertion for the referenced node label too.

Tests for core (and my contrib) are green locally, so this is RTBC when the bot catches up.

Status:Needs review» Reviewed & tested by the community

Yes, this looks good.

Status:Reviewed & tested by the community» Fixed

Committed 82d9d6f and pushed to 8.x. Thanks!

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