Convert taxonomy terms to be revisionable

We can close some old feature requests to convert terms to have a status field.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -0,0 +1,114 @@
+    $status->setInitialValueFromField('content_translation_status');
...
+    $status->setInitialValue(1);

We already have status column in {taxonomy_index} table which tracks the indexed node status. Maybe we should start tracking the term status in {taxonomy_index} table as well. Something like:

diff --git a/core/modules/taxonomy/taxonomy.module b/core/modules/taxonomy/taxonomy.module
index 4a93989ad5..146deadc83 100644
--- a/core/modules/taxonomy/taxonomy.module
+++ b/core/modules/taxonomy/taxonomy.module
@@ -528,9 +528,14 @@ function taxonomy_build_node_index($node) {
     }
     // Insert index entries for all the node's terms.
     if (!empty($tid_all)) {
-      foreach ($tid_all as $tid) {
-        db_merge('taxonomy_index')
-          ->key(['nid' => $node->id(), 'tid' => $tid, 'status' => $node->isPublished()])
+      $terms = Term::loadMultiple($tid_all);
+      foreach ($terms as $term) {
+        \Drupal::database()->merge('taxonomy_index')
+          ->keys([
+            'nid' => $node->id(),
+            'tid' => $term->id(),
+            'status' => $node->isPublished() && $term->isPublished(),
+          ])
           ->fields(['sticky' => $sticky, 'created' => $node->getCreatedTime()])
           ->execute();
       }

This feels like a followup issue but it is important to mention here that it with \Drupal\taxonomy\Plugin\EntityReferenceSelection\TermSelection::getReferenceableEntities() not taking into account the term status this would become a potential security issue of information disclouser which obviously doesn't exist in HEAD right now.

not taking into account the term status this would become a potential security issue of information disclouser which obviously doesn't exist in HEAD right now.

Its a bit tricky. One could argue that the taxonomy status doesn't have yet a concrete meaning, given there is no UI support for it yet. Once we have a UI to actually unpublish a term, we need to check access, as you said, on entity queries as well as views queries, if possible.

Looks like this just depends on one issue now, #2860654: Field storage CRUD operations must use the last installed entity type and field storage definitions, which is RTBC.
If we can get this ready maybe it can get in 8.4.0 too?

Here's a reroll of #2.

Last dependency is in.

I'm a bit confused why this issue doesn't add tests which ensure that for example saving revisions actually works. I just see an update path test.

Maybe we can create an EditorialContentEntityBase test with a @dataProvider which loops through all entity types that extend it and tests saving revisions and publishing.

That sounds like a good idea. I think it could be a kernel test maybe. On the other hand the existing entity tests kind of cover many of the details, so maybe this test should really just test some basic functionality.

Here's an idea, let's get Content Moderation to test it for us. I started creating an EditorialContentEntityBase test, but it ended up pretty similar to this content moderation one.

This makes sense I guess! Thank you @timmillwood for adding it.

Ok, once we get @amateescu's agreement on #15 we can RTBC.

Awesome!

Change record please. :) Other than that, this seems great in principle and fits with the consensus in #2745619: [policy, no patch] Which core entities get revisions?.

Edit: Also, you can call this subsystem maintainer signoff. :)

