Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Title: Refactor 'autocreate' handling from entity_reference » Refactor 'autocreate entity' handling from entity_reference
Status: Active » Needs review
FileSize
8.51 KB

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

yched’s picture

amateescu’s picture

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

yched’s picture

Status: Needs review » Reviewed & tested by the community

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

yched’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
641 bytes
13.32 KB

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

yched’s picture

FileSize
5.46 KB

Er, wrong interdiff.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks @yched!

yched’s picture

testbot seems a bit lazy - reuploading

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cd8a251 and pushed to 8.x. Thanks!

yched’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
788 bytes

Ooops - trivial coding style followup.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

tstoeckler’s picture

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?

Berdir’s picture

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.

amateescu’s picture

Status: Active » Needs review
FileSize
3.72 KB

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.

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

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

Berdir’s picture

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

amateescu’s picture

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

Berdir’s picture

Issue tags: +Needs tests

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

I think the tag was removed accidently?

agentrickard’s picture

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

amateescu’s picture

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

agentrickard’s picture

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

Berdir’s picture

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

amateescu’s picture

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

agentrickard’s picture

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

amateescu’s picture

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 field settings when referenced entity bundles are deleted.

amateescu’s picture

Title: Refactor 'autocreate entity' handling from entity_reference » Regression: Refactor 'autocreate entity' handling from entity_reference
Priority: Major » Critical
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.96 KB
1.24 KB

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

Berdir’s picture

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

amateescu’s picture

FileSize
1.46 KB
5.49 KB

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

agentrickard’s picture

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

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, this looks good.

alexpott’s picture

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.