Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- @hchonov pointed out in #2860097: Ensure that content translations can be moderated independently that adding
load_latest_revision
to routes is a BC break because it causes behavior to change, which would break modules making different use of forward revisions. - Consequently, #2860097's patch was updated to not do this for Content Translation routes.
- But … #2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option. (committed in November) updated
\Drupal\Core\Entity\EntityResolverManager::setRouteOptions()
to automatically set theload_latest_revision
flag on all_entity_form
routes for revisionable entity types. In other words: that is where the real root cause of @hchonov's concerns lies.
This is a problem, because it makes all _entity_form
routes use this by default, meaning that the behavior is changed by default, and can only be opted out from. Instead, the behavior should remain unchanged by default and should be an opt-in thing.
Proposed resolution
- Do not set the
load_latest_revision
on all_entity_form
routes by default. - Let Content Moderation set the
load_latest_revision
flag on_entity_form
routes. This means only installing Content Moderation introduces this behavior change automatically. (It needs this, and it used to originally live in Content Moderation too, until #2865616 made this the default behavior for all of core.)
Remaining tasks
- Implement the proposed resolution.
- Review.
- Discuss.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#14 | cm-latest_revision_flag_revert-2940899-14.patch | 21.52 KB | plach |
Comments
Comment #2
Wim LeersPlease credit @hchonov and @plach.
Comment #3
Wim LeersComment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #5
plachHere's some initial work
Comment #6
Wim Leerss/translation/form/
Or better yet:
revisionable entity forms
Why
10
? Can't this be0
? If not, let's document why.Comment #8
plachComment #10
plachThis should fix the last test failures by moving unit test coverage to a dedicated test.
Comment #11
Wim LeersThe class is marked
@internal
, great. Can we also setpublic: false
then?So … whenever a
Workflow
entity is updated, we must rebuild routes.First: this should use
::setRebuildNeeded()
AFAICT.Second: why do we even need to do this? Why is the route subscriber alone not enough? I think it's because the route subscriber only sets that
load_latest_revision
flag on entity types for which aWorkflow
entity exists that dictates acontent_moderation
-powered workflow, right? It is not trivial to connect those dots. It'd be good to have a@see \F\Q\C\N::getEntityTypes()
, that'd help connect those dots.Still not accurate, apparently it's only for
moderated revisionable entity forms
?getModeratedEntityTypes()
This is highly unusual. A
::setUp()
with a@covers
. Is this really correct?Nice edge case, I didn't even realize this was possible!
Glad to see this edge case tested.
But I'm not seeing test coverage for an entity form for a non-moderated entity type?
Comment #12
plachThis should address the review above.
Comment #13
Wim LeersWe're testing that they're not affected. In other test cases where we're testing that a case is unaffected, we don't pass that final array, because it is the same. We should do the same here.
Other than that, this looks great.
Comment #14
plachAddressed that, I also realized I was owing you a dots-connecting comment :)
Comment #15
Wim Leers👍
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe patch looks great to me. +1 from me on committing this to 8.6 and 8.5. But why is this prioritized as Critical? Related to the priority question, is it important (with respect to known contrib modules) for this to land before beta, or would between beta and RC be ok? My preference would be before, since it's good to minimize behavior changes between beta and RC, but I'm not currently thinking of a reason that it would be hugely problematic to do it after.
I pinged other committers for their thoughts, and also posted #2865616-92: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option. to get feedback from the people who intentionally introduced the behavior that this is now proposes to partially undo.
Comment #18
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCrediting @hchonov per #2.
Comment #19
Wim LeersBecause without this landing before 8.5.0, we'll have shipped a behavior change and hence a BC break.
Comment #22
catchRight. The bc break is allowable, but given we've decided we don't want to make it, we shouldn't ship it, then change it back again later.
Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!