UI for publishing/unpublishing taxonomy terms

Here is an update

Thanks for the quick review.

Test failing with:
The Taxonomy term entity type needs to be updated.

at Drupal\FunctionalTests\Update\UpdatePathTestBase::runUpdates does it mean we need to wait for #2880149: Convert taxonomy terms to be revisionable to get in?

1. +++ b/core/modules/taxonomy/src/Plugin/Action/PublishTaxonomyTerm.php
   @@ -0,0 +1,40 @@
   +class PublishTaxonomyTerm extends ActionBase {
   
   +++ b/core/modules/taxonomy/src/Plugin/Action/UnpublishTaxonomyTerm.php
   @@ -0,0 +1,40 @@
   +class UnpublishTaxonomyTerm extends ActionBase {
   

   Remove this classes and use the generic entity action plugins.

2. +++ b/core/modules/taxonomy/src/Plugin/views/field/TaxonomyTermBulkForm.php
   @@ -0,0 +1,21 @@
   +class TaxonomyTermBulkForm extends BulkForm {
   

   Remove this class and use the generic BulkForm. We did that for media and block_content as well. It's overkill to have this.

Blocker is in :)

Fixed some tests.

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -131,6 +131,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDisplayOptions('form', [
...
+      ->setDisplayConfigurable('form', TRUE);

We need an upgrade path for these changes.

Thought a bit about #28:
I agree that we should only focus on the status checkbox in this issue, because having bulk operations for terms is much more work.

I guess we first have to replace the EntityListBuilder with a real view on the term overview page. The related issue for that might be #113344: Taxonomy term manager view

I guess we first have to replace the EntityListBuilder with a real view on the term overview page.

Unfortuntely this is not possible until we add https://www.drupal.org/project/draggableviews to core.

Re #34: I don't think we need an upgrade path for this. After a cache clear the status field is already on all the term forms. So I added an empty post_update function to ensure the cache is cleared.

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -131,6 +131,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDisplayConfigurable('form', TRUE);

Do we need to set up this as well in upgrade path?

I still think we don't need the update path. Running the testStatusCheckbox() with an empty taxonomy_post_update_configure_status_field_widget() function also passes.

We discussed this in slack with amateescu

amateescu [1:04 PM]
reading backscroll
@Manuel Garcia @chr.fritsch view/form display settings are not stored in any "last installed definitions" afaik, so an empty post update function to clear the caches should be enough (edited)

chr.fritsch [1:09 PM]
@amateescu Thats also my understanding. Thank you, Sir. I will copy your answer into the issue

amateescu [1:10 PM]
@chr.fritsch let me confirm that locally :slightly_smiling_face:

Manuel Garcia [1:10 PM]
humm ok, great thanks @amateescu - do we want the update test? else your previous patch is good @chr.fritsch

amateescu [1:13 PM]
my answer was slightly wrong though
we do keep that information in the last installed definitions, but the code which instantiates the widget/formatters does not use that information, it only uses the runtime (in-code) definition
which means that the data stored in the last installed definitions is irrelevant, at least for now
the outcome is the same though, we only need an empty post update function and no update path test
because the update path test should not check runtime definitions

chr.fritsch [1:17 PM]
@amateescu @Manuel Garcia Perfect. Thanks for investigating. I also cleared node_post_update_configure_status_field_widget() and ran NodeUpdateTest and it’s still green

amateescu [1:17 PM]
we would need a proper update function with test coverage only if/when this issue will be fixed: https://www.drupal.org/project/drupal/issues/2554235

Manuel Garcia [1:19 PM]
oh wow, thats very good to know thanks @amateescu

That means #38 is the patch for further reviewing.

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -131,6 +131,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      ->setDisplayOptions('form', [
+        'type' => 'boolean_checkbox',
+        'settings' => [
+          'display_label' => TRUE,
+        ],
+        'weight' => 100,
+      ])
+      ->setDisplayConfigurable('form', TRUE);

This settings change has nothing to do with installed base field definition. Yes, we are setting an option on BFD object but we are making changes to default form mode. Entity form modes are config entities and setDisplayOptions is making a change to the values of the config entity which should be replicated in the active storage so we need an update path for this.

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -125,3 +126,22 @@ function taxonomy_post_update_handle_publishing_status_addition_in_views(&$sandb
+    if ($entity_form_display->getTargetEntityTypeId() !== 'taxonomy_term') {

We should also bail out if status field is already visible we don't want to change the settings of already visible field.

This settings change has nothing to do with installed base field definition. Yes, we are setting an option on BFD object but we are making changes to default form mode. Entity form modes are config entities and setDisplayOptions is making a change to the values of the config entity which should be replicated in the active storage so we need an update path for this.

I don't see where we are making changes to the form mode config entity:

  public function setDisplayOptions($display_context, array $options) {
    $this->definition['display'][$display_context]['options'] = $options;
    return $this;
  }

On the contrary, not touching the default form mode config entity in this patch allows their possibly custom values to remain the same (as configured by the site builder) so the fact that we don't override user configuration is a good thing.

On the contrary, not touching the default form mode config entity in this patch allows their possibly custom values to remain the same (as configured by the site builder) so the fact that we don't override user configuration is a good thing.

I think if the status field is already visible we shouldn't change the configuration of the status field in default form mode and if the status field is not visible then we should set up the default display option because after applying this patch and enabling the taxonomy module the user will get the status field visible in the default form mode so adding the upgrade path will allow us to keep the default behavior.

Let's test your theory we only need an empty post update function with the upgrade path test. If the test would fail then that would mean we needed to update the status field config in the form modes.

I do think we need to resave all the form displays, though, so that the status field will get added to the list of disabled fields at least. Otherwise people will have unexpected diffs in their config the next time they happen to save a taxonomy form display.

Thinking a bit more about this, I think I agree with @tstoeckler that we probably should include the update path, but only add the status component to the form display if it's not already there (e.g. already configured by the site builder).

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -132,7 +132,7 @@ function taxonomy_post_update_handle_publishing_status_addition_in_views(&$sandb
+    if ($entity_form_display->getTargetEntityTypeId() !== 'taxonomy_term' || !empty($entity_form_display->getComponent('status'))) {

The logic is quite convoluted but correct and I don't see how can we simplify this. Anyway, this is the last outstanding feedback so RTBC.

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -125,3 +126,22 @@ function taxonomy_post_update_handle_publishing_status_addition_in_views(&$sandb
+    if ($entity_form_display->getTargetEntityTypeId() !== 'taxonomy_term' || !empty($entity_form_display->getComponent('status'))) {
+      return FALSE;
+    }

I would split this into two different if statements. That way it is more readable and more self documenting. Ie. don't update if it not for taxonomy terms. And then don't update if there is configuration for the status field already. Or perhaps this is better written the other way around. Ie.

    // Only update taxonomy term entity form displays with no existing options for the status field.
    if ($entity_form_display->getTargetEntityTypeId() === 'taxonomy_term' && empty($entity_form_display->getComponent('status'))) {
      $entity_form_display->setComponent('status', [
        'type' => 'boolean_checkbox',
        'settings' => [
          'display_label' => TRUE,
        ],
      ]);
      return TRUE;
    }

    return FALSE;

We also need testing that the entity displays with existing options are not overwritten.

Since this is just minor logic re-work feel free to re-rtbc on a new patch.

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -125,3 +126,21 @@ function taxonomy_post_update_handle_publishing_status_addition_in_views(&$sandb
+    if ($entity_form_display->getTargetEntityTypeId() == 'taxonomy_term' || empty($entity_form_display->getComponent('status'))) {

I think this should be &&

Plus as mentioned in #55 we also need testing that the entity displays with existing options are not overwritten.

Plus the code comment added in #55 is helpful as it explains why the condition.

// Only update taxonomy term entity form displays with no existing options for the status field.

The new test coverage looks good to me!

+++ b/core/modules/migrate/tests/src/Kernel/MigrateBundleTest.php
@@ -18,7 +18,7 @@ class MigrateBundleTest extends MigrateTestBase {
-  public static $modules = ['taxonomy', 'text', 'user'];
+  public static $modules = ['taxonomy', 'text', 'user', 'system'];

+++ b/core/modules/migrate/tests/src/Kernel/MigrateRollbackEntityConfigTest.php
@@ -17,7 +17,7 @@ class MigrateRollbackEntityConfigTest extends MigrateTestBase {
-  public static $modules = ['field', 'taxonomy', 'text', 'language', 'config_translation', 'user'];
+  public static $modules = ['field', 'taxonomy', 'text', 'language', 'config_translation', 'user', 'system'];

+++ b/core/modules/migrate/tests/src/Kernel/MigrateRollbackTest.php
@@ -20,7 +20,7 @@ class MigrateRollbackTest extends MigrateTestBase {
-  public static $modules = ['field', 'taxonomy', 'text', 'user'];
+  public static $modules = ['field', 'taxonomy', 'text', 'user', 'system'];

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d6/MigrateUpgrade6Test.php
@@ -82,7 +82,7 @@ protected function getEntityCounts() {
+      'action' => 25,

+++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/MigrateUpgrade7Test.php
@@ -82,7 +82,7 @@ protected function getEntityCounts() {
-      'action' => 17,
+      'action' => 19,

unrelated?

All those changes were needed to fix the test fails discovered back in #28 on the "combined" patch and then fixed in #33. System module is needed for the action config entity schema, and this patch adds actions so the action counts have gone up!

thanks

Confirming RTBC. Thank you very much! I added a test runner against 8.6.x to see if this patch an be used in that branch already for stable users who need that functionality (backported). Can this be part of a minor 8.6.x release or will it definitely be 8.7.x?

Thank you all very much!

Update: 8.6.x passed!

@Anybody: As this is a feature request and not a bug fix, it will not be added to 8.6.x.

We're using this in production on 8.6.x now - successfully. Thank you all very very much!

I just ran into the issue that the published checkbox state is not saved (enabled to disabled) when using content translation.
I managed to uncheck it by a workaround (but that is only possible for one language, so no solution): Also uncheck "This translation is published" AND uncheck "Published", then save... then the "Published" checkbox is saved unchecked.

Otherwise the taxonomy term stays published and the checkbox is checked again after save (reload).

I had that situation on our live environment together with other modules so could someone else please test that or write a test?

See #2971390: Make exposure of translation meta fields conditional, those ui elements are currently hardcoded, the patch there tries to make them more dynamic. Right now, it's just the node translation handler that overwrites some of that logic and even that is partially wrong.

@Manual Garcia: I just tried that in the same isntallation and unpublishing / publishing a node works as expected. So this problem doesn't exist for nodes. Thank you @Berdir so that's the cause for these problems?

I can confirm this problem still persists and think THIS issue can not be completet until that problem is finally solved.

I did notice that even though the taxonomy is unpublished, the taxonomy term page still accessible. Node have an item in permission View own unpublished content. Shouldn't taxonomy has the same ability to limit access to unpublished terms? Thanks

@Manuel Garcia, thank you very much! If I read the patch correctly, the problem from #76 should be solved with this?
If yes, I'd retest the patch manually. Any other known missing pieces in the patch or tests?

Thank you very very much, the patch from #86 works perfectly and solves the problem described in #76! GREAT NEWS!
RTBC +1 from manual testing! No other related issues found!

Perhaps RTBC + a note / reference to the issue in the RTBC Comment and in the issue description?

RTBC as of #92. Now waiting for #2971390: Make exposure of translation meta fields conditional

Upgrade to 8.6.8 broke with old patch. But #98 seems to have fix it, thanks Manuel.

I can confirm #98 works with latest Drupal core. Re-Confirming RTBC.

This needs a reroll but the 8.6 patch was somehow preventing it from being set back.

Re-roll for conflict in use

There are some @todo about this issue in core now after #2880149-291: Convert taxonomy terms to be revisionable.

Partially restored test and changes from #2880149-291 except validation which needs work as a test fails

Created the followup issue for re-enabling CM integration for terms: #3047110: Enable Content Moderation integration for taxonomy terms

I think #107 is good to go, again!

Patch works fine! I want it in the Drupal core :)

Also working for me against 8.7.3. Nice work everyone!

Thanks all for fixing the concerns raised!

Committed 8e31070 and pushed to 8.8.x. Thanks!

I'm getting failures on contrib testing: https://www.drupal.org/pift-ci-job/1327224. I thought it was a head failure and I could ignore, but happened again last night.

Still getting test failures: https://www.drupal.org/pift-ci-job/1333904

@heddn fix is available at #3065517: Fix tests on 8.x-4.x HEAD

#110 works thanks.

Will this be committed to 8.8?

It was already committed in June, see #113.

I think #3083609: The new "Published" checkbox on the forum edit page doesn’t do anything may be related to this issue. Is it?

  - Installing drupal/core (8.7.5): Loading from cache
  - Applying patches for drupal/core
    https://www.drupal.org/files/issues/2019-05-13/2899923-110.patch (Term status UI)

Confirmed working for 8.7.

Remember to remove patch before upgrading to 8.8

I have imported thousands of terms from one site in to a new Drupal 8 site and all of them are unpublished and I didn't realise.

Anyway, not to worry, but is there a way to bulk publish taxonomy terms or do I have to go through them all individually?

I assume you did that programatically? You could either try to update it directly in the database tables and clear cache (and create a backup first), or load all terms, set to published and save again, that could take a while though.

Or create a temporary taxonomy term view with a https://www.drupal.org/project/views_bulk_operations field to set entity values / published status, if you want to solve this in UI. I guess that should also work.

Ended up figuring it out straight after this with VBO yes and "bulk change" field in a view.

Got it all done in a few minutes, thanks Berdir and Anybody!

If anyone needs a patch for 8.7.13, please see #3127752: UI for publishing/unpublishing taxonomy terms (version 8.7.13 patch).