Add taxonomy term revision UI

If anyone needs the UI now, have a look at Taxonomy Revision UI(sandbox)

#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

Hi,
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.

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

Maintainer 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 🤯

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

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

@Aaron, my bad, you are totally right.
I started working on it the proper way...

Rebased for 10.1.x and added this patch.

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

#3323788: Revert and Delete actions should not be available for the current revision has landed.

Fixing things based on #3323788: Revert and Delete actions should not be available for the current revision

We 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 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(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.

Taking a look at the test failures.

Should fix up all the failures

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

@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 #3386379: Add setReason() calls for debugging and consistency

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

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

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

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, 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 #3419344: Add translation revert functionality for generic revision UI

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!

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 #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:

/**
 * Set initial values for new revision fields.
 */
function taxonomy_post_update_set_initial_revision_field_values(&$sandbox) {
  $connection = \Drupal::database();

  /** @var \Drupal\Core\Entity\ContentEntityTypeInterface $entity_type */
  $entity_type = \Drupal::entityTypeManager()->getDefinition('taxonomy_term');
  $data_table = $entity_type->getDataTable();
  $revision_table = $entity_type->getRevisionTable();

  if ($entity_type->hasKey('owner') && $entity_type->hasRevisionMetadataKey('revision_user')) {
    $fields[$entity_type->getKey('owner')] = $entity_type->getRevisionMetadataKey('revision_user');
  }
  if ($entity_type->hasRevisionMetadataKey('revision_created')) {
    $field_definitions = \Drupal::service('entity_field.manager')->getBaseFieldDefinitions($entity_type->id());
    if (!empty($field_definitions['changed'])) {
      $fields['changed'] = $entity_type->getRevisionMetadataKey('revision_created');
    }
    elseif (!empty($field_definitions['created'])) {
      $fields['created'] = $entity_type->getRevisionMetadataKey('revision_created');
    }
  }

  if (empty($fields)) {
    return 'There were no revision fields to update';
  }

  $id_field = $entity_type->getKey('id');
  $revision_field = $entity_type->getKey('revision');
  $langcode_field = $entity_type->getKey('langcode');

  $results = [];
  foreach ($fields as $existing_field_name => $new_field_name) {
    $revision_field_name = "[$revision_table].[$new_field_name]";
    $subquery = $connection->select($data_table, 'base_table')
      ->fields('base_table', [$existing_field_name])
      ->where("[$revision_table].[$id_field] = [base_table].[$id_field]")
      ->where("[$revision_table].[$revision_field] = base_table.[$revision_field]")
      ->where("[$revision_table].[$langcode_field] = [base_table].[$langcode_field]");

    $update_query = $connection->update($revision_table);
    $updated = $update_query
      ->expression($revision_field_name, $subquery)
      ->isNull($revision_field_name)
      ->execute();
    $results[] = "Updated $updated `$new_field_name` field values(s).";
  }

  return implode(' ', $results);
}

I tried to make it generic enough so it can be easily applied to other content entity types.

@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: #3432746: Taxonomy revision UI is missing log messages

@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 #3459169: Protect against empty revision timestamps in VersionHistoryController::getRevisionDescription

Hello,

Thank you for the work done!

I notice that the "Revisions" link in the primary local tasks is not positioned in the same place as on the node entity type:.

Indeed, an entry is missing in taxonomy.links.tasks.yml.
Here is the one present in the node entity type:

entity.node.version_history:
  route_name: entity.node.version_history
  base_route: entity.node.canonical
  title: 'Revisions'
  weight: 20

Same thing with "version_history" routing.

What do you think?

Today, I use this workaround :

/**
 * Implements hook_local_tasks_alter().
 */
function mymodule_local_tasks_alter(array &$local_tasks): void {
  $name = 'entity.version_history:taxonomy_term.version_history';

  if (isset($local_tasks[$name])) {
    // Move the local task at the end.
    $local_task = $local_tasks[$name];
    unset($local_tasks[$name]);
    $local_tasks[$name] = $local_task;
  }
}

(Change "weight" local task property in alter don't seems to work).