Add taxonomy term revision UI

Adding patch for 11.x.

• Adds new 'view all taxonomy revisions' permission.
• Handles 'revert' operation in access control handler.
• De-pluralises existing permissions that didn't sound right.

I think, as we are using generic revision UI, we should block this issue on 🐛 Revert and Delete actions should not be available for the current revision Fixed . Once the blocker is in,TermAccessControlHandler will become simpler.

🐛 Revert and Delete actions should not be available for the current revision Fixed has been committed.

Fixing things based on 🐛 Revert and Delete actions should not be available for the current revision Fixed

We don't need to check for the default and latest revision in this issue any more, because, 🐛 Revert and Delete actions should not be available for the current revision Fixed 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 do

return AccessResult::allowedIfHasPermission();

with the conjunction.

Please ignore the (wrong) interdiff in the last comment.

Personally 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!

1. +++ b/core/modules/taxonomy/src/Entity/Term.php
   @@ -40,6 +45,7 @@
   + *   show_revision_ui = TRUE,
   

   Should 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?

2. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -5,9 +5,30 @@
   +
   +function taxonomy_update_10100(&$sandbox = NULL): TranslatableMarkup {
   

   Missing a docblock

Should 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?

My 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 →

Trying to implement #36 📌 Add taxonomy term revision UI RTBC (config to create a new revision)

We already have a create new revision option available, if I am not understanding the comment wrong.

My 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.

Included the missing docblock mentioned on #36.

The last submitted patch, 42: add-taxonomy-revision-ui-2936995-42.patch, failed testing. View results →

Taking a look at the test failures.

Should fix up all the failures

Can someone confirm second part of #33 📌 Add taxonomy term revision UI RTBC ?? And if yes, let's simplify this, otherwise we should add setReason() in other AccessControlHandler eg. BlockContentAccessControlHandler.php to make it consistent throughout.

@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.

Retesting 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()

Created 📌 Add setReason() calls for debugging and consistency Postponed

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -19,3 +21,13 @@ function taxonomy_removed_post_updates() {
+function taxonomy_post_update_set_new_revision(&$sandbox = NULL) {

For 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 🤔

The last submitted patch, 45: add-taxonomy-revision-ui-2936995-45.patch, failed testing. View results →

Tests are still green.

I'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 ✨ Add Block Content revision UI Fixed , 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.

Tackling all that.

Rerolled and fixed CCF in #45

Screenshots.

CR drafted, rerolled properly for 11.x with no fuzz.

All done, let me know if I missed anything @quietone

The last submitted patch, 59: add-taxonomy-revision-ui-2936995-58.patch, failed testing. View results →

Looks like a random fail

CR 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!

Did 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.

This needs a rebase, the diff does not apply to 11.x.

And answering the question in #66 would be helpful.

Patch 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.

acbramley → changed the visibility of the branch 2936995-add-taxonomy-term to hidden.

acbramley → changed the visibility of the branch 11.x to hidden.

acbramley → changed the visibility of the branch 2936995-taxonomy-revision-ui to hidden.

acbramley → changed the visibility of the branch 2936995-11.x to hidden.

@acbramley, thanks for making it so others won't have the same confusion!

Adding needs followup tag for #66 per #69.

Added some nits to the MR.

Feedback addressed, thanks for the thorough review @longwave

Created follow-up in ✨ Add translation revert functionality for generic revision UI Active

Hi, I've tested MR- MR !5666 mergeable on Drupal version- 11.0-dev. The patch was applied cleanly.

Testing Steps:

• Download the patch & apply it on drupal-11
• Go to Structure> Create new taxonomy term & verify

Testing Result- After applying the patch, The revisions are getting logged for the changes and working fine.

• Tested the reverting functionality- Working for me.
• Tested Delete Revision- Working for me.
• Tested Edit term- While Create Revision is unchecked- No revision is created. RTBC++

Appears all feedback has been addressed.

Re-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!

longwave → closed merge request !5666

Automatically closed - issue fixed for 2 weeks with no activity.

I applied this patch on 10.x, and upon viewing revisions of existing taxonomy term, it crashes with the following:

The website encountered an unexpected error. Please try again later.

InvalidArgumentException: The timestamp must be numeric. in Drupal\Component\Datetime\DateTimePlus::createFromTimestamp() (line 201 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).
Drupal\Core\Datetime\DateFormatter->format(NULL, 'short', '') (Line: 148)
Drupal\Core\Entity\Controller\VersionHistoryController->getRevisionDescription(Object) (Line: 279)
Drupal\Core\Entity\Controller\VersionHistoryController->buildRow(Object) (Line: 252)
Drupal\Core\Entity\Controller\VersionHistoryController->revisionOverview(Object) (Line: 76)
Drupal\Core\Entity\Controller\VersionHistoryController->__invoke(Object)
call_user_func_array(Object, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 592)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Object, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

And not sure if an update hook is necessary to generate the default getRevisionCreationTime() value, so that it no longer resolves to NULL.

I'm aware this was mainly for 11.x, just thought I'd document in case an additional upgrade path is necessary.

@codebymikey would you mind providing some steps to reproduce that issue? I will take a look today.

@acbramley, I believe this might be related to an existing issue 🐛 New revision fields don't have a default value after making an entity revisionable Active , additional documentation on this is available here → .

And because the site I ran this particular update on is fairly old, it was 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.

@codebymikey so you went from <8.8 to 10.3 in a single update?

Is there a reason the UI for revision log messages weren't included in the recent commit?

In Term.php, line 213

    // @todo Keep this field hidden until we have a revision UI for terms.
    // @see https://www.drupal.org/project/drupal/issues/2936995
    $fields['revision_log_message']->setDisplayOptions('form', [
      'region' => 'hidden',
    ]);

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.

@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.

Thanks for letting me know, and apologies for the confusion. Opened new issue: 📌 Taxonomy revision UI is missing log messages Active

rwanth → changed the visibility of the branch 2936995- to hidden.

@acbramley sorry about the delay responding to this

@codebymikey so you went from <8.8 to 10.3 in a single update?

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.

I'm having the same issue with taxonomy revisions throwing "InvalidArgumentException: The timestamp must be numeric. ".
How to reproduce:
- Upgraded 10.2 to 10.3,
- Taxonomy terms have "Revisions" local task tab

- Navigating to "Revisions" tab throws an exception.

- after editing a term and creating a new revision "Revisions" tab still crashes

this happens on multiple instances of 10.2 to 10.3 sites, these sites are stock standard Drupal installation without any special bells nor whistles

I've created 🐛 Protect against empty revision timestamps in VersionHistoryController::getRevisionDescription Active