When applying a recipe, we need to trigger an event pre importing content

@alexpott We don't recreate it: $importer->importContent($content, Existing::Skip);
We are just failing to detect it's existing. That's why the trash context is relevant.

Hi, Gab from Drupalcon Signapore Contribution Day. Looking at this.

TL;DR: I'm with current changes without alteration.

I spent quite a while, this is my understanding:

The original aim of the changes proposed is to allow subscribers to react by altering application states, while not changing the content. This seems a reasonable no-disruptive approach, since any default content provided will be processed as expected by their providers.

Passing $existing
Passing "Existing" is harmless per-se, but it will require splitting the loop in two parts and dispatching this event after the first loop loading the entities. Or similar approaches.
The benefit of having $existing is to let event subscribers react when something exists.

IMO the benefits don't pay back for the work that needs to be done. Subscribers can anyway load the entities by themselves and checking if they exist. Loading entities will be cached so when re-run by the Importer class it will be seamless.

Allowing subscribers to remove items
Subscriber will be able to remove items from the list (let's not give the chance to alter content too 🙏). It won't require many changes and it could be done in a data-integrity safe way.

However if I'm a recipe owner I want to ultimately be responsible for my content. I expect for it to be imported and if there is a failure I'm ultimatelly responsible for fixing it.
If I'm a contrib module owner, and my module weirdly handle content I should be ultimatelly responsible to fix my 💩 (like Trash module is trying to do).

Allowing any subscribers to manipulate the list without notifying the rest of the subscribers - which would require a new event :D - seems dangerous to me.

Conclusion
For this reason I believe the current implementation is the most obvious, covering the majority of usecases and reducing risks of overcomplicating the import flow.

Why not a hook? 📌 [policy, no patch] Encourage hooks over event subscribers Active

@nicxvan I don't know. What's the policy in this scenario?

Moving this back to Needs Work for passing the $existing intent together with $content.

This is not a Drupal CMS stable blocker. It would be very nice to have it, and hopefully it can land in a patch release of 11.1.x as a change in an experimental subsystem, but it does not block a stable release of Drupal CMS. Getting this would, however, allow us to avoid a nasty bug that is relatively edge-casey, so keeping it as a release target in case this lands in a core patch release before Drupal CMS's general launch.

Went ahead and implemented the skip mechanism.

Marking as RTBC. Love to see some more improvement in the default content system!

I would advocate to do that in a follow-up.

Trash could use that to say that an entity was skipped because one with the same UUID is in the trash bin.

Hmmm...yeah, okay @amateescu, you've convinced me that we should support filing a reason why the skip happened (the reason should be optional). Trash's use case makes perfect sense.

And, done.

Looks good to me, thanks @phenaproxima! Left a comment on the MR, feel free to close it if you disagree :)

Discussed #8 with @phenaproxima we agreed that in this situation the event is offering benefits - the event object collects skips and validates them. This type of collecting and validating with an object has not been done with hooks yet and there is no need for this to be first.

Also discussed with @longwave ahile back and we agreed to backport this to 11.1.x because while this is not a bug in core it helps fix a bug possible when you have trash installed - which Drupal CMS does.

Committed and pushed 51ae03891c1 to 11.x and c813f52a5be to 11.1.x. Thanks!

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