Problem/Motivation

\Drupal\Core\DefaultContent\PreImportEvent allows skipping content but nothing else. Having an event fired after $this->toEntity before validation would be great. So ImportEntityEvent? so things can do stuff before validating

Examples:

  • Canvas stores target_id in its component tree, not UUID. So target IDs need to be fixed in dumps if the DefaultContent API's UUID -> ID resolution cannot be fully used
  • Crops store an entity ID in an integer field, rather than an entity reference.

Proposed resolution

Dispatch ImportEntityEvent per-entity once populated and before validation

CommentFileSizeAuthor
#7 3522779-nr-bot_3xffhlv4.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3522779

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:

About issue forks

Comments

mglaman created an issue. See original summary.

mglaman opened merge request !13436

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

I think we can make the metadata read-only and therefore remove some accessors from the event.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs work » Needs review

did the change

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Title: Dispatch ImportEntityEvent during content import » Dispatch an event for manipulating entity data during content import
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3522779-nr-bot_3xffhlv4.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Issue tags: +Vienna2025
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

This looks really good to me, but I suspect we can tighten scope slightly and reduce the size of the change set.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Assigned: Unassigned » mglaman
Status: Needs review » Needs work

I need to fix the phpcs/phpstan

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: mglaman » Unassigned
Status: Needs work » Needs review

I'm happy with this. I ended up removing the unit test because, after the simplifications we made, it's not really testing anything worthwhile. The kernel test remains.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes
thejimbirch’s picture

thejimbirch commented
Status: Needs review » Reviewed & tested by the community

Reading through the tests, When entities are imported, the test subscriber listens for PreEntityImportEvent and modifies the entity data before creation. After import, the test confirms that the entity’s label was changed as expected ("Changed name"), proving event data modification works. All threads resolved. Moving to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review

Fixed with a test (confirmed that it fails without the appropriate fix).

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Nice idea, @alexpott. That's a lot cleaner!

mglaman’s picture

mglaman
Primary language English
Location WI, USA
commented
Status: Needs review » Reviewed & tested by the community

I know I touched the MR, but it's been changed quite a bit by @phenaproxima and looks great, and has addressed @alexpott feedback

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 44799b5173b to 11.x and 74f8df5f8cd to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 74f8df5f on 11.3.x 
    Issue #3522779 by mglaman, phenaproxima, alexpott: Dispatch an event for...

  • alexpott committed 44799b51 on 11.x 
    Issue #3522779 by mglaman, phenaproxima, alexpott: Dispatch an event for...

mglaman closed merge request !13436

Status: Fixed » Closed (fixed)

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