Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rely on ids + support autocreate. #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Artusamak
Copy link

The current implementation looks bugged. At the moment, you never run line 28 if we are testing line 21 for an empty target bundle.
If the field is filtering a specific bundle of taxonomy terms for instance, you want to set the value of $target_bundle_key when you have multiple bundles too.
Steps to reproduce the issue, create two taxonomy terms with the same name in two different vocabularies, let's say voc1 > item & voc2 > item. Tid are respectively 1 and 2.
Now, try to create a node through the Drupal 8 behat driver that has an entity reference field with a configuration of entity type taxonomy terms and vocabulary "voc2" as target bundle. Your node should be created but with the wrong term ID because there will be a match to the "item" term that has the tid 1 instead of the tid 2 expected.
If we reference entities like medias, names are useless. Relying on entity ids is more logical and helps deduplicate the issue mentionned above.
We also need to support auto created entities like taxonomy terms. If the term is not known, do not throw the exception.

If we reference entities like medias, names are useless. Relying on entity ids is more logical.
We also need to support auto created entities like taxonomy terms. If the term is not known, do not throw the exception.
@jonathanjfshaw
Copy link
Contributor

The current implementation looks bugged. At the moment, you never run line 28 if we are testing line 21 for an empty target bundle.
If the field is filtering a specific bundle of taxonomy terms for instance, you want to set the value of $target_bundle_key when you have multiple bundles too.
Steps to reproduce the issue, create two taxonomy terms with the same name in two different vocabularies, let's say voc1 > item & voc2 > item. Tid are respectively 1 and 2.
Now, try to create a node through the Drupal 8 behat driver that has an entity reference field with a configuration of entity type taxonomy terms and vocabulary "voc2" as target bundle. Your node should be created but with the wrong term ID because there will be a match to the "item" term that has the tid 1 instead of the tid 2 expected.

Sorry Artusamak, I understand the problem you're describing but not what the bug is in the code that's causing it. If $this->getTargetBundles() on line 21 returns 'voc2' then why does line 28 not run?

If we reference entities like medias, names are useless. Relying on entity ids is more logical

How do you imagine this fitting with BDD? At the moment a Behat step author can reference an entity by name in a human-readable table. That's important. Wouldn't the change you propose break those steps in the Drupal Extension?

Ultimately I think this is yet another case where we need plugins in the Driver. I had imagined plugins for entities and (better) plugins for fields, but looks like maybe we need plugins for references as well. That way a developer could add a plugin to their project that modified the logic used when referencing a particular entity type from a particular field on a particular entity type or whatever.

Using the label addresses the 90% use case but is not perfect. We've already got a case in another PR where we're adding hardcoded logic to handle the fact that the user entity don't have a label key and needs to be using 'name'.

I thought media entities in D8 have a label? I agree it's rather awkward though and often hidden from users and so it can violate BDD principles to be using it.

We also need to support auto created entities like taxonomy terms. If the term is not known, do not throw the exception.

That seems right from a BDD perspective. At the moment the referenced entities have to be previously created. Your fix makes writing a step closer to the experience of the site user. I didn't know this actually worked in Drupal, that you could autocreate through the API in this way.

@jonathanjfshaw
Copy link
Contributor

See also See also #94 and #117 for discussions addressing this problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants