Problem/Motivation

The Media Widget module, which defines media reference field widgets mimicking core's File upload and Link widgets, includes an ugly hack (see here and here) to work around a problem happening when submitting the entity form with an existing media item.

When creating a new media item, the submission will work just fine, since the entity reference field supports automatically saving new referenced entities, however it does not currently support re-saving existing referenced entities. As a consequence, when submitting the entity form, if any media field value was updated (e.g. if the "alt" media field value was changed), there is no way for the save to happen as part of referencing entity save transaction. This in turn means that, if either the referencing entity save or the media entity save fail, an inconsistent state can result.

Proposed resolution

Add support to optionally re-save a referenced entity when saving the referencing entity.

Remaining tasks

  • Validate the proposed solution
  • Write code
  • Perform reviews

User interface changes

None

Introduced terminology

None

API changes

None, only API additions

Data model changes

None

Release notes snippet

TBD

Issue fork drupal-3490274

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

plach created an issue. See original summary.

plach’s picture

Issue summary: View changes
quietone’s picture

Version: 11.1.x-dev » 11.x-dev
plach’s picture

Assigned: Unassigned » plach

plach’s picture

Assigned: plach » Unassigned
Status: Active » Needs review

The MR adds configurable functionality to automatically save referenced entities when the referencing entity is saved. I heavily relied on AI to generate tests, fix test failures, and draft the initial solution. See the MR description for more details.

plach’s picture

Issue summary: View changes
amateescu’s picture

Found an older issue with the same purpose: #3203913: Make EntityReferenceItem autosave and another one that would help greatly: #2862574: Add ability to track an entity object's dirty fields (and see if it has changed)

Started reviewing the current MR and I have an initial architectural question: what's the purpose of making this mechanism optional? Is it only for BC purposes (i.e. we could get into double-save situations in various contrib modules that already handle this themselves)

If that's the only reason, I think the burden of this decision shouldn't be placed on the user (site builder) and it would be worthwhile updating those contrib modules (Inline Entity Form, Entity Reference Revisions, Media Widget) to stop auto-saving existing entities when the core functionality is detected instead.

plach’s picture

[...] another one that would help greatly: #2862574: Add ability to track an entity object's dirty fields (and see if it has changed)

Yep, I have a @todo in the code about that one :)

[...] what's the purpose of making this mechanism optional? Is it only for BC purposes (i.e. we could get into double-save situations in various contrib modules that already handle this themselves)

BC was one aspect, the other main one was performance: depending on the widget you are using, it may or may not make sense to ever re-save the referenced entity, especially until we have proper change-detection in place. Even a simple entity may have several entity references, so I was trying to reduce the performance impact as much as possible.

You definitely have a point about admin UX, though: I was imagining that modules providing widgets able to leverage this logic would automatically switch the save mode when their widgets are activated, however this may be problematic if multiple entity form displays activate widgets with different save mode requirements. Maybe we could do something similar to what @Berdir was suggesting in #3203913: Make EntityReferenceItem autosave but on the field item itself? E.g. a ::setNeedsEntitySave method setting a needsEntitySave property, defaulting to the ::hasNewEntity return value?

amateescu’s picture

This is a popular feature request, I just closed another duplicate: #2657716: Entity reference field to save referenced entities on parent entity presave hook :)

I think that could work, but we should ask @Berdir why it wasn't implemented like that in ERR, maybe there are some hidden gotchas around dealing with entity reference items in a separate request, like a modal or something...

amateescu’s picture

plach’s picture

Maybe we could do something similar to what @Berdir was suggesting in #3203913: Make EntityReferenceItem autosave but on the field item itself? E.g. a ::setNeedsEntitySave method setting a needsEntitySave property, defaulting to the ::hasNewEntity return value?

I posted an implementation of this approach here:

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new2.55 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

plach’s picture

The alternative approach above is being discussed in Slack.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.