Continued from #2800875: Autocreate for Entity Reference selection handlers that extend ViewsSelection.

EntityReferenceAutocompleteWidget has this code:

   protected function getAutocreateBundle() {
     $bundle = NULL;
    if ($this->getSelectionHandlerSetting('auto_create') && $target_bundles = $this->getSelectionHandlerSetting('target_bundles')) {
      // does stuff
    }
    return $bundle;
  }

As a result of this, anyone trying to implement autocreate in something that extends this widget has to define a target_bundles setting (in addition to an autocreate_bundle setting) , even where it is unnecessary and unnatural to define target_bundles, for example in situations like ER views or bundleless entities.

The solution is pretty much a one line fix: see #2800875: Autocreate for Entity Reference selection handlers that extend ViewsSelection #7 / #15 for the code.

But does this need tests? How should we test for this? Some guidance needed.

Comments

jonathanjfshaw created an issue. See original summary.

almaudoh’s picture

How should we test for this?

A way to do this is to create an entity that has not bundle with an entity_reference field. Then attempt to do auto-create on that field.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

#2 is right, a test for this issue would add a new test method to EntityReferenceAutoCreateTest and it can try to auto-create User entities because they do not have bundles, or it can use the \Drupal\entity_test\Entity\EntityTestNoBundle test entity type which is there for this reason :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pancho’s picture

Here's the one-liner from #2800875: Autocreate for Entity Reference selection handlers that extend ViewsSelection to start with. However, it doesn't work yet:

User warning: The 'Create referenced entities if they don't already exist' option is enabled but a specific destination bundle is not set.

BTW: Current code was introduced by #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles.

pancho’s picture

Status: Needs work » Needs review
StatusFileSize
new2.06 KB

New patch, manually tested to work flawlessly with either of:

  • entities with a single target bundle
  • entities with multiple target bundles but properly set autocreate bundle
  • + (new): bundle-less entities

If existing tests pass, we still need some additional tests. We also might want to refactor the code a bit further, so we don't have to discriminate between NULL and an empty string. But basically this should be working.

jonathanshaw’s picture

Issue tags: +Needs tests
jian he’s picture

Version: 8.6.x-dev » 8.7.x-dev
Issue tags: -Needs tests
StatusFileSize
new4.89 KB
new2.67 KB
jian he’s picture

StatusFileSize
new5.87 KB
new3.19 KB

Fix automated test. Only entity with label can be auto created.

RaphaelBriskie’s picture

StatusFileSize
new11.16 KB

Using patch #12 with 8.7.3 did not work for me. I combined the patch from here:

https://www.drupal.org/project/drupal/issues/2800875#comment-13143414

With patch #12 and it is working for me now.

Status: Needs review » Needs work

The last submitted patch, 13: 2821352-entity-reference-auto-create.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jonathanshaw’s picture

We definitely should not combine these patches.
#12 is still the right patch for this issue, and does something very different to #13.

#2800875: Autocreate for Entity Reference selection handlers that extend ViewsSelection may well never be fixed in core. The only relationship between these issues is that this one makes that one easier to solve in core or in your own code.

