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.
Comment | File | Size | Author |
---|---|---|---|
#36 | 2821352-31-d89.patch | 7.26 KB | chr.fritsch |
#31 | 2821352-31.patch | 7.27 KB | chr.fritsch |
#31 | interdiff-2821352-25-31.txt | 1.73 KB | chr.fritsch |
#25 | 2821352-25.patch | 7.14 KB | chr.fritsch |
#25 | interdiff-2821352-23-25.txt | 2.3 KB | chr.fritsch |
Comments
Comment #2
almaudoh CreditAttribution: almaudoh commentedA 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.
Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#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 :)Comment #8
PanchoHere's the one-liner from #2800875: Autocreate for Entity Reference selection handlers that extend ViewsSelection to start with. However, it doesn't work yet:
BTW: Current code was introduced by #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles.
Comment #9
PanchoNew patch, manually tested to work flawlessly with either of:
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.
Comment #10
jonathanshawComment #11
jian he CreditAttribution: jian he commentedComment #12
jian he CreditAttribution: jian he commentedFix automated test. Only entity with label can be auto created.
Comment #13
RaphaelBriskie CreditAttribution: RaphaelBriskie as a volunteer commentedUsing 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.
Comment #15
jonathanshawWe 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.
Nit: "Tests autocreation for an entity that has no bundles"
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:
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.
Comment #18
chr.fritschI
I am not sure about @jonathanjfshaw second point in #15. Maybe this is a follow-up?
Comment #19
chr.fritschOops, this is still 8.9.x
Comment #20
chr.fritschThis one applies to 8.9.x
Comment #21
chr.fritschComment #22
larowlanLooking solid here, one nitpick and one request re the tests
phpcs: > 80 chars here.
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
Comment #23
chr.fritschFixed #22
Comment #24
amateescu CreditAttribution: amateescu at Centarro commentedWhile 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 abundle
key.Comment #25
chr.fritschTrue story. I just realized I am already doing the same in the select2 module :)
Comment #26
amateescu CreditAttribution: amateescu at Centarro commentedIt 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 :)
If we do that, the
is_null()
check wouldn't be needed anymore.Comment #27
chr.fritschHm, not sure. Because returning a bundle name when autocreate is not active feels a bit odd for getAutocreateBundle.
Comment #28
alexpottI don't get why this change is necessary.
Let's improve the documentation to state that NULL is returned when auto creation is not active.
use the target_type
Should we be checking if this is a bundleless entity type?
Comment #29
alexpott#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
can become
Comment #30
alexpottOTOH 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.
Comment #31
chr.fritschHere is new patch. Now for 9.1.x
Comment #32
jonathanshawI 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.
Comment #33
jonathanshawMy bad, the review points have now all been addressed.
Comment #34
alexpottCommitted 9e46cd9 and pushed to 9.1.x. Thanks!
Gonna ping other committers about backporting this.
Comment #36
chr.fritschThank you. Here is a patch for 8.9.x
Comment #37
alexpottDiscussed with @catch who +1'd the backport. So I've cherry-picked to 9.0.x
Comment #39
alexpottCommitted 0f469b6 and pushed to 8.9.x. Thanks!