Problem/Motivation

In \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::onDependencyRemoval() we trigger a critical error if all the bundles are removed. This is the incorrect behaviour because this method is called to work out what is going to change in a "dry run" mode so it is logging critical errors for things that have not actually happened.

Proposed resolution

Move the log message to field_field_config_presave().

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#11 2925913-11.patch10.42 KBalexpott
#11 8-11-interdiff.txt781 bytesalexpott
#8 2925913-8.patch10.42 KBalexpott
#4 2-4-interdiff.txt8.88 KBalexpott
#4 2925913-4.patch11.65 KBalexpott
#2 2925913-2.patch2.32 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Active » Needs review
FileSize
2925913-2.patch2.32 KB

Let's see what and if anything breaks.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
FileSize
2925913-4.patch11.65 KB
2-4-interdiff.txt8.88 KB

Fixing a couple of tests and making field_entity_bundle_delete() work the same way - if the entity type that the reference field is referencing has no bundles then the field can't exist because there's nothing to reference. As the old log message used to say - the field is broken. We shouldn't be leaving broken configuration around.

The last submitted patch, 2: 2925913-2.patch, failed testing. View results

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

if the entity type that the reference field is referencing has no bundles then the field can't exist because there's nothing to reference.

As far as I remember from #2412569: Allow setting the auto-create bundle on entity reference fields with multiple target bundles, this is not about the referenced entity having no bundles, it's about the last configured *target bundle* being deleted. It's also what is being explained by this comment that you are removing:

-              // In case we deleted the only target bundle allowed by the field
-              // we have to log a critical message because the field will not
-              // function correctly anymore.

The default behavior of the entity reference field when there are no specific target bundles configured is to reference *all* bundles of the target entity type, that's what we're trying to prevent with that log message.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

@amateescu pointed out that history here is actually in #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Title: EntityReferenceItem logs in onDependencyRemoval() whereas it should let itself be deleted » EntityReferenceItem logs critical errors in onDependencyRemoval()
Issue summary: View changes
FileSize
2925913-8.patch10.42 KB

Slight different approach after reading #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted. This maintains the critical error it just moves it's generation to field_field_config_presave() since at this point we know that the field is being saved with an empty target_bundles setting. @amateescu mentioned that an empty array might mean all bundles - actually it doesn't that's what a NULL means and is part of the change introduced by #1978714: Entity reference doesn't update its field settings when referenced entity bundles are deleted. @amateescu also pointed out my incorrect assumption that removing the bundle meant that there were not more possible bundles. This is not correct there might be its just that the field might not have been configured to use them. The new approach prevents dry run calls to onDependencyRemoval logging incorrect critical errors but still maintains the error is you do end up with an entity reference field that's configured with target_bundles as an empty array and hence can actually reference nothing.

No interdiff because this is a new approach.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
FileSize
8-11-interdiff.txt781 bytes
2925913-11.patch10.42 KB

Fix the comment to make more sense - we've not deleted a bundle in the code that is being referenced.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

That looks much better! Thanks @alexpott :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 34965ce on 8.5.x 
    Issue #2925913 by alexpott, amateescu: EntityReferenceItem logs critical...

  • catch committed 1e568eb on 8.4.x 
    Issue #2925913 by alexpott, amateescu: EntityReferenceItem logs critical...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.