Exception when creating an entity reference field targeting an entity type without an ID

Why not use admin>config>search>URL aliases to add a pretty path to the existing form?

Are you referencing contact form or contact message? Either way it won't do what you want.

Able to replicate the issue with the steps provided.Here is the error log message which might helps in investigating/fixing the issue.

Uncaught PHP Exception Drupal\\Core\\Database\\DatabaseExceptionWrapper: "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal_8_rc3.node_revision__field_contact' doesn't exist: SELECT 1 AS expression\nFROM \n{node_revision__field_contact} t\nWHERE ( (field_contact_target_id IS NOT NULL ) )\nLIMIT 1 OFFSET 0; Array\n(\n)\n at /drupal-8-rc3/core/lib/Drupal/Core/Database/Connection.php line 676,

Still occurring in 8.1.1,see attached detailed error page.

FWIW contact_storage in contrib makes this possible - it adds a view builder so you can reference a form and use 'rendered entity' to embed the form

I got the problem with the same steps, with an "SQLSTATE[42S02]: Base table or view not found". The problem is that the table really doesn't exist in the DB, and I get a fatal error on my drupal install and can't access either front or back office anymore. Not sure yet how I'm going to get my site back...
If this action really doesn't work (and worse get sites to crash), why keep it? Either it should work or disappear.

This issue was also in Drupal 8.6.12. refer the image attachments.

Re-titling to try to make it clearer what's going on. Moving to contact module because it's likely an issue there (unless we're missing a mechanism to prevent entity types from being referencable).

Also downgrading to major per https://www.drupal.org/core/issue-priority

Updating because this works fine when you try to reference a contact *form*, it only errors if you try to reference a contact *message*.

I'm bumping this down to normal as it's hard to fathom intentionally referencing a contact message? It's not clear what that would even be since contact messages are not stored without contact storage?

Updating the IS too because it's a little confusing that it references contact forms before the STR specify contact message.

Doh.

Bumping this back to major because I've just realised (and #10 points out) the field can't be deleted through the UI, and it leads to a fatal at admin/content or when viewing any node of the type with this field.

So it may even be critical? A workaround might be to change the yml to reference a different entity type and import the updated config? Haven't tried this as simplytest doesn't support config import.

It's possible you could do this by accident, as Contact message sits between comment and content in the entity reference type list.

The fix here is to change \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::storageSettingsForm to filter out entity types that don't have an ID key

Closed #3114984: Error adding and deleting contact form link fields in custom block type as a duplicate so adding credit here.

Per #18 this is not related to the contact module per-se

Tagging for Novice, happy to mentor someone on this

This might help.

@larowlan I wrote a small snippet to filter types which do not have id in their entity_keys

$options = \Drupal::service('entity_type.repository')->getEntityTypeLabels(TRUE);
    foreach ($options as $group => $option) {
      foreach ($option as $label => $value) {
        $entity_type = \Drupal::service('entity_type.manager')->getDefinition($label);
        if (!$entity_type->hasKey('id')) {
            unset($options[$group][$label]);
        }
      }
    }
    $element['target_type']['#options'] = $options;

However filtering types in storageSettingsForm using entity_type.manager seems like an overkill to me since the getEntityTypeLabels method itself used entity_type.manager to get the labels of types. So instead it would be better if we create a different method in EntityTypeRepository class to only fetch entity types which have id, or we can add a parameter in the getEntityTypeLabels to accept a filter key and return only the matching types

I think having a filter param which accepted an optional callable that could be passed to array_filter would probably be the cleanest approach, but let me poll the other framework managers first

@shriaas2898 I discussed this with alexpott and we agreed that filtering it in the settings form is fine.

Thanks

So should I create a patch with snippet suggested in #25 ?

Adding patches with changes, I have added a function to check if the entity type has id key and used it to filter the options array. Also I don't think we can use array_intersect_key here because the getDefinition() function returns an object of \Drupal\Core\Entity\EntityTypeInterface not an array. Please check if this looks fine.

Applied patch #29 and it removes the Contact Message option from type of item to reference.

Before patch:

After patch:

Removing unnecessary line and using single quotes

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/EntityReferenceItem.php
@@ -353,16 +353,20 @@ class EntityReferenceItem extends FieldItemBase implements OptionsProviderInterf
+    $options = \Drupal::service('entity_type.repository')->getEntityTypeLabels(TRUE);

is there a way to inject the service?

Hi, I applied the patch, and is working fine!

I'm attaching the screenshots below,

Before,

After,

Considering the below change,

.. I think the previous documentation was better as it is more meaningful! Otherwise RTBC!

Updated patch works as intended.
Comment #32 asks for dependency injection. Should it be done here and be the only DI of the whole class, or should it be another issue altogether?
Also, patch works moving forward, but user who had used contact message fields will have their sites entirely broken, should we fix that (if possbile) ?
Still needs tests so I'm moving it back to needs work.

is there a way to inject the service?

@guilhermevp I think adding DI should be a separate issue since other methods of the class will needed to be changed too. Need maintainers suggestion on this.

Also, patch works moving forward, but user who had used contact message fields will have their sites entirely broken, should we fix that (if possbile) ?

Well the issue here was exactly that users are not able to use contact messages as entity reference, so I don't understand how they could have existing field for contact messages 🤔 am I missing something?

I think adding DI should be a separate issue since other methods of the class will needed to be changed too. Need maintainers suggestion on this.

I fully agree! Just opened the question because I'm new here, and I think someone more experienced should comment on the matter, since it is a core issue.

I missing something?

No, it's just that I notice that the comment #10 could happened to more people. But then again, I believe that we should move forward on the issue.

I hope y'all don't mind that I converted this to a merge request :) They're just so much easier to review in GitLab. (For the record, anybody can push changes to the issue fork; just click the green "Get push access" button at the bottom of the issue summary, then "Show commands" to see how to set up your local git to work with the issue fork. Further documentation about how to work with merge requests on drupal.org can be found at https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa....)

