We have a node with an entityreference field that only allows one other node type as reference values. When creating content in this node the autocomplete is showing valid values. However it's possible to bypass the type by pasting a valid NID in the field.

So practical example:
Title | NID | Node type
Test 1 | 1234 | X
Test 2 | 5678 | Y

When creating a node of type Z with the node reference field set to only allow node type X. It's possible to reference to node "Test 2" of type Y by pasting the value "Test 2 (5678)" into the field. Actually the title value doesn't matter, if a valid NID is supplied you can save the entity.

What I think is happening is because we supplied a valid node id it's bypassing the check for the type. Or was the "allowed types" option in the field settings never intended for validation on the save event and only for the dropdown values.

Our users discovered this when copying over the data from different environments (the node ID's differ of course on each environment) hence the weird linking occurred.

Field configuration used: Simple (with optional filter by bundle).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

BramDriesen created an issue. See original summary.

BramDriesen’s picture

Title: Saving entity ignores the allowed bundles settings when a valid nid is supploed » Saving entity ignores the allowed bundles settings when a valid nid is supplied

Corrected a typo.

BramDriesen’s picture

If I have time today I'll try to dig deeper into the issue and try to propose a patch for it.

BramDriesen’s picture

Finally had time to debug the issue, turns out it was introduced a long time ago. (see linked issue). Basically what's happening is when you supply a random string and a valid id like `foo (456)` of a different bundle as allowed, the `$valid_ids` array would be empty and thus skipping adding the warnings. This is only a problem when you try to add one bogus item or all bogus items. Patch attached.

BramDriesen’s picture

Actually, I was not happy with the patch. Refactored it a bit so we don't have the foreach duplicated. Interdiff and patch attached.

hgoto’s picture

Status: Needs review » Needs work

Thank you for the patch. I could reproduce the problem. Also I tested the patch manually and it works well.

Though, it may be possible to make the patch simpler. For example, replacing the following line in entityreference_field_validate() (the function you're patching) seems to work.

-    if (!empty($valid_ids)) {
+    if (count($valid_ids) < count($ids)) {

What do you think?

BramDriesen’s picture

Status: Needs work » Needs review

Your change won't work because the function will then not process the ID's. The array_flip on the valid_ids array (which is empty in my use case) will fail. That's the reason why the if wrapper was added in the linked issue. So I still think my patch is the best solution.

hgoto’s picture

Your change won't work because the function will then not process the ID's. The array_flip on the valid_ids array (which is empty in my use case) will fail.

Hmm. I don't understand what you told well. Which function do you mean by "the function"? And what does "not process the ID's" mean concretely?

Seeing all the code of validateReferencableEntities() in Entity Reference module again, it seems to always return an array and array_flip($valid_ids) works. Is there a case that array_flip($valid_ids) doesn't work?

BramDriesen’s picture

@hgoto Check the errors thrown in the linked issue.

Warning: array_flip() expects parameter 1 to be array, null given in entityreference_field_validate() (line 224 of /srv/http/epik/sites/all/modules/entityreference/entityreference.module).

In my scenario there is no valid data, thus the $valid_ids array is empty. If we apply your change to the if we will go inside the if and then crash on this line.

$invalid_entities = array_diff_key($ids, array_flip($valid_ids));

Because we then do again an array_flip() on an empty array like in the linked issue.

That, is why your solution is not going to work and I prefer my patch.

PS: With "the function" I mean the function I'm patching.

BramDriesen’s picture

Small bump. It's still a major issue...

BramDriesen’s picture

We're using this patch now for over a year in production without issues. Can this be reviewed once again and maybe put into the next release?

maurizio.ganovelli’s picture

The patch works well for me also; i've applied it on a production platform last week and no problem so far.

webadpro’s picture

WAnted to chip in saying the patch works for me also.

BramDriesen’s picture

Status: Needs review » Reviewed & tested by the community

I guess we can set it to RTBC then and just hope someone might commit it :-)

BramDriesen’s picture

Priority: Major » Critical
BramDriesen’s picture

Version: 7.x-1.5 » 7.x-1.x-dev

Updating to dev version. Not sure if the patch still applies.

drumm’s picture

BramDriesen’s picture

Seems related indeed.

minorOffense’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

Closing as duplicate. Feel free to reopen if you add a valid test before the other issue resolves their test problem (or preferably fix their test).