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
Currently quickedit kicks on pages even when there is a forward revision. When content is updated on the page, the values from the default revision override the forward revision. The mental model that the content displayed on the page is always the content which should be changed is incorrect in this instance.
Screencast: https://www.youtube.com/watch?v=gvnXaT7HCeU
Proposed resolution
Only add quickedit when the content being rendered is the latest revision.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#17 | 2903524-follow-up.patch | 992 bytes | samuel.mortenson |
#7 | 2903524-7.patch | 3.79 KB | timmillwood |
#7 | interdiff-2903524-7.txt | 2.83 KB | timmillwood |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedKicking this off with an approach for fixing this.
It doesn't deny any access to the routes which do the updating, so if someone was determined the content could still be changed, but they'd be modifying something they have access to anyway.
Comment #4
samuel.mortensonComment #5
samuel.mortensonWill
$revision_ids
always have a length of at least one? If not, we should be checking if it's empty.Test coverage looks great! Normally I would include a positive test case in the method but Quick Edit has fairly extensive testing already.
Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe usually write boolean return descriptions like this:
TRUE if the entity is the latest revision, FALSE otherwise.
We call them pending revisions now :)
And the doxygen sentence should start with: Tests that ...
Procedural functions that are @internal should start their name with an underscore.
Can we use
$this->testNode->id()
instead of hardcoding '1'?Also, the assertion is a negative one so it doesn't really mean anything if it's not paired with a positive one.
So we need to somehow display two nodes, using the default revision for the first node and the latest revision for the second one and then we can have both a positive and a negative assertion for 'data-quickedit-entity-id'.
Edit: Now I see this was also mentioned in #5, so feel free to leave it like that if you think it's not that important to have a positive assertion as well :)
Comment #7
timmillwoodThis addresses everything in #6, including a simple solution for #6.5.
Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat looks much better!
Comment #9
yoroy CreditAttribution: yoroy commentedAdded link to the screencast that explains and shows the problem.
Comment #10
yoroy CreditAttribution: yoroy commentedThis does "fix" things by making it impossible to do things. Preventing errors or unexpected results is good. Not letting people do things that in other places (content not under moderation) they can do is taking away control. This is not good. But I think this solution is the right trade-off to make. I'm also weighing here that from what we know from usability testing that quick edit is not very discoverable itself.
Comment #14
catchThis doesn't link back to #2815221: Add ability to use Quick Edit to the latest_revision route, adding it as a related issue.
I can definitely see quickedit working with workspaces (when you're in a workspace we let you edit any content with quickedit and it will be added to the set of content in that workspace, also it will show the draft revision linked to the workspace when viewing content so no mis-match). Locking things down for now though is good until we resolve those issues.
Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #15
Wim LeersA change in behavior using an
@internal
helper method with explicit integration test coverage and a@todo
to improve it in the future.As the module maintainer: thank you, this is perfect :)
Comment #17
samuel.mortensonCan we commit this patch as a quick follow-up? The committed patch does not add the data-quickedit-entity-id attribute to non-latest entities, but always adds the data-quickedit-field-id attribute. This means that JS errors ("Quick Edit could not associate the rendered entity field markup...") will be thrown whenever viewing a default, non-latest entity. I'm posting this here and not in a new issue as it's basically the same code, just added to
quickedit_preprocess_field
as well.Comment #18
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCan we assert
data-quickedit-field-id
is gone in the test case?Comment #19
samuel.mortenson@Sam152 Yes - we should do that. I'll open a new issue today so that the test bot can run.
Comment #20
samuel.mortensonOpened #2910005: JavaScript errors thrown when viewing non-latest default revision of entity to address the bug introduced by this issue.