I also wrote a kernel test to make sure this is fixed. However, the most recent patch (#34) does NOT appear to fix the underlying bug; it does fix it when creating fields in the UI, but not when creating fields programmatically (which is what the test covers, and therefore implicitly covers it for the UI too). So the actual fix might need some reworking. However, now that there is low-level test coverage, I leave that task to the rest of you smart people. :)

Oops, forgot to set this to "needs work" to rework the proposed fix to get the test passing.

Hiding the previous patch so that committers don't accidentally review it.

So I discussed this issue with @larowlan in Slack. We reached the following conclusions:

• The fundamental nature of this bug is that it is possible to create an entity reference field to entity types which don't have an ID. You can do this in the UI, or programmatically. Either way will result in brokenness. That's not good. I'm not sure if Contact is the only example in core, but it should not be allowed for any ID-less entity type.
• The UI and the API should be consistent -- creating an entity reference field to an entity type without an ID should always be disallowed. In the UI, ID-less entity types should not be presented as options when creating an entity reference field. Programmatically, trying to create a field to an ID-less entity type should throw an exception.
• We should fix this in a consistent way, not just for contact messages.

So, with that in mind, I think we need to re-title this issue, rewrite the issue summary, and widen the scope. I think we should still use the code from patch #34 (which I've already added to the issue fork), with a test to ensure that contact messages are not presented as an option in the UI when creating an entity reference field. This is probably no longer novice material, sadly, so I'm removing that tag.

Re-titled again to make it clearer what the actual problem is.

Updated the issue summary to reflect the new understandings and scope.

Write a test to ensure that ID-less entity types are not presented in the UI as options when creating a new entity reference field.

Added the above test case.

Write a test to ensure that we throw an exception when programmatically creating an entity reference field that targets an ID-less entity type.

This seems to be already there in form of Kernel tests.

@phenaproxima, Please confirm if these tests are sufficient. After that I'll remove the "Needs tests" tag.

That looks great, @mohit_aghera! Removing the tag and updating the issue summary.

Regarding this question from #37:

Comment #32 asks for dependency injection. Should it be done here and be the only DI of the whole class, or should it be another issue altogether?

I think adding dependency injection here is out of scope. EntityReferenceItem has at least one other non-static method which uses \Drupal, and I've seen it elsewhere in the field system too. Fixing all that would be nice, but it doesn't affect the actual bug we're trying to squash in this issue. So if we're using \Drupal now -- in code that was previously using \Drupal, to be clear -- then there's no need to add dependency injection now.

Moving to RTBC. It has tests and as stated in comments #36, #37 patch works as intended.
Re-adding evidence.

Error:

Proposed resolution:

I think adding dependency injection here is out of scope. EntityReferenceItem ...

Typed data plugins don't support DI, so this is moot #2053415: Allow typed data plugins to receive injected dependencies

left one minor optimisation/question

I discussed this issue with @webchick to see if she could think of any edge cases or things I might have missed with this fix. The only questions that came up were:

1. How does this surface to end users?
2. What happens if, somehow, you have some invalid configuration somewhere and you try to import it into your site?

This will surface to end users if they try to interact with a reference field that, for some reason, references an entity type without an ID. I could see this happening if, say, they installed the contrib Contact Storage module (which adds an ID field to contact messages), made a reference field to contact messages, and then later uninstalled Contact Storage without removing the field. That would go kaboom, with a fatal error about "cannot call getType() on null", as you see in the issue summary.

In a case like that, this fix will not solve the problem of the invalid field, but it WILL make the error message quite a bit more useful and friendly by actually naming the offending field, and stating the reason why it's hosed.

The same thing will happen if you try to import an invalid field config. I tested this locally by:

1. Creating a legit reference field, targeting nodes.
2. Exporting config.
3. Altering the field config so it would point to contact messages.
4. Running drush si --existing-config.

Without the patch, I got the same terrifying error as in the issue summary. With it, I got the much more readable and helpful exception. (Although, in the interest of full disclosure, the exception was repeated many times. But I don't know if that's something we can address in this issue.)

So although this will not actually fix existing broken configuration, it does improve the status quo somewhat by a) producing a much more useful error, and b) making it a lot harder, if not impossible, to create broken configuration in the UI.

Awesome. That indeed sounds far better than the status quo. And in terms of the overall solution, agreed that filtering these invalid options out of the available ones to choose from seems like a good approach.

However, it seems like @larowlan has his paws all over this one, so not pressing the Big Green Button™ just yet. :) Thanks for looking into my one potential concern, @phenaproxima!

Adding issue credits

Going to merge this locally because the MR is against 9.2, but this needs to go into 9.3 first

Committed cf80690 and pushed to 9.3.x. Thanks!

As there is minimal risk of disruption here and this is a major bug, backported to 9.2.x and 9.1.x

Thanks all 🐛

Nice to see this one fixed! Thanks all.

I had to re-do the commits here.

I used https://git.drupalcode.org/project/drupal/-/merge_requests/614.patch locally to apply the changes.

However, that's not the same as https://git.drupalcode.org/project/drupal/-/merge_requests/614.diff, which has the correct changes.

I've redone using the later

We might have to fix this for DER as well so tagging.