+++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
@@ -86,6 +86,9 @@ protected function getExpectedNormalizedEntity() {
+      'revision_id' => [
+        ['value' => 1],
+      ],

@@ -129,6 +132,16 @@ protected function getExpectedNormalizedEntity() {
+      'status' => [
+        [
+          'value' => TRUE,
+        ]
+      ],
+      'revision_created' => [
+        $this->formatExpectedTimestampItemValues((int) $this->entity->getRevisionCreationTime()),
+      ],
+      'revision_user' => [],
+      'revision_log_message' => [],

See #2880154-11: Convert comments to be revisionable.

This is technically a BC break for API-First… I'll let core committers decide here.

Making a start on the constraint for taxonomy hierarchy. This currently just checks changes to the parent on the node form, I guess it should check weight too, or should we make weight revisionable?

@todo:
- What do we do about weight and parent changes on the overview page? This will change the default revision, so maybe that's ok?
- Add tests.

Opps, deleted the tags.
On that note too, more discussion on BC in #2880154-16: Convert comments to be revisionable

#23 won't work with multiple parents, this will.

On the overview page making any change will update one or more taxonomy terms default revision. With Content Moderation enabled a new default revision will be created. If there are pending revisions the new default will be created ahead of these. Therefore they will be lost.

We should add validation to the overview page, either in taxonomy, or via a form alter in content moderation. If any of the entities being moved have pending revisions and error will be thrown.

Or we should prevent terms with pending revisions being moved at all, because if there are hundreds of terms a user's has moved, there work would be lost and would need to be done again once pending revisions have been published.

When playing with the overview page I found a bug with #29. Steps to reproduce:

1. Add a new term, don't set a parent, save as published.
2. Edit term, don't change parent, save as draft.
3. Error will be thrown.

This is because we are comparing $original_parents = [] with $new_parents = [0 => 0].

Another interesting bug I've seen is terms losing as the parent. If a term has two parents, plus another term, when loading the edit form will not be selected, therefore saving again will remove as the parent.

Here's a simple fix for #31, anyone have any better suggestions?

diff --git a/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraintValidator.php b/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraintValidator.php
index 2059a665e5..36e4d80a8a 100644
--- a/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraintValidator.php
+++ b/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraintValidator.php
@@ -48,7 +48,7 @@ public function validate($entity, Constraint $constraint) {
       $term_storage = $this->entityTypeManager->getStorage($entity->getEntityTypeId());
 
       $new_parents = array_column($entity->parent->getValue(), 'target_id');
-      $original_parents = array_keys($term_storage->loadParents($entity->id()));
+      $original_parents = array_keys($term_storage->loadParents($entity->id())) ?: [0];
       if ($new_parents != $original_parents) {
         $this->context->buildViolation($constraint->message)
           ->atPath('parent')

On the overview page we can use a query like select tid from taxonomy_term_revision ttr where revision_id > (select revision_id from taxonomy_term_data where tid = ttr.tid) group by tid; to find all terms with pending revisions, then prevent the weight or parent from being changed.

CR updated: https://www.drupal.org/node/2897789/revisions/view/10578842/10580842

Trying to work out how to implement the query from #34.

  protected function getTermsWithPendingRevisions(VocabularyInterface $taxonomy_vacabulary) {
    $term_definition = $this->entityManager->getDefinition('taxonomy_term');
    $base_table = $term_definition->getBaseTable();
    $revision_table = $term_definition->getRevisionTable();
    $connection = \Drupal::database();
    $subquery = $connection->select($base_table, 'ttd')
      ->fields('ttd', [$term_definition->getKey('revision')])
      ->condition($term_definition->getKey('id'), 'ttr.' . $term_definition->getKey('id'));
    return $connection->select($revision_table, 'ttr')
      ->fields('ttr', [$term_definition->getKey('id')])
      ->condition($term_definition->getKey('revision'), $subquery, '>')
      ->groupBy($term_definition->getKey('id'))
      ->execute()
      ->fetchAll();
  }

This is just returning an empty array.

Adding needs tests tag for #31 and #32

Sorry, I didn't explain #32 very well, and yes, this does happen without this patch, but this patch adds to the issue.

Steps to reproduce:

1. Create a few terms, with as the parent, and publish.
2. Create another term with the root and one (or more) other terms as the parents, and publish.
3. Edit this term, and will not be selected anymore as the parent, so saving will remove as a parent. (This is the existing part of the bug)
4. Saving as a draft (pending revision) is not blocked by the constraint and the published (default) revision will lose it's parent.

As this is an existing bug and not really made much worse by this issue I'll open a follow up.

Putting back to "Needs review" for further discussion on the overview page.

Created a follow up; #2898903: Terms lose <root> as the parent when editing

How about something like this?

Find which terms have pending revisions, throw a warning, and highlight them with a *.

Forum extends the taxonomy overview page!

Opened up #2899923: UI for publishing/unpublishing taxonomy terms for when this is done

I would like us to use a more clear signal (the * is already used for unsaved, so I am not sure about reusing that).

What about adding a background color (warning yellow) and possibly appending the warning icon.

Just wondering we say "could mean losing", do we not know?

The wording needs some work, after testing this, as far as I can see, all pending revisions for this vocabulary are lost.

The warning icon would be a nice addition, and not theme specific.

We reviewed this on the weekly UX call today.

It sounds like the bulk of these concerns are because Taxonomy terms (as well as Custom Blocks, which I didn't know) don't have a "Revision Log" page like nodes do that you could use to revert to a previous revision. So pending changes automatically get applied and become the published revision, which may or may not be what you want.

A few suggestions to address this (in addition to Bojhan's above, which is a good one):

- Grey out/disable the table drag option for any taxonomy terms with pending revisions, so you can't get into this situation in the first place.
- Put the warning icon that @Bojhan suggested in place of the table drag icon, to keep the padding the same.
- Add the moderation state as a column to this table, and add the * there instead.
- Grey out/disable the save button for the table while this condition exists, forcing you to resolve it prior to making other changes.

One other thing that was weird is there were *two* error message sections... one at the top of the page and one at the top of the table. We should merge the two of them. Possibly look to Inline Form Errors for a pattern we can use here, since it has both a summary of the errors at the top of the page, but also link to where the error actually is.

Based on the notes from the UX call I created 2 options

1. - when there is a pending revision, disable the drag&drop icons and the reset button to prevent making changes. This would be the preferred solution, not letting user to make changes that could confuse him.

2. - if (1) wouldn't be possible, disable the save & reset button and show error message and inline error icon/warning.

In both cases warning/error messages need to have a clear copy so that user would know exactly how to resolve the issue/what to do.

Option 1:

Option 2:

Here's something similar to option 1 in #49.

I've replaced the handle with the warning triangle, removed the save buttons (instead of disabling them), and updated the message.

I think we should add @todo on taxonomy_build_node_index with the issue discussed in #3 and #4.

Nice find @amateescu!

Looks good to me, but I'll review properly when I'm on my computer rather than my phone.

The message sounds a bit wordy, but think it covers all areas well and can't think of anything better. Only nit pick there is we have "drag and drop" as well as "drag-and-drop", should use the same format everywhere. Also I wonder if something like "reordering" or "sorting" is clearer than "drag-and-drop", but as "drag-and-drop" is already used in the two parents example, then it should be fine.

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -502,26 +520,28 @@ public function submitReset(array &$form, FormStateInterface $form_state) {
-  protected function getTermsWithPendingRevisions(VocabularyInterface $taxonomy_vocabulary) {
-    $term_definition = $this->entityManager->getDefinition('taxonomy_term');
-    $id_key = $term_definition->getKey('id');
-    $revision_key = $term_definition->getKey('revision');
-    $base_table = $term_definition->getBaseTable();
-    $revision_table = $term_definition->getRevisionTable();
-    $query = $this->database
-      ->select($revision_table, 'ttr')
-      ->fields('ttr', [$id_key]);
-    $query->join($base_table, 'ttd', 'ttr.' . $id_key . ' = ttd.' . $id_key);
-    return $query->where('ttr.' . $revision_key . ' > ttd.' . $revision_key)
-      ->groupBy($id_key)
-      ->execute()
-      ->fetchCol();
+  protected function getTermsWithPendingRevisions(VocabularyInterface $vocabulary) {
+    $entity_type = $this->entityManager->getDefinition('taxonomy_term');
+
+    $query = $this->storageController->getQuery()
+      ->condition($entity_type->getKey('bundle'), $vocabulary->id(), '=')
+      ->sort($entity_type->getKey('revision'), 'DESC');
+    $default_revisions = $query->execute();
+
+    $latest_revisions = (clone $query)
+      ->allRevisions()
+      ->execute();
+    $latest_revisions = array_unique($latest_revisions);
+

Are two queries + array_unique + array_diff_key, better than one query? Serious question, I've no idea.

--- a/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php

❤️ I love how simple it is to update the REST test coverage!

P.S.: it's again my duty to point out how this technically a BC break in the data model. But it's only adding fields, and we have a precedent for this. It's very unlikely to break decoupled apps. So I'm fine with this.

#1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission) has landed :)

Re-rolled.

Corrected the coding standards errors.

Not sure how to fix the test failures.

@Jo Fitzgerald - The failing tests looks as though it's related to permissions issue, so it is related to the changes added in #1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission).

This seems to move the help text from taxonomy_help() into the form?

Not sure why, but that's what's causing that error, the help text is now dynamic based on the overview permission. This removes the new code in taxonomy_help() but I didn't properly update the new code?

Also, note that forum.module extends that form which means it would inherit that info text as well? That might not actually make sense and would need to be overriden again?

marked #2878229: Port to Drupal 8? and #2919540: Project necessity? as duplicates.

I think that since #22, it's been established that adding fields is okay. It's removing/changing them that constitutes a BC break.

Updating issue tags :)

Rerolling #63.

Had to manually fix core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php, and I was not 100% sure, so that could use review :)

I'm trying to review this but ran into some problems.

How does I get these views: (?)

I have content moderation and I'm able to create revisions to terms but I can't review any revisions from the UI.

This patch is functionally ready IMO, so it needs review if we want to have revisionable and publishable taxonomy terms in Drupal 8.5 :)

Would indeed be great to get that in for 8.5 :) So, here's a review!

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
   @@ -140,6 +143,21 @@ protected function getExpectedNormalizedEntity() {
   +        $this->formatExpectedTimestampItemValues((int) $this->entity->getRevisionCreationTime()),
   

   Nit: This typecast should not be necessary. Because CreatedItem extends TimestampItem, which uses \Drupal\Core\TypedData\Plugin\DataType\Timestamp extends IntegerData.

2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoBundle.php
   @@ -12,6 +12,7 @@
   + *     "label" = "name",
   

   I'm slightly confused why this change is in this patch?

3. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -227,6 +228,56 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   +          $help_message = $this->t('You can reorganize the terms in %capital_name using their drag-and-drop handles, and group terms under a parent term by sliding them under and to the right of the parent.', $args);
   ...
   +          $help_message = $this->t('%capital_name contains terms grouped under parent terms. You can reorganize the terms in %capital_name using their drag-and-drop handles.', $args);
   ...
   +          $help_message = $this->t('%capital_name contains terms with multiple parents. Drag and drop of terms with multiple parents is not supported, but you can re-enable drag-and-drop support by editing each term to include only a single parent.', $args);
   

   Isn't adding better help text out of scope for an issue that is about adding revision support?

4. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -267,6 +318,13 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   +      if (in_array($term->id(), $pending_term_ids)) {
   

   Nit: let's do strict comparison.

5. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -504,4 +562,29 @@ public function submitReset(array &$form, FormStateInterface $form_state) {
   +   * @return array
   +   *   An array of term IDs which have pending revisions, keyed by revision IDs.
   

   Should this be int[] given this description?

6. +++ b/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraint.php
   @@ -0,0 +1,26 @@
   + * Validation constraint for changing the term hierarchy in pending revisions.
   ...
   + *   label = @Translation("Taxonomy hierarchy.", context = "Validation"),
   

   Should the label be Term hierarchy?

7. +++ b/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraint.php
   @@ -0,0 +1,26 @@
   +  public $message = 'You can only change the taxonomy hierarchy for the <em>published</em> version of this term.';
   

   Missing inheritdoc?

   More importantly: s/taxonomy hierarchy/hierarchy/

8. +++ b/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraintValidator.php
   @@ -0,0 +1,68 @@
   +      $new_parents = array_column($entity->parent->getValue(), 'target_id');
   

   Clever!

9. +++ b/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraintValidator.php
   @@ -0,0 +1,68 @@
   +      if ($new_parents != $original_parents) {
   

   Can/should this become a strict comparison?

10. +++ b/core/modules/taxonomy/src/Plugin/Validation/Constraint/TaxonomyHierarchyConstraintValidator.php
    @@ -0,0 +1,68 @@
    +      /** @var \Drupal\Core\Entity\ContentEntityInterface $original */
    +      $original = $term_storage->loadUnchanged($entity->id());
    

    Can't we typehint this to TermInterface instead?

11. +++ b/core/modules/taxonomy/src/TermForm.php
    @@ -123,6 +124,28 @@ public function buildEntity(array $form, FormStateInterface $form_state) {
    +    // Manually flag violations of fields not handled by the form display.
    

    How do we know these fields aren't handled by the form display?

12. +++ b/core/modules/taxonomy/src/Tests/Update/TaxonomyUpdateTest.php
    @@ -0,0 +1,75 @@
    +class TaxonomyUpdateTest extends UpdatePathTestBase {
    

    Shouldn't this get a less generic name? For example TermRevisionablePublishableUpdateTest?

1. Right, it should not be necessary. I know that there are occurrences of this elsewhere in core (which can be tricky/risky to fix due to BC), but I was hoping that we could not repeat this problem in more places.
2. 👍
3. D'oh, sorry!
4. :( Any chance we can just fix getRevisionCreationTime()? People in generic (non-Term-specific) calling code will use strict comparison (I would) and then will run into problems like this.
5. 👍
6. 👍
7. 👍
8. :)
9. See 4.
10. 👍
11. Can we then add an @see for that?
12. 👍

#77: you're right. Could you perhaps create a follow-up for that?

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -504,4 +562,29 @@ public function submitReset(array &$form, FormStateInterface $form_state) {
+
+    $query = $this->storageController->getQuery()
+      ->condition($entity_type->getKey('bundle'), $vocabulary->id(), '=')
+      ->sort($entity_type->getKey('revision'), 'DESC');
+    $default_revisions = $query->execute();
+
+    $latest_revisions = (clone $query)
+      ->allRevisions()
+      ->execute();
+    $latest_revisions = array_unique($latest_revisions);

This needs an accessCheck(FALSE), and maybe test coverage with/without an access module.

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -504,4 +562,30 @@ public function submitReset(array &$form, FormStateInterface $form_state) {
+    $latest_revisions = (clone $query)
+      ->allRevisions()
+      ->execute();

Reports of
Parse error: syntax error, unexpected '->' (T_OBJECT_OPERATOR)

for this on simplytest.me, queueing testing on php5

Crediting @pameeela who manually tested and found #85 (reported via slack)

Test on php 5.5 indeed failed linting

Looks good to me!

@amateescu so I'd expect it would be very hard to see the terms on the overview page at all if you didn't already have access to them so test coverage there probably isn't the most important.

However I'm a bit more concerned about jibran's comment #3 which isn't really covered by the UI follow-up:

- do all views need to be updated to add a status = 1 condition?

- what do permissions look like?

- entity reference selection handlers and similar too.

- if we do that, do we need to add term_status to the taxonomy_index table?

On top of that contrib/custom code will need updating as well, but I don't think we can recommend that until core is consistent.

I'm not really sure if this is scope that should be added to the UI follow-up (including a re-title) or here though.

Media is also a publishable entity type and it doesn't have any special permission in regards to publishing, so I'm not sure if we need to add new permissions for terms.

They are not special, but media entities did just get per-type permissions: #2862422: Add per-media type creation permissions for media

Just noting that this patch does introduce one UI change, it adds a "Revision log message" field to each term. I know that the UI component is covered in #2899923: UI for publishing/unpublishing taxonomy terms so it seems it might be better to hold back this field until then? It is a bit strange to have a field where you can enter text but then never access it.

Fair enough if #2899923: UI for publishing/unpublishing taxonomy terms isn't related, I just assumed it was captured elsewhere because this issue says "No UI changes."

So at what point do the revisions become accessible in the UI? When you make a revision of a node you get the Revisions tab. That doesn't happen with terms as far as I can tell. Is there a separate follow up for that?

It looks as though @amateescu has addressed all of the points, so we can go back to RTBC.

#94.1 on one hand I think it's good to do the work in reviewable chunks, on the other we're technically introducing an information disclosure bug if we commit this without filtering on published status (although not one you could run into from just using the UI). Will try to get a second opinion.

#94.2

Media is also a publishable entity type and it doesn't have any special permission in regards to publishing, so I'm not sure if we need to add new permissions for terms.

I wasn't thinking about permissions to publish, but more something like 'view unpublished terms' permission or similar - i.e. if you edit a term, set it to unpublished, then it seems like you wouldn't be able to then view the term page to edit and republish it.

With nodes we manage that with 'view own unpublished nodes'. Media entities usually aren't accessed via the canonical URL as much so it's a bit different.

If new views get the filter (as they do according to #94) then I think I agree with @catch and they we should the filter to existing views.

Yes as #103 notes you can't unpublish a term via the UI but if the patch goes in as in we have an hard to guess inconsistency. If a user creates a view on taxonomy terms then upgrades to a version of Drupal with this patch and they use the API to unpublish a term, or some contrib module, that term still be listed on that view. If they repeat exactly the same steps to create view it won't be listed on the new view because the filter will be automatically added.

Yes I think we need to add the filters here too.

The issues around editing of unpublished terms and similar we can probably defer to follow-ups though.

Reroll of #99.
Manually fixed conflict on taxonomy.install

On permissions, since media was mentioned, note that there's a patch to introduce the 'View any unpublished' permission (borrowed from Node module) here #2936652: Add "view unpublished $bundle media" permissions for each media bundle.

Just to clarify, this is currently "needs work" because of failing tests, and because we need to filter out unpublished terms, per #106?

Let's postpone this on #2947351: DefaultTableMapping does not return the revision table name for multi-valued base fields so it's clear what's going on.

This is nearly there, awesome work!

We should probably add an update test for taxonomy_update_8601, which was introduced on #115. There's an example covering a very similar change on the patch for #2909435: Update block library page to adapt publishable block content implementation

Here is a start for the views update test. Not passing locally but I can't figure out why, let's see if the bot helps.

Looks like it does pass then!

Thanks @Manuel Garcia and @amateescu!

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -504,4 +562,29 @@ public function submitReset(array &$form, FormStateInterface $form_state) {
 
+  /**
+   * Gets the terms with pending revisions from a vocabulary.
+   *
+   * @param \Drupal\taxonomy\VocabularyInterface $vocabulary
+   *   A taxonomy vocabulary.
+   *
+   * @return int[]
+   *   An array of term IDs which have pending revisions, keyed by revision IDs.
+   */
+  protected function getTermsWithPendingRevisions(VocabularyInterface $vocabulary) {
+    $entity_type = $this->entityManager->getDefinition('taxonomy_term');
+
+    $query = $this->storageController->getQuery()
+      ->condition($entity_type->getKey('bundle'), $vocabulary->id(), '=')
+      ->sort($entity_type->getKey('revision'), 'DESC')
+      ->accessCheck(FALSE);
+    $default_revisions = $query->execute();
+
+    $latest_revisions_query = clone $query;
+    $latest_revisions = $latest_revisions_query->allRevisions()->execute();
+    $latest_revisions = array_unique($latest_revisions);
+
+    return array_diff_key($latest_revisions, $default_revisions);
+  }
+

This is getting every revision of every term in a vocabulary, it's quite easy to have tens of thousands of terms in a vocabulary - for example the Encylopedia of Life or any community site with a tags field.

It could probably be a method on the term storage instead doing a direct query - if it's not possible to do this logic via entity query which I assume it' s not.

+++ b/core/modules/taxonomy/taxonomy.install
@@ -126,3 +145,174 @@ function taxonomy_update_8503() {
+
+  $config_factory = \Drupal::configFactory();
+  foreach ($config_factory->listAll('views.view.') as $view_config_name) {

This should be done in a batch, I think we recently added a helper class to make it easier to batch config updates.

This should be done in a batch, I think we recently added a helper class to make it easier to batch config updates.

@catch hope you are referring to #2949351: Add a helper class to make updating configuration simple

Patch in #122 has been re-rolled for 8.5.x at https://www.drupal.org/project/drupal/issues/2956149.

#2949351: Add a helper class to make updating configuration simple landed, so this needs some work now

Patch no longer applies. Re-rolled.

Nice work @amateescu - #124.2 has been addressed, and I agree with the solution provided for #124.1 - so RTBCing - please let us know what you think @catch =)

How about if we restrict those two entity queries to the terms that are actually shown on the page? I'd like to avoid adding a method to the term storage which will only be used in one place.

I don't think this is right. The message says that a vocabulary with just one term that has a pending revision can't be dragged and dropped, which means any term in the vocabulary not just what's on the page. So either the help message is wrong or the code is after that change.

This all stems from taxonomy hierarchy itself not being revisionable but we're obviously not there yet.

The trick to check for pending revisions is kinda neat, but catch is right that the query for that is horrible.

With workspaces obviously it would be simple to check if this is live and then allow it, else disallow it.

Lets see if we can optimize that query (I left out the type):

SELECT * from taxonomy_term_data td WHERE td.entity_id in ($tids)
LEFT JOIN taxonomy_term_revision_data tr ON tr.entity_id = td.entity_id
WHERE tr.entity_revision > td.entity_revision

would be my try unless you can publish a revision as default, which is not the highest, but in that case the query in the patch would fail, too as it depends on DESC sort order.