Problem/Motivation
If you create a pending revision, this is not loaded by EntityConverter
. This is correct sometimes, but not all the time. The default revision makes sense for displaying content but for forms, without loading the latest revision, all the data in a pending revision is lost when the form is saved.
Proposed resolution
Content moderation and workbench moderation both rolled their own solution to this problem, but this a more generic problem to do with pending revisions. That is, this shouldn't be either of these modules concerns. This also has come up in a requirement for quickedit, which has to load the latest revision to update field values on pages displaying a pending revision, #2815221: Add ability to use Quick Edit to the latest_revision route.
Remaining tasks
Fix this problem and give modules utilising pending revisions a much easier time integrating with the rest core.
User interface changes
TBD.
API changes
TBD.
Data model changes
TBD.
Comment | File | Size | Author |
---|---|---|---|
#84 | 2865616-84.patch | 14 KB | Sam152 |
#84 | interdiff.txt | 2.94 KB | Sam152 |
#62 | 2865616-62.patch | 13.79 KB | plach |
#62 | interdiff.txt | 1.38 KB | plach |
#50 | ab-2865616-50.txt | 3.26 KB | plach |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #3
timmillwoodToday I learnt: There are two EntityRevisionConverter classes, one in Workflows, one in ContentModeration.
Don't think we're ready yet to move any of this into
Drupal\Core\ParamConverter
yet.Comment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAt this stage we couldn't without a way of query for latest revision in core, hence the followup to the linked issue.
Comment #6
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRejigging this a little bit and updating the IS. I think this could be framed as a generic entity API issue, because as we all know forward revisions are meant to be part of entity API.
Comment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAnother issue that would be a lot easier with better latest revision handling, but copying in the query for now. Big cleanup in CM + a tool that other modules (quickedit/WBM/workspaces et al) can use without a duplicate implementation or soft dependency on content_moderation. Patch would get even smaller once
getLatestRevision
drops.First patch just adds the feature and fixing the "bug", the second will test as a complete replacement to CM's converter.
Comment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFixing some tests.
Comment #13
dawehnerIt feels like we should have some level of test coverage here :)
Can we add a loadLatestRevision method to Entity storage or
\Drupal\Core\Entity\EntityRepositoryInterface
?Comment #14
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTests incoming! I was curious to see if works as a complete drop-in for what content_moderation has. Based on #8 having the same amount of fails, I think it does.
Re:
loadLatestRevision
, we absolutely need something like that. If you have input, please add it to #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types. I haven't seenEntityRepositoryInterface
suggested yet.Comment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedNW for tests.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdding some tests. Also another version of the patch which should proves content_moderation should no longer be concerned with entity forms and forward revisions.
Comment #17
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis looks awesome! Especially the 'replace-cm-converter' patch. Here are a few points that I found:
As far as I've seen in other core routing files, custom parameters are not prefixed with an underscore, so I think we can drop it as well. Some examples are in
views_ui.routing.yml
(the 'tempstore' stuff) andnode.routing.yml
(the 'with_config_overrides' stuff) :)Although, looking into this made we wonder if this should be a route option instead of a parameter, in which case it should be prefixed with an underscore..
Should we have a different priority for the new event? I don't think that equal priorities is the norm, so we can at least have some minimal difference between them.
These doesn't seem to be used by the new code added in this file.
We need some quotes around the name of the option/parameter.
Comment #18
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the review, notes below.
1. I wasn't sure what the rules were here, updated. Converters, since they are only dealing with individual parameters in a route are only given the parameter as context, so a route option would be tricky. Additionally for an entity form route with multiple entity parameters, we'd force the latest revision to be loaded for each param even if it wasn't the form for the entity in question. I think that behaviour is probably correct, but we should allow people to override it by first checking if someone has possibly specified
"load_latest_revision" => FALSE
.2. Done.
3. Good catch.
4. Done.
Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #20
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'm happy with this patch now. This is the last bit that is contentious that would be good to get some feedback on. If the route is an
entity_test.edit
form, do we load the latest revision of 'node'? It's an edge case, not 100% sure.Comment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedMaybe routing is the more-correct component for this issue?
Comment #22
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #23
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #24
timmillwoodOne thought, this ends up loading the entity twice. The initial load gets passed to the EntityConverter where it's switched for the latest revision. Why don't we just load the latest revision initially rather than loading twice?
Also, two other queries:
Does this need to be a lower weight?
Does this query need accessCheck(FALSE)?
Comment #25
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the review!
0. Good catch, replacing with a version which only loads the entity once.
1. The -150 event sets all the entity type information in the params. -149 runs after and uses that information, so I think the order is correct.
2. Good point! I don't think converters are concerned with access.
Comment #26
timmillwoodAs long as testbot agrees, looks good to me.
Comment #27
dawehnerShould we add this flag just for revisionable entity types?
IMHO We use american english aka. the version with z.
Comment #28
timmillwoodBack to "Needs review" based on #27.
Comment #29
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSo now that we're looking at the entity type anyway, I think it makes sense to only set the flag for params of the type of entity in question.
Fixed :)
Test for a non-revisionable entity type.
Comment #30
timmillwoodLooks good, back to RTBC.
Comment #31
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the reviews!
Comment #32
larowlannit: could use a comma after 'flag' - can be fixed on commit
Do we need a test for a non-revisionable entity? Happy for the answer to be 'no we already have tests for that in {some class}'
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented1. Fixed (https://twitter.com/xjmdrupal/status/922090356077932544).
2.
EntityConverterTest
seems to have this covered pretty well. I wanted to Kernel test this and use real entities, hence the two test classes, but I suppose a Unit test could have been made to work as well. The originalEntityConverterTest
doesn't seem to cover the\Drupal\Core\Entity\EntityRepositoryInterface::getTranslationFromContext
code path (which the kernel test does), so not sure if that's missing coverage, possibly out of scope for this issue?Edit: We'd have lots of implicit coverage for it however.
Comment #34
timmillwoodI agree with #33.2.
All looking good, so back to RTBC.
Comment #35
larowlanFair nuff.
Can we get a change record pls.
Thanks
Comment #36
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdded here: https://www.drupal.org/node/2918286
Comment #37
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #38
plachI was manually testing this because I need it in #2860097: Ensure that content translations can be moderated independently and I was not able to get node edit forms to load the latest revision. The following diff seemed to fix the issue:
Comment #39
tim.plunkettOne key piece of feedback, and some nits.
If this code continues to exist, please add 2 as a 3rd param to explode
Now that I read this more...
It reminded me more and more of
\Drupal\Core\Entity\EntityResolverManager::setParametersFromEntityInformation()
Only to see that it is this subscriber that triggers that code.
Which begs the question, why punt this to another subscriber? Revisionability is a core enough concept that I don't know that it needs to be kept separate.
$storage cannot ever be falsy, it will throw an exception or return a valid storage class.
I realize that this is kept from the original patch, but since you're changing the lines anyway might as well not put it in the conditional.
Also, in what case would !$definition be true?
Nit: following lines of @todo should be indented an additional 2 spaces.
I think this would be best in a protected method that returns the latest revision ID. Then it would be much clearer the contrast between $entity = $storage->loadRevision($value) and $entity = $storage->load($value)
Or it could return the entity itself.
IMO this sort of line should always have a comment asserting that yes, we do want to turn off access checking. Could just be me.
Nit: this and others in this class should be "Tests" not "Test"
Comment #40
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks everyone for reviewing! I'm going to fix the bug and add a test for the feedback in #38 first, then see what #39.2 looks like in a subsequent patch, in case there is a good reason to back-out of that approach.
Comment #41
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #42
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCorrect patch for #40.
Comment #43
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the review @tim.plunkett, implementing #39.
1. Fixed, look like this is in line with the rest of
EntityResolverManager
, so good catch.2. Good point. The control flow of
setParametersFromEntityInformation
was such that it looked a bit messy to put this stuff inside that particular function, but another method running after it I think suited the purpose quite nicely. Updated moved the coverage intoEntityResolverManagerTest
. I think as you point out, setting this flag is very much in the spirit of the existing class, so happy with this.3. Sounds like this is being needlessly defensive. Removed both checks, lets see what the bot says.
4. Done.
5. Done, looks much better.
6. Added.
7. Done.
The interdiff is noisy enough that I think reviewing the patch as a whole is probably easier, but one provided for posterity.
Comment #44
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@plach also pointed out on IRC that it would be good to make sure we aren't introducing any performance regressions. It's possible the linked @todo as well as a static cache for revisions would mitigate any such regressions, but I'm hoping they are minimal enough that the architectural win is worth the penalty.
#2620980: ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load()
#2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types
Comment #46
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest fixes.
Comment #47
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTest fixes.
Edit: Double post, one day I'll learn to use drupal.org...
Comment #48
plachChanges look good to me and work well here, but I'll let Tim put this back to RTBC, if he's happy too :)
Comment #49
plachOk, I performed some quick benchmarking with a DB with roughly 13500 generated nodes (1 revision per node). According to my findings in that case this introduces a ~5% performance regression with cold page/render caches. Details attached.
Comment #50
plachWith warm render caches (and page cache disabled) the difference is of course higher: around ~12% in the same situation. With page cache enabled there's no significant difference obviously.
Comment #51
plachBtw, I agree that probably the tickets mentioned in #44 would alleviate or even cancel the performance penalty introduced here.
Comment #52
dawehnerI'm profiling a similar scenario at the moment to figure out why this causes such a high regression.
Comment #53
dawehnerI can reproduce similar bad numbers: https://blackfire.io/profiles/compare/2950eebf-e4fe-400e-9806-370537ce06...
This is all coming from checking edit access on the link relationships.
Comment #54
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSo why do we even try to do something with link relationships in an entity form? That doesn't make any sense to me..
Comment #55
dawehnerI agree, I would be personally fine with skipping the edit and delete form from within
\Drupal\node\Controller\NodeViewController::view
. I'm not sure why this would be helpful, ever.Comment #56
plachLet's move this back to RTBC, we need to hear from committers about the performance issue anyway.
Comment #58
catchWe should open a follow-up for #53-55, and either fix that before this goes in or decide to eat the temporary regression here, better to avoid the checks altogether than rely on revision caching - still same hit with cold revision cache.
Comment #59
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe need to use the new
latestRevision()
method added to entity query here :)After we fix the point above, this will simply become
return $storage->load(key($entity_revisions));
Also, I tried setting a breakpoint in
\Drupal\node\Controller\NodeViewController::view()
and then loaded an entity form and the breakpoint was never reached, which sounds like the correct behavior, because I don't see any way of reaching that code through an entity form. Which makes me wonder.. how did the profiling from #53 arrived to a code path which was checking access on the link relationships?Comment #60
xjmFor #58 and ##59.
Comment #61
plachI just created #2922570: The node view page triggers a lot of expensive access checks for relationship links to address #58.
@amateescu:
In #49 - #50 I had benchmarked the node view route, because I was seeing the latest revision being loaded in my testing. I don't have access to the profile above, but I suspect @dawehner did the same. No entity form involved here, just edit access checks.
Comment #62
plachAnd this should address #59.
Comment #63
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThen this is the problem that we need to address/answer: why is the latest revision loaded on a regular node view route?
The patch should only do that for entity forms by default.
Comment #64
dawehnerWait, I thought we know the answer for that already. Due to #2922570: The node view page triggers a lot of expensive access checks for relationship links we generate also a link to the edit form, which with this patch triggers a loading to the latest revision, so access checking can be applied.
Comment #65
plach@amateescu:
Any other concern or can we move this back to RTBC?
Comment #66
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNo other concerns from me, but I think #2922570: The node view page triggers a lot of expensive access checks for relationship links needs to get in first, no?
Comment #67
dawehnerWon't we need to fix the performance regression first?
Comment #68
plach#58 does not specify which should be the preferred way, so I'd like to send this back to committers for a final word :)
Comment #69
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@plach, wouldn't that be an infinite loop now that you are a committer as well? :D
Comment #70
plachI'm provisional and I couldn't make decisions on issues I've actively worked on anyway :)
Comment #71
plach@Sam152:
I just realized that, unless I'm missing something, we no longer need
EntityRevisionConverter
(see #2496337-164: [plach] Testing issue). I guess it could be removed altogether unless we have BC concerns, in which case it could be kept as an empty extension ofEntityConverter
.Thoughts?
Comment #72
plach@amateescu @dawehner @catch:
Btw, I did some additional research in #2865616: Add an option for EntityConverter to load the latest entity revision and fix all entity forms to use this option., I'd appreciate your feedback over there :)
Comment #73
dawehner@plach
You just caused an actual infinite redirect loop :P
Comment #74
plachOuch, revoke my d.o driving license :(
#2922570-5: The node view page triggers a lot of expensive access checks for relationship links
Comment #75
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRe: #71: we tested that in #16, so we're on the same wavelength :). Just a matter of scope, given there are BC questions I was going to push that to another issue, but happy to bring it in here too.
Comment #76
plachOk, a follow-up works for me, but can we make sure we create it before getting back to RTBC?
Since CM is beta I'd go with a (deprecated) empty subclass.
Comment #77
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAs per https://www.drupal.org/core/d8-bc-policy:
Of course we can argue such things in the follow up :)
#2924812: Deprecate EntityRevisionConverter in content_moderation.
Comment #78
plachThanks!
Sending back to @catch, please note also #2922570-5: The node view page triggers a lot of expensive access checks for relationship links.
Comment #79
plachComment #80
catchCaught up on this and #2922570: The node view page triggers a lot of expensive access checks for relationship links.
Statically/persistently caching the revision load would help a bit, but I think we're missing an opportunity to avoid loading the revision (as a revision) altogether.
i.e. if the latest revision is also the default revision, we can just use the regular entity load. This will then re-use both the persistent and static cache keyed on entity ID as normal.
With the code as it is currently, we'll end up writing/fetching two cache entries for what is exactly the same entity revision.
Should be able to load the entity via a regular load, query for the latest revision ID as this does. Then loadRevision() if it differs, but just return the entity if they're the same.
edit: this still leaves the entity query on node view pages, it's a shame we have to do that just for an access check on an edit link, but don't see a way around that.
Comment #81
plachGood point, let's do that!
No, not for now. #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types should let us rip out the query though. Would you prefer if we added a @todo mentioning that issue?
Comment #82
plachComment #83
catch@todo sounds good.
Comment #84
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHere is an implementation of the approach in #80.
We also already have a
@todo
pointing to that issue, to replace the method that does the query, so should be all good there.Comment #85
plachLooks good!
Comment #86
catchCommitted d0518c2 and pushed to 8.5.x. Thanks!
Comment #89
Wim Leers@hchonov never reviewed this issue, and in an issue where we're making the logic here compatible with entity translations, he discovered the negative consequences of this patch. See #2860097-123: Ensure that content translations can be moderated independently for a summary.
Comment #90
Wim LeersComment #91
Wim LeersIt's possible that this bit of this issue's CR:
is going to be reverted.
Comment #92
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPer #91, does anyone here object to #2940899: Regression: _entity_form routes should not get load_latest_revision_flag by default: changes default behavior (= regression) while only Content Moderation needs it being committed, which changes that line to only being applied to entity types that are moderated by
content_moderation
module? Leaving it to modules that manage pending revisions their own way to need to explicitly set that flag themselves if that's the behavior they want.