Option widgets integration is broken for the entity reference field

Confirmed. The gaps in Entity Reference testing are quite high. We need tests for each field configuration type. The current tests all assume the default autocomplete.

The problem here seems to be that getSettableOptions() has been deprecated.

Here's a version of a test I've been using with Domain to handle entity reference forms.

It illustrates the problem. When you try to generate a node form that contains something other than an autocomplete, it explodes.

Ideally, this test would loop through all the form display types,

It is likely that testEntityReferenceFormField() can be removed from this test, but I use it in contrib, so I left it in here.

Here's a very basic patch with a working test. This should at least unblock more work.

Fixes an issue when the sort field is set to none.

I have no idea why the other tests are failing.

Running test on local machine gave this:

Symfony\Component\Validator\Exception\ConstraintDefinitionException: Either "choices" or "callback" must be specified on constraint Choice in Symfony\Component\Validator\Constraints\ChoiceValidator->validate()

Right. But I cannot replicate that outside of the test. And I have no idea where in the call stack it's coming from.

Re-roll and basic clean-up.

Let's see if it works now after last changes.

Changing version to test against HEAD

12: entity_reference-2160575-form-fail-12.patch queued for re-testing.

After some digging I believe where are the problems with EntityReferenceFieldTest and EntityReferenceAutoCreateTest.

First one comes form the number of violations during validation. Before this patch, it threw only a ValidReferenceConstraint, but now it also throws a Choice violation related to the AllowedValuesInterface implementation. Test expects only 1 violation.

Fails in AutoCreateTest are due to the entity_reference field validation is done before the new entity is created automatically, so the $options array in getSettableOptions is empty and throws an exception in ChoiceValidator because $constraint->choices array is empty.

Maybe we could find something in the code related to taxonomy autocomplete widget, where it works fine.

Hope this helps.

#12 fixes the select widget for me

Coming from #2171393: Remove implementation of removed hook_options_list().

We need to have @amateescu take a look at that, but it seems this is because the code that should be in getSettableOptions() is still in entity_reference's implementation of the old hook_options_list(), which is a dead hook now.

Sorry, #19 is a bit cryptic.

I'd say the correct fix is to move the code from entity_reference_options_list() into getSettableOptions(), and then remove entity_reference_options_list().

Marking #2171393: Remove implementation of removed hook_options_list() as duplicate.

@yched, yeah this code should be moved to getSettableOptions(), but IMHO, that's not the problem here. Code from hook_options_list is invoked from getSettableOptions() by call_user_func_array() and works. Moving it to getSettableOptions() won't fix test fails, I think.

After some debugging, I believe that we should work on the direction I explained in #17.

But maybe I'm wrong ;)

Regards

Code from hook_options_list is invoked from getSettableOptions() by call_user_func_array() and works

Hmm - I don't see where ? AFAICT, entity_reference_options_list() is never ever called, and thus never does what it's there to do ?

getSettableOptions() calls $this->getFieldSetting('options_list_callback') if it's not empty, but that's something else entirely - just a custom callback possibly specified by a given field definition, that's not hook_options_list().

Of course, I was wrong :(

That comment was about one of the several tests I did, not about #12.

Anyway, I believe we have to do more changes that explained in #19.

Sorry for the noise.

@plopesc: ok, right, I didnt look at the tests and the fails, #20 was only about the "code fix" part, which doesn't seem to be doing the right thing.

So #20 doesnt invalidate #17 ;-)

So.. this is how I think the patch should look like. Bear in mind that it's written from scratch and it's sans tests.

For testing, I'd prefer to extend EntityReferenceIntegrationTest either with a new method, or by abstracting testConfigAutocompleteWidget() and make it test both a content and a config entity type, and do those in a loop for each widget supported by the e_r field.

The main problem that causes the fails in the patches above comes from the fact that right now, ConfigurableEntityReferenceItem is used for both configurable *and base* fields, because of entity_reference_field_info_alter(). This wasn't a problem so far because no "configurable field only" code was invoked for base fields, but since we're now implementing AllowedValuesInterface, a new constraint is added automatically by TypedDataManager that wreaks havoc when used on base fields (which mostly reference config entity types).

I couldn't find an acceptable solution for the above, so I just chose to get that validation out of the way, which I think is what we needed to do anyway since EntityReferenceItem already uses the custom "ValidReference" constraint.

Any other thoughts on that? Maybe we should also ping @fago for this.

Rewritten EntityReferenceIntegrationTest as proposed above. Now we test both e_r widgets and all the others supported by it, for both config and content entity types. This test already found a bug in the autocomplete tags widget which didn't support config entity types.

The failure in #25 looks legit, apparently the taxonomy reference validation relies on that constraint, so I moved the getConstraints() override to only affect the configurable e_r field.

Better title.

Great work @amateescu. Tests now are much more exhaustive. :)

Not sure if someone else should take a look at this, but I believe it could be RTBC'ed.

Regards.

The patch also fixes my contrib tests, which is what I had ported in #12.

Minor re-roll to catch up to head.

patching file core/modules/entity_reference/entity_reference.module
Hunk #1 succeeded at 144 (offset -1 lines).

Looks RTBC, but I have not reviewed code style or comments.

Any other thoughts on that? Maybe we should also ping @fago for this.

I think that the AllowedValues validation is superior than the ValidReference validaion as not any valid reference might be allowed. So if AllowedValues validation is able to take the job of ValidReference over, I think we should remove the ValidReference validadtion and always go with AllowedValues.

But why does the allowed values code only work for configurable fields? I couldn't spot the issue with a short look at the patch?

AllowedValues doesn't scale, we can't load 100k nodes to just check if 5 is a valid reference.

Maybe we could add a isAllowedValue($value) that defaults to calling the list and check if it's there and ER could optimize it?

But why does the allowed values code only work for configurable fields? I couldn't spot the issue with a short look at the patch?

I tried to explain why in the third paragraph of #25. If that doesn't make sense, let me know and I'll try harder :)

@Berdir, the isAllowedValue($value) addition sounds like a good improvement for AllowedValuesInterface, but I feel that it should be discussed in its own issue, where we can also update entity reference to use that validation. In the meantime, this patch fixes a critical bug..

Rerolled because #2156649: The 'entity_reference_autocomplete' widget doesn't handle multiple values correctly went in and it contained most of the test changes from this patch.

Agreed, improving the performance of AllowedValuesInterface and possibly replacing ValidReference sounds like a good follow-up, but let's not block this issue on that and until we do that, we can not rely on AllowedValuesInterface.

Opened #2188933: Evaluate using AllowedValues validation for the EntityReference field item to discuss the usage of AllowedValuesInterface validation and addressing its performance issue.

Yes this looks like a much better stop-gap than loading all possible items then filtering out. Committed/pushed to 8.x, thanks!