The event "Create content entity" (and similar ones) are used accidentally pretty often and lead to errors. This is because the wording suggests, that this will be triggered when the entity is being created, i.e. written to the database. However, that's not the case as it is already triggered when Node::create() is being called in PHP. That's very early in the process and the entity doesn't really exist at that time. At least not with all it's properties that would be called in an ECA model subsequently.
Let's change the labels for those events in \Drupal\eca_content\Plugin\ECA\Event\ContentEntityEvent::definitions. A proposal by @mherchel was "Programmatically create content entity" and we could add some extra warning to the description.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 2022-11-25_10-21.png | 41.38 KB | jurgenhaas |
| #4 | Example__Accessibility_Basic___Olivero.png | 110.5 KB | mherchel |
Show commandsStart 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:
changes
Comments
Comment #2
mherchelAnother potential part of this would be to relabel the "Insert Content" to "Create Content".
This would make sense from a site-builder perspective, however it may confuse users who are already familiar with ECA.
Comment #3
jurgenhaasDifficult call to make. There is also some users that may be familiar with the Drupal hooks that are behind those events. And they come from core, so they are widely known at least for programmers. Unfortunately, those hooks are called "create", "insert" and "update". The first one is the one that causes problems, and the other two are called after writing the entity to the database, either after initial creation or after an update/edit of the entity.
Maybe @rkoller has some ideas on this one?
Comment #4
mherchelI could see this usability issue happening in many places throughout the UI.
IMO, a great way to handle this would be through tooltips, which webform implements really well.
Comment #5
mxh commentedWe're already in an advanced progress of teaching our clients about the events and when to use them, and they have learned the current names. Switching the names, especially one existing event "Insert content entity" into "Create content entity" would lead to a lot of confusion.
One place we can improve is by adding a description to events, similar what's already existing for actions. In the description then, we can explicitly tell more details about the event.
Comment #6
mxh commentedComment #7
jurgenhaasI'm in favour of #5 and I wonder if something like the following screenshot would be a good approach:
Of course, the wording of the description needs some work, just proposing this as an approach to resolve the issue.
Comment #8
jurgenhaasLooking for feedback on this topic, i.e. if the screenshot from my previous comment would be sufficient for a first next step?
Comment #9
mherchelIMO #7 works, and it does the job... but it's not a standard pattern that we normally see in Drupal.
Comment #10
jurgenhaasYes, while the whole canvas is controlled by the integrated third-party product bpmn.io, there is very little we can do about the UI without getting into a maintenance issue long term when trying to alter their product too much.
Comment #11
mherchelWell in that case, it's certainly an improvement!
That being said, having the message be "Attention: this might not be what you expect", doesn't tell the developer what to expect. Maybe something like, "Creation happens before save", or "Entity values will not be available."
Comment #12
rkolleri agree with #11 "Attention: this might not be what you expect" is not advisable because it potentially causes uncertainty and confusion. How about something like:
Caution: No URL available for entities not written to DB yet.or "created in DB yet" at the end.Comment #13
mherchelIt's not just the URL, it's field values (and probably other data), too.
Comment #14
mherchelWould it be possible to catch output a useful error if a user runs into this?
Something like "XXX cannot happen because the event is an entity create event, which does not have access to fields, urls, and data. Try to use an entity insert event"
Comment #15
jurgenhaasSure thing, the text in the screenshot is just a placeholder so far ;-)
How about
ATTENTION: this event gets triggered very early in the process by Drupal and the entity is not ready for any further processing yet. If you're uncertain, you might rather want to use one of the "Pre-save entity", "Insert entity" or "Update entity" events.ECA tries to catch all exceptions, but it won't output them on screen because they may happen when a user uses the website, not the site builder who should know about it. Therefore, related messages will be in the error log only.
Comment #16
rkollerre #13: oh good point. i've based my suggestion on https://drupal.slack.com/archives/C0287U62CSG/p1669225791335759?thread_t... which @jurgenhaas confirmed back then. but i agree my suggestion was too tied too specifically to my linked question. perhaps something like that?
Caution: Full range of functions only available after entity was inserted to the DB. the sentence is not perfect yet. but that way you would communicate that after the creation not the full range of functionality is available. and by using the term insert you point indirectly to the other available option that would provide the full functionality.Comment #17
mxh commentedAdditional note about my argument from #5: Clients are mostly used to the meaning of the Insert and Update events, but I've seen no one ever using the "Create content entity" event (yet). That leads to the assumption that it's probably fine to change the actual name of that event to something that may reduce this risk of confusion. However the naming of Insert and Update events is already in the head of many.
I think the Drupal ecosystem also makes use of the term "prepopulate", think it's coming from the Entityreference prepopulate project. I estimate that the "Create content entity" event is usually being used to prepopulate some default values to an entity on runtime. So maybe we could rename the Create event to something like "Prepopulate content entity", and its description contains something like "An entity object is being created (instantiated) on runtime, without being saved.".
The Insert and Update events could get a description like "After a new entity got saved (persistently created)" (for Insert) and "After an existing entity got saved (persistently changed)". Just adding some more possible "buzzwords" may help to find the proper event. But I think currently the search tool is not including the description (could be wrong about this though).
Comment #18
jurgenhaasI like the approach in #17 except for the word "Prepopulate" as it feels too specific and not quite what's happening right at that time when the event triggers.
What about the word "Initialize" instead? And all the rest from the above comment?
Comment #19
mxh commentedSure thing, "Initialize" fits into that mechanic perfectly 🙂
Comment #20
rkoller+1 initialize
Comment #21
mherchel+💯 That sounds great IMO
Comment #24
jurgenhaasKudos, everyone, this was a very nice decision-making process!
Now it needs a code review.
Comment #25
mxh commentedNot sure whether the MR needs another merge from 1.1.x branch? It states in #23 that it's currently not mergeable.
Comment #27
jurgenhaasSorry, I had pushed to the wrong branch. Corrected now.
Comment #28
mxh commentedGuess this is good to go. Any actions need to be taken on the bpmn modeller integration?
Comment #30
jurgenhaasGreat, thanks for your review. The bpmn_io modeller does not require any changes, it just gets the extra information automatically.