+++ b/core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAutoCreateTest.php
@@ -225,4 +225,49 @@ public function testMultipleTargetBundles() {
+   * Tests if an entity reference field have no bundles.

Nit: "Tests autocreation for an entity that has no bundles"

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -145,12 +146,19 @@ public function massageFormValues(array $values, array $form, FormStateInterface
   protected function getAutocreateBundle() {
...
+        $bundle = '';

I'm not sure about this fix. It works, but it seems unnecessarily clumsy.

I think we should solve this problem upstream: currently the EntotyAutocomplete element insists on a bundle setting, which seems wrong in principle.

Currently it has:

   if ($element['#autocreate']) {
      if (!isset($element['#autocreate']['bundle'])) {
        throw new \InvalidArgumentException("Missing required #autocreate['bundle'] parameter.");
      }

It shouldn't do this if the target entity type has no bundles.

If we did this we wouldn't be having to do distinguish between NULL and empty string as bundle values in the way the patch currently does. Instead, the widget would not set the bundle parameter on the element if there was no bundle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.9 was released on November 6 and is the final full bugfix release for the Drupal 8.7.x series. Drupal 8.7.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.8.0 on December 4, 2019. (Drupal 8.8.0-beta1 is available for testing.)

Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

chr.fritsch’s picture

Version: 8.9.x-dev » 9.0.x-dev
Status: Needs work » Needs review
StatusFileSize
new5.87 KB

I

  • rerolled the patch for 9.0.x
  • fixed the test, it contained deprecated 8.x code
  • fixed the nit from #15

I am not sure about @jonathanjfshaw second point in #15. Maybe this is a follow-up?

chr.fritsch’s picture

Version: 9.0.x-dev » 8.9.x-dev

Oops, this is still 8.9.x

chr.fritsch’s picture

StatusFileSize
new5.86 KB

This one applies to 8.9.x

chr.fritsch’s picture

Issue tags: +Bug Smash Initiative
larowlan’s picture

Looking solid here, one nitpick and one request re the tests

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -158,12 +159,19 @@ public function massageFormValues(array $values, array $form, FormStateInterface
    +      // If there's more than one target bundle, use the autocreate bundle stored in
    +      // selection handler settings.
    

    phpcs: > 80 chars here.

  2. +++ b/core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAutoCreateTest.php
    @@ -237,4 +237,50 @@ public function testMultipleTargetBundles() {
    +    $result = \Drupal::entityQuery('node')
    

    I think we should add an assertCount(1, $result) here, just to ensure that there is only one created node, to avoid a possible future false-positive

    Or alternatively, we could extend the query to use ->condition($field_name . '.target_id', $referenced_id) to ensure we get the exact item

chr.fritsch’s picture

StatusFileSize
new5.9 KB
new1.89 KB

Fixed #22

amateescu’s picture

Title: EntityReferenceAutocompleteWidget getAutoCreateBundle unnecessarily requires target_bundles setting » EntityReferenceAutocompleteWidget::getAutocreateBundle() unnecessarily requires the 'target_bundles' setting
Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
@@ -158,12 +159,19 @@ public function massageFormValues(array $values, array $form, FormStateInterface
+      // If there's no target bundle at all, pass an empty string. Entities are
+      // not required to have bundles.

While it's true that entities are not required to use/store bundle names, they do have one default bundle using the same name as the entity type.

Given that, I think the fix should be something like this:

1) \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::getAutocreateBundle() should return the target entity type ID as the "autocreate bundle name" if the target entity type doesn't use bundles.
2) \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::createNewEntity() shouldn't try to set a bundle value if the entity type doesn't have a bundle key.

chr.fritsch’s picture

Status: Needs work » Needs review
StatusFileSize
new2.3 KB
new7.14 KB

True story. I just realized I am already doing the same in the select2 module :)

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -158,12 +159,19 @@ public function massageFormValues(array $values, array $form, FormStateInterface
         $bundle = NULL;
    ...
    +        $bundle = $this->getFieldSetting('target_type');
    

    It feels like we could revert all the changes in this method if we do the default assignment to the entity type ID instead of NULL :)

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -117,7 +117,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    -    if ($this->getSelectionHandlerSetting('auto_create') && ($bundle = $this->getAutocreateBundle())) {
    +    $bundle = $this->getAutocreateBundle();
    +    if ($this->getSelectionHandlerSetting('auto_create') && !is_null($bundle)) {
    

    If we do that, the is_null() check wouldn't be needed anymore.

chr.fritsch’s picture

Hm, not sure. Because returning a bundle name when autocreate is not active feels a bit odd for getAutocreateBundle.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -117,7 +117,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    -    if ($this->getSelectionHandlerSetting('auto_create') && ($bundle = $this->getAutocreateBundle())) {
    +    $bundle = $this->getAutocreateBundle();
    +    if ($this->getSelectionHandlerSetting('auto_create') && !is_null($bundle)) {
    

    I don't get why this change is necessary.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -158,12 +159,19 @@ public function massageFormValues(array $values, array $form, FormStateInterface
        */
       protected function getAutocreateBundle() {
    

    Let's improve the documentation to state that NULL is returned when auto creation is not active.

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php
    @@ -158,12 +159,19 @@ public function massageFormValues(array $values, array $form, FormStateInterface
    +      // If there's no target bundle at all, pass the target_type. It's the
    +      // default for bundleless entity types.
    

    use the target_type
    Should we be checking if this is a bundleless entity type?

  4. We need a 9.1.x patch first...
    error: patch failed: core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAutoCreateTest.php:19
    error: core/modules/field/tests/src/Functional/EntityReference/EntityReferenceAutoCreateTest.php: patch does not apply
alexpott’s picture

#26.1 vs #27 is a tricky call. It comes from the fact we have two settings auto_create and auto_create_bundle that can possibly be out of sync. I think I agree with #27 - as then

if ($this->getSelectionHandlerSetting('auto_create') && ($bundle = $this->getAutocreateBundle())) {

can become

if ($bundle = $this->getAutocreateBundle()) {
alexpott’s picture

OTOH looking at http://grep.xnddx.ru/search?text=getAutocreateBundle&filename= #29 might cause a problem or two...

This all feels very awkward. The fact we check $this->getSelectionHandlerSetting('auto_create') inside \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget::getAutocreateBundle() and before we call it puts as in a very tricky position.

I think continuing to allow a NULL return from getAutocreateBundle is the way we have to go.

chr.fritsch’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Needs work » Needs review
StatusFileSize
new1.73 KB
new7.27 KB

Here is new patch. Now for 9.1.x

jonathanshaw’s picture

Status: Needs review » Needs work

I created #3172192: EntityAutocomplete element should not require autocreate bundle, but I no longer think we should postpone this issue on that, as it doesn't seem that it would now simplify the code here.

jonathanshaw’s picture

Status: Needs work » Reviewed & tested by the community

My bad, the review points have now all been addressed.

alexpott’s picture

Version: 9.1.x-dev » 9.0.x-dev

Committed 9e46cd9 and pushed to 9.1.x. Thanks!

Gonna ping other committers about backporting this.

  • alexpott committed 9e46cd9 on 9.1.x
    Issue #2821352 by chr.fritsch, jian he, Pancho, RaphaelBriskie,...
chr.fritsch’s picture

StatusFileSize
new7.26 KB

Thank you. Here is a patch for 8.9.x

alexpott’s picture

Version: 9.0.x-dev » 8.9.x-dev

Discussed with @catch who +1'd the backport. So I've cherry-picked to 9.0.x

  • alexpott committed 1d5aaac on 9.0.x
    Issue #2821352 by chr.fritsch, jian he, Pancho, RaphaelBriskie,...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0f469b6 and pushed to 8.9.x. Thanks!

  • alexpott committed 0f469b6 on 8.9.x
    Issue #2821352 by chr.fritsch, jian he, Pancho, RaphaelBriskie,...

Status: Fixed » Closed (fixed)

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