Problem/Motivation
Taxonomy term entities will be revisionable after #2880149: Convert taxonomy terms to be revisionable but it's not possible to revert to a revision or delete a specific revision.
Proposed resolution
Introduce the revisions tab and show the revision fields in the term form, like we have it for node.
Remaining tasks
Add release note snippet
Add change record
Investigate the slowness of the update hook, see #51 - I think this was a one off, disregard.
Review patch in #58 - please disregard prior patches and MRs.
https://git.drupalcode.org/project/drupal/-/merge_requests/5666 is the canonical MR.
User interface changes
Taxonomy terms will have a revision UI similar to BlockContent.
Revisions tab:
Revert form:
Delete form:
Release notes snippet
There is now a UI for viewing, reverting and deleting Taxonomy term revisions.
Comment | File | Size | Author |
---|---|---|---|
#77 | Screenshot 2024-02-06 at 11.54.52 AM.png | 121.32 KB | Sandeep_k |
#77 | Screenshot 2024-02-06 at 11.54.36 AM.png | 115.64 KB | Sandeep_k |
#57 | Screenshot from 2023-10-26 14-50-40.png | 33.9 KB | acbramley |
#57 | Screenshot from 2023-10-26 14-50-35.png | 34.18 KB | acbramley |
#57 | Screenshot from 2023-10-26 14-50-27.png | 36.31 KB | acbramley |
Issue fork drupal-2936995
Show commands
Start 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:
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis is postponed on #2880149: Convert taxonomy terms to be revisionable and #2350939: Implement a generic revision UI.
Comment #5
jibranComment #6
hugronaphor CreditAttribution: hugronaphor as a volunteer and at Acrosto for University of Dundee, manifesto commentedIf anyone needs the UI now, have a look at Taxonomy Revision UI(sandbox)
Comment #14
dpi#2350939: Implement a generic revision UI is merged, unblocking this issue.
Are any maintainers of Taxonomy Revision UI Sandbox interested in working on this?
Checklist available in the revision UI CR: https://www.drupal.org/node/3160443
Comment #15
samit.310@gmail.com CreditAttribution: samit.310@gmail.com commentedHi,
I have submitted the new contrib module Taxonomy Revisions UI.
This module adds "Revisions" tab in the "Edit" section of Taxonomy Term which displays a list of revisions and allows viewing, reverting or deleting revisions.
Note: Requires core patch from issue #3269029.
Please review above module.
Thanks
Samit K.
Comment #16
dpiWe already had a module that does this. Why create a new one? Or collaborate and promote the existing sandbox.
This issue is about integrating work into core. If anything, borrowing the work #1984588: Add Block Content revision UI would be next steps after that is merged.
Comment #17
AaronMcHaleComment #18
hugronaphor CreditAttribution: hugronaphor as a volunteer and at Acrosto for University of Dundee, manifesto commentedMaintainer of Taxonomy Revision UI(sandbox) here.
Don't think any of us can dedicate time to this.
On another side "samit.310@gmail.com" asked me to be the maintainer on which I said that the work should be done here, at witch I didn't get an answer and now we have contrib module which actually has to be implemented here 🤯
Comment #21
viappidu CreditAttribution: viappidu as a volunteer commented@hugronaphor It's a work in progress based on your work and that of @samit which has not sense of being separate.
Still missing work translation revision (and breadcrumb was not even in my mind) but I stopped... I worked on your implementation though I started to think it was not the right approach...
With my code we can select to create a new revision for the term on the edit page BUT (big BUT) I think it should be implemented at vocabulary level a-la-content type to be clear.
Opinions?
Comment #22
AaronMcHale@viappidu All this issue needs to do is implement the new Generic Revision UI introduce in 10.1, Taxonomy does not need to provide the UI from scratch.
Please see the Change Record for instructions on how this should be implemented: Revision UI available to revisionable entities.
Please also see the merge request in #1984588: Add Block Content revision UI for how this is being implemented for the block_content module.
Comment #23
viappidu CreditAttribution: viappidu as a volunteer commented@Aaron, my bad, you are totally right.
I started working on it the proper way...
Comment #25
slydevil CreditAttribution: slydevil commentedRebased for 10.1.x and added this patch.
Comment #27
sumit_saini CreditAttribution: sumit_saini as a volunteer commentedComment #29
mcaddz CreditAttribution: mcaddz at Service NSW commentedAdding patch for 11.x.
Comment #30
yash.rode CreditAttribution: yash.rode at Acquia commentedI think, as we are using generic revision UI, we should postpone this issue on #3323788: Revert and Delete actions should not be available for the current revision. Once the blocker is in,
TermAccessControlHandler
will become simpler.Comment #31
naveenvalecha#3323788: Revert and Delete actions should not be available for the current revision has landed.
Comment #32
yash.rode CreditAttribution: yash.rode at Acquia commentedFixing things based on #3323788: Revert and Delete actions should not be available for the current revision
Comment #33
yash.rode CreditAttribution: yash.rode at Acquia commentedWe don't need to check for the default and latest revision in this issue any more, because, #3323788: Revert and Delete actions should not be available for the current revision does that for us.
Can someone clarify, do we really need
setReason()
calls at the end of every case in\Drupal\taxonomy\TermAccessControlHandler::checkAccess
? Anyway, the messages won't be available for the users (editors) I guess. If we don't need to set those messages, we can simply dowith the conjunction.
Comment #34
yash.rode CreditAttribution: yash.rode at Acquia commentedPlease ignore the (wrong) interdiff in the last comment.
Comment #35
smustgrave CreditAttribution: smustgrave at Mobomo commentedPersonally I find the reason helpful when debugging in the logs, but that's just me
But testing #33 on 10.1.x standard install
Created a taxonomy term (kept forgetting to check "new revision") but made a few revisions
Verified the UI appeared just like nodes
Verified all my revisions are there.
Verified I could revert back to previous ones.
Looks good!
Comment #36
lauriiiShould we add a setting to the entity type that allows configuring if a new revision should be created like we do on other reivisionable entity types?
Missing a docblock
Comment #37
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedMy kneejerk reaction was no, but I think you're right. Vocabulary also needs to implement RevisionableEntityBundleInterface and we should probably add a moderation handler as well?
Here's an issue where we added this for microcontent #3261439: Add new_revision config and implement RevisionableEntityBundleInterface
Comment #38
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedComment #39
yash.rode CreditAttribution: yash.rode at Acquia commentedTrying to implement #36(config to create a new revision)
Comment #40
yash.rode CreditAttribution: yash.rode at Acquia commentedWe already have a create new revision option available, if I am not understanding the comment wrong.
Comment #41
mcaddz CreditAttribution: mcaddz at Service NSW commentedMy understanding is we should probably have a config setting that allows the creation of new revisions by default so the user doesn't need to check the checkbox each time.
This patch should demonstrate which borrows from #3261439.
Includes a moderation control handler that forces revisions if content moderation is used.
Comment #42
sokru CreditAttribution: sokru as a volunteer commentedIncluded the missing docblock mentioned on #36.
Comment #44
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedTaking a look at the test failures.
Comment #45
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedShould fix up all the failures
Comment #46
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedComment #47
yash.rode CreditAttribution: yash.rode at Acquia commentedCan someone confirm second part of #33?? And if yes, let's simplify this, otherwise we should add
setReason()
in other AccessControlHandler eg.BlockContentAccessControlHandler.php
to make it consistent throughout.Comment #48
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented@yash.rode as per #35 I think it's helpful and not doing any harm. Feel free to open up another issue to make it consistent in BlockContent.
Comment #49
smustgrave CreditAttribution: smustgrave at Mobomo commentedRetesting with previous steps
Created a taxonomy term (kept forgetting to check "new revision") but made a few revisions
Verified the UI appeared just like nodes
Verified all my revisions are there.
Verified I could revert back to previous ones.
And still working!
Was going to tag for follow up but will let you @yash.rode decide if you want to follow up on the setReason()
Comment #50
yash.rode CreditAttribution: yash.rode at Acquia commentedCreated #3386379: Add setReason() calls for debugging and consistency
Comment #51
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFor such a simple update hook, this took a very long time to run on our deployment, really not sure how it could take longer than processing thousands of entities in another one of our update hooks 🤔
Comment #53
smustgrave CreditAttribution: smustgrave at Mobomo commentedTests are still green.
Comment #54
quietone CreditAttribution: quietone at PreviousNext commentedI'm triaging RTBC issues.
Thanks for getting this 6 year old issue to RTBC.
I read the IS and the comments. I didn't find any unanswered questions. However, in #51 @acbramley points out that the update hook took a long time but there has been no investigation of that.
This is changing the UI so adding usability tag. There should be screenshots, available from the Issue Summary, here to show the change as well.
Like the related issue to #1984588: Add Block Content revision UI, this should have a Change record, with screenshots to help the user, and a release not snippet. Using the existing the change record and release note snippet as a basis should help. Adding tag.
There are multiple branches here, an MR and a patch. I think during this transition time we should indicate in the Remaining Tasks which of those the reviewer should look at. By reading the comments I see that it is the patch in #45 that was RTBCed. That no longer applies, so I adding tag.
So, a few more things to do here.
Comment #55
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedTackling all that.
Comment #56
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedRerolled and fixed CCF in #45
Comment #57
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedScreenshots.
Comment #58
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedComment #59
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedCR drafted, rerolled properly for 11.x with no fuzz.
Comment #60
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedComment #61
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedAll done, let me know if I missed anything @quietone
Comment #63
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedLooks like a random fail
Comment #64
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedComment #65
smustgrave CreditAttribution: smustgrave at Mobomo commentedCR reads well.
#59 applies cleanly to 11.x
Tested by creating a taxonomy term.
Edited that term and created a new revision
Verified Revisions tab
Verified both revisions are there
Verified reverting to revision 1 worked.
Think this is good to go!
Comment #66
sokru CreditAttribution: sokru as a volunteer commentedDid not date to set this to "Needs work", but heavy note that Block/Media/Taxonomy revision UI's miss one feature present on node revision UI: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/node/... to let user decide what to do with untranslatable fields when reverting.
Comment #67
quietone CreditAttribution: quietone at PreviousNext commentedThis needs a rebase, the diff does not apply to 11.x.
And answering the question in #66 would be helpful.
Comment #69
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedPatch in #59 still applies cleanly. However, I've opened yet another branch and MR to hopefully solve the confusion.
Re #66 - please open a new issue for this.
Comment #74
quietone CreditAttribution: quietone at PreviousNext commented@acbramley, thanks for making it so others won't have the same confusion!
Adding needs followup tag for #66 per #69.
Comment #75
longwaveAdded some nits to the MR.
Comment #76
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFeedback addressed, thanks for the thorough review @longwave
Created follow-up in #3419344: Add translation revert functionality for generic revision UI
Comment #77
Sandeep_k CreditAttribution: Sandeep_k at QED42 for Drupal India Association commentedHi, I've tested MR- MR !5666 mergeable on Drupal version- 11.0-dev. The patch was applied cleanly.
Testing Steps:
Testing Result- After applying the patch, The revisions are getting logged for the changes and working fine.
Comment #78
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears all feedback has been addressed.
Comment #79
longwaveRe-reviewed the whole patch, no further comments, this looks great - thanks to everyone who worked on this feature.
Tagging as a release highlight for 10.3.0 and will publish the change record.
Committed 4a0bdc3 and pushed to 11.x. Thanks!
Comment #83
codebymikey CreditAttribution: codebymikey at Zodiac Media commentedI applied this patch on 10.x, and upon viewing revisions of existing taxonomy term, it crashes with the following:
And not sure if an update hook is necessary to generate the default
getRevisionCreationTime()
value, so that it no longer resolves toNULL
.I'm aware this was mainly for 11.x, just thought I'd document in case an additional upgrade path is necessary.
Comment #84
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented@codebymikey would you mind providing some steps to reproduce that issue? I will take a look today.
Comment #85
codebymikey CreditAttribution: codebymikey at Zodiac Media commented@acbramley, I believe this might be related to an existing issue #3317361: New revision fields don't have a default value after making an entity revisionable, additional documentation on this is available here.
And because the site I ran this particular update on is was created a fairly long time ago, probably before taxonomy terms were even revisionable (<8.8.0), so it was missing the relevant update hook which ensures that the relevant revision fields are prepopulated as part of the revisions update.
So in terms of recreation, I'd say the site needs to have taxonomies created from taxonomies were revisionable. Update to the latest version of Drupal, then have this update applied on top of it.
So it's probably a bit of an edge case, but probably worth being aware of nonetheless since it'll affect any future revisionable conversions - the upgrade solution should ideally be available in core.
edit: was able to work around with the following post update hook:
I tried to make it generic enough so it can be easily applied to other content entity types.
Comment #86
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented@codebymikey so you went from <8.8 to 10.3 in a single update?
Comment #87
rwanthIs there a reason the UI for revision log messages weren't included in the recent commit?
In Term.php, line 213
However, deleting this doesn't expose revision logs to the UI. This appears to be because entity-moderation-form.html.twig is attempting to render form.revision_log (note the lack of '_message'.)
Comparing the annotations of Term with Node, revision_metadata_keys has "revision_log_message" = "revision_log" in Node, as opposed to "revision_log_message" = "revision_log_message" in Term. If we change Term to match, then the revision log message form is displayed.
I'll make a new merge request with this change.
Comment #89
longwave@rwanth thanks for spotting that but please open a new issue to discuss and fix the revision log message - convention is to open followups instead of reopening closed issues.
Comment #90
rwanthThanks for letting me know, and apologies for the confusion. Opened new issue: #3432746: Taxonomy revision UI is missing log messages
Comment #92
codebymikey CreditAttribution: codebymikey at Zodiac Media commented@acbramley sorry about the delay responding to this
No, I just meant that the original installation of the site started before 8.8, so the update hook that was ran at the time that Taxonomy terms were made revisionable didn't do any of content population stuff, and is why this bug probably occurs for it.