- First commit to issue fork.
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - First commit to issue fork.
- 🇦🇺Australia mcaddz
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.
- last update
over 1 year ago 30,047 pass - 🇮🇳India yash.rode pune
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. - 🇮🇳India naveenvalecha New Delhi
🐛 Revert and Delete actions should not be available for the current revision Fixed has been committed.
- Assigned to yash.rode
- 🇮🇳India yash.rode pune
Fixing things based on 🐛 Revert and Delete actions should not be available for the current revision Fixed
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 12:03pm 25 August 2023 - last update
about 1 year ago 30,061 pass - 🇮🇳India yash.rode pune
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 needsetReason()
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 doreturn AccessResult::allowedIfHasPermission();
with the conjunction.
- 🇮🇳India yash.rode pune
Please ignore the (wrong) interdiff in the last comment.
- Status changed to RTBC
about 1 year ago 3:03pm 30 August 2023 - 🇺🇸United States smustgrave
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!
- last update
about 1 year ago 30,101 pass - Status changed to Needs work
about 1 year ago 12:45pm 1 September 2023 - 🇫🇮Finland lauriii Finland
-
+++ 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?
-
+++ b/core/modules/taxonomy/taxonomy.install @@ -5,9 +5,30 @@ + +function taxonomy_update_10100(&$sandbox = NULL): TranslatableMarkup {
Missing a docblock
-
- 🇦🇺Australia acbramley
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 →
- Assigned to yash.rode
- 🇮🇳India yash.rode pune
Trying to implement #36 📌 Add taxonomy term revision UI RTBC (config to create a new revision)
- Issue was unassigned.
- Status changed to Needs review
about 1 year ago 10:17am 5 September 2023 - 🇮🇳India yash.rode pune
We already have a create new revision option available, if I am not understanding the comment wrong.
- last update
about 1 year ago Patch Failed to Apply - 🇦🇺Australia mcaddz
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.
7:01 33:28 RunningThe last submitted patch, 42: add-taxonomy-revision-ui-2936995-42.patch, failed testing. View results →
- Assigned to acbramley
- Status changed to Needs work
about 1 year ago 10:26pm 5 September 2023 - Status changed to Needs review
about 1 year ago 11:08pm 5 September 2023 - last update
about 1 year ago 30,140 pass - Issue was unassigned.
- 🇮🇳India yash.rode pune
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. - Status changed to RTBC
about 1 year ago 5:37pm 8 September 2023 - 🇺🇸United States smustgrave
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()
- last update
about 1 year ago 30,150 pass - last update
about 1 year ago 30,150 pass - 🇮🇳India yash.rode pune
Created 📌 Add setReason() calls for debugging and consistency Postponed
- last update
about 1 year ago 30,152 pass - 🇦🇺Australia acbramley
+++ 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 🤔
- last update
about 1 year ago 30,158 pass - last update
about 1 year ago 30,165 pass - last update
about 1 year ago 30,172 pass - last update
about 1 year ago 30,172 pass - last update
about 1 year ago 30,209 pass - last update
about 1 year ago 30,209 pass - last update
about 1 year ago 30,364 pass - last update
about 1 year ago 30,361 pass, 2 fail The last submitted patch, 45: add-taxonomy-revision-ui-2936995-45.patch, failed testing. View results →
- last update
about 1 year ago 30,365 pass - last update
about 1 year ago 30,364 pass - last update
about 1 year ago 30,375 pass - last update
about 1 year ago 30,381 pass - last update
about 1 year ago 30,381 pass - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - last update
about 1 year ago Custom Commands Failed - Status changed to Needs work
about 1 year ago 3:41am 24 October 2023 - 🇳🇿New Zealand quietone
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.
- Assigned to acbramley
- Status changed to Needs review
about 1 year ago 3:54am 26 October 2023 - last update
about 1 year ago Patch Failed to Apply - last update
about 1 year ago 30,437 pass, 2 fail - 🇦🇺Australia acbramley
CR drafted, rerolled properly for 11.x with no fuzz.
- Issue was unassigned.
- 🇦🇺Australia acbramley
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 →
- last update
about 1 year ago 30,442 pass - Status changed to RTBC
about 1 year ago 3:18pm 27 October 2023 - 🇺🇸United States smustgrave
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!
- last update
about 1 year ago 30,460 pass - last update
about 1 year ago 30,485 pass - last update
about 1 year ago 30,487 pass - last update
about 1 year ago 30,490 pass - last update
about 1 year ago 30,490 pass - 🇫🇮Finland sokru
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.
- last update
about 1 year ago 30,497 pass - last update
about 1 year ago 30,516 pass - last update
about 1 year ago 30,523 pass - last update
about 1 year ago 30,534 pass - last update
about 1 year ago 30,558 pass - last update
about 1 year ago 30,606 pass 42:12 38:48 Running- last update
12 months ago 30,609 pass - last update
12 months ago 30,671 pass - last update
12 months ago 30,679 pass - last update
12 months ago 30,683 pass - last update
12 months ago 30,688 pass - last update
12 months ago 30,692 pass - Status changed to Needs work
12 months ago 4:32am 4 December 2023 - 🇳🇿New Zealand quietone
This needs a rebase, the diff does not apply to 11.x.
And answering the question in #66 would be helpful.
- Status changed to RTBC
12 months ago 4:58am 4 December 2023 - 🇦🇺Australia acbramley
- 🇦🇺Australia acbramley
acbramley → changed the visibility of the branch 2936995-add-taxonomy-term to hidden.
- 🇦🇺Australia acbramley
acbramley → changed the visibility of the branch 11.x to hidden.
- 🇦🇺Australia acbramley
acbramley → changed the visibility of the branch 2936995-taxonomy-revision-ui to hidden.
- 🇦🇺Australia acbramley
acbramley → changed the visibility of the branch 2936995-11.x to hidden.
- 🇳🇿New Zealand quietone
@acbramley, thanks for making it so others won't have the same confusion!
Adding needs followup tag for #66 per #69.
- Status changed to Needs work
10 months ago 4:48pm 5 February 2024 - Status changed to Needs review
10 months ago 10:38pm 5 February 2024 - 🇦🇺Australia acbramley
Feedback addressed, thanks for the thorough review @longwave
Created follow-up in ✨ Add translation revert functionality for generic revision UI Active
- 🇮🇳India Sandeep_k New Delhi
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++
- Status changed to RTBC
10 months ago 3:19pm 9 February 2024 - Status changed to Fixed
10 months ago 6:03pm 9 February 2024 -
longwave →
committed 4a0bdc34 on 11.x
Issue #2936995 by viappidu, mcaddz, acbramley, slydevil, yash.rode,...
-
longwave →
committed 4a0bdc34 on 11.x
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 toNULL
.I'm aware this was mainly for 11.x, just thought I'd document in case an additional upgrade path is necessary.
- 🇦🇺Australia acbramley
@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.
- 🇦🇺Australia acbramley
@codebymikey so you went from <8.8 to 10.3 in a single update?
- 🇺🇸United States rwanth New York, USA
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.
- Merge request !7118Modify revision_log_message key/value pair to expose log message UI → (Open) created by rwanth
- 🇬🇧United Kingdom longwave UK
@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.
- 🇺🇸United States rwanth New York, USA
Thanks for letting me know, and apologies for the confusion. Opened new issue: 📌 Taxonomy revision UI is missing log messages Active
@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.
- 🇦🇺Australia jannakha Brisbane!
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 crashesthis happens on multiple instances of 10.2 to 10.3 sites, these sites are stock standard Drupal installation without any special bells nor whistles
- 🇦🇺Australia acbramley
- 🇫🇷France S3b0uN3t Nantes
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
What do you think?