Add a publishing status to taxonomy terms

I can't find any issues, looks good to me.

Should we also port all of the credit from #2880149: Convert taxonomy terms to be revisionable to this issue?

This looks good, I like how the update was considering the content translation scenario and views. On the other hand terms could appear in different ways in views, eg. as a relationship. What would happen to those?

Similar question: does this affect entity reference fields, do those have settings to allow unpublished/published or how does the handling of access happen there?

Also not RTBC yet given the lack of credit carryover.

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -18,3 +21,107 @@ function taxonomy_post_update_clear_views_data_cache() {
+ * Add a 'published' = TRUE filter for all Taxonomy term views and converts
+ * existing ones that were using the 'content_translation_status' field.
+ */

Shouldn't we also convert views that show entities with taxonomy term arguments, so that the argument validation fails if the term is unpublished?

> @joachim, I looked at all the views argument plugins provided by the node module (in core/modules/node/src/Plugin/views/argument and .../argument_default) and I couldn't find anything related to the publishing status of a node. I guess that means there's nothing special to do for taxonomy terms as well?

I don't really follow... a term argument won't care about node status. But it *should* care about taxonomy status.

Here's how I see it:

Consider the taxonomy_term view that taxonomy module provides OOTB. This shows published nodes that are tagged with a term.

Suppose node N is tagged with term T.

- if I unpublish the node N, then it doesn't appear in the list of nodes for term T
- if I unpublish the term T, then the whole of the page at taxonomy/term/T should go. Because otherwise, it's possible to get to that URL and see the term's name, and the things it's tagged with. Which is pretty much what "seeing the term" means. And unpublishing it should mean you can't see it any more.

+++ b/core/modules/taxonomy/taxonomy.post_update.php
@@ -18,3 +21,107 @@ function taxonomy_post_update_clear_views_data_cache() {
+ * Add a 'published' = TRUE filter for all Taxonomy term views and converts
...
+    // Only alter taxonomy term views.
+    if ($view->get('base_table') !== 'taxonomy_term_field_data') {
+      return FALSE;
+    }

This update is only changing views whose base is terms. So it won't change core's taxonomy_term view, for instance, as the base of that is nodes.

Am I looking at the wrong one?

> That's right, the post update function from this patch does not change core's taxonomy_term view.

That's what I am trying to say.

> And the scenario you describe is *exactly* what the patch from #15 provides.

And so no, this does not happen.

If term 42 is 'Cats', and I unpublish it, then /taxonomy/term/42 will *still* show a title of 'Cats' and the nodes tagged with 'Cats'.

So, what has unpublishing actually done? The term's title is still visible, and it's still possible to see the relationships. Surely that's a security issue.

Yes, you're right. Sorry for getting all mixed up -- running on insufficient sleep :(

@amateescu++

Went over the test, I think its pristine, only one nitpick

+++ b/core/modules/taxonomy/tests/src/Functional/TermAccessTest.php
@@ -0,0 +1,124 @@
+    // For some reason, Views emits a 404 status code if the argument does not
+    // validate.

Hummm this sounds like we should open up a bug report and link to it here perhaps (not a blocker for this obviously).

Thanks @amateescu, even with a patch!

This looks RTBC to me...

+++ b/core/modules/rest/tests/src/Functional/EntityResource/XmlEntityNormalizationQuirksTrait.php
@@ -63,6 +64,9 @@ protected function applyXmlFieldDecodingQuirks(array $normalization) {
+            //   https://www.drupal.org/project/drupal/issues/2936864.

+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -116,6 +120,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    //   https://www.drupal.org/project/drupal/issues/2936864.

+++ b/core/modules/taxonomy/tests/src/Functional/TermAccessTest.php
@@ -0,0 +1,124 @@
+    //   https://www.drupal.org/project/drupal/issues/2983070 is fixed.

Nit: do we need these two extra spaces before the links?

Ah of course, thanks @amateescu for clarifying :)

I think this is ready now, anything else left here besides carrying credit from #2880149: Convert taxonomy terms to be revisionable ?

I think we're done..

Looks good!

Looks like we need to add @group legacy in the update tests since #2973791: Fix deprecation messages related to deprecated comment Action plugins was committed.

Can we please also add a change notice for this?

Ignore previous comment found https://www.drupal.org/node/2897789

But it looks like we should split the CR as well

I have a question: if a term is published and its parent is unpublished would the user(with view access) be able to see the term? If the same user has the edit access and can't view unpublished parent then how would the parent widget look like or allow the user to change the parent?

1. +++ b/core/modules/taxonomy/src/TermAccessControlHandler.php
   @@ -18,19 +18,37 @@ class TermAccessControlHandler extends EntityAccessControlHandler {
   +    if ($account->hasPermission('administer taxonomy')) {
   +      return AccessResult::allowed()->cachePerPermissions();
   +    }
   ...
   -        return AccessResult::allowedIfHasPermissions($account, ["edit terms in {$entity->bundle()}", 'administer taxonomy'], 'OR');
   +        if ($account->hasPermission("edit terms in {$entity->bundle()}")) {
   +          return AccessResult::allowed()->cachePerPermissions();
   +        }
   +
   +        return AccessResult::neutral()->setReason("The following permissions are required: 'edit terms in {$entity->bundle()}' OR 'administer taxonomy'.");
    
          case 'delete':
   -        return AccessResult::allowedIfHasPermissions($account, ["delete terms in {$entity->bundle()}", 'administer taxonomy'], 'OR');
   +        if ($account->hasPermission("delete terms in {$entity->bundle()}")) {
   +          return AccessResult::allowed()->cachePerPermissions();
   +        }
   +
   +        return AccessResult::neutral()->setReason("The following permissions are required: 'delete terms in {$entity->bundle()}' OR 'administer taxonomy'.");
   ...
   -        return AccessResult::neutral();
   +        return AccessResult::neutral()->cachePerPermissions();
   

   👍

2. +++ b/core/modules/taxonomy/src/TermAccessControlHandler.php
   @@ -18,19 +18,37 @@ class TermAccessControlHandler extends EntityAccessControlHandler {
   -        return AccessResult::allowedIfHasPermission($account, 'access content');
   +        $access_result = AccessResult::allowedIf($account->hasPermission('access content') && $entity->isPublished())
   +          ->cachePerPermissions()
   +          ->addCacheableDependency($entity);
   +        if (!$access_result->isAllowed()) {
   +          $access_result->setReason("The 'access content' permission is required and the taxonomy term must be published.");
   +        }
   +        return $access_result;
   

   👍

3. +++ b/core/modules/taxonomy/tests/fixtures/update/views.view.test_taxonomy_term_view_without_content_translation_status.yml
   --- a/core/modules/taxonomy/tests/src/Functional/Rest/TermResourceTestBase.php
   +++ b/core/modules/taxonomy/tests/src/Functional/Rest/TermResourceTestBase.php
   
   +++ b/core/modules/taxonomy/tests/src/Functional/Rest/TermResourceTestBase.php
   +++ b/core/modules/taxonomy/tests/src/Functional/Rest/TermResourceTestBase.php
   @@ -200,6 +200,11 @@ protected function getExpectedNormalizedEntity() {
   
   @@ -200,6 +200,11 @@ protected function getExpectedNormalizedEntity() {
              'langcode' => 'en',
            ],
          ],
   +      'status' => [
   +        [
   +          'value' => TRUE,
   +        ],
   +      ],
        ];
      }
    
   @@ -237,7 +242,7 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
   
   @@ -237,7 +242,7 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
    
        switch ($method) {
          case 'GET':
   -        return "The 'access content' permission is required.";
   +        return "The 'access content' permission is required and the taxonomy term must be published.";
          case 'POST':
            return "The following permissions are required: 'create terms in camelids' OR 'administer taxonomy'.";
          case 'PATCH':
   @@ -348,4 +353,13 @@ public function providerTestGetTermWithParent() {
   
   @@ -348,4 +353,13 @@ public function providerTestGetTermWithParent() {
        ];
      }
    
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function getExpectedUnauthorizedAccessCacheability() {
   +    // @see \Drupal\taxonomy\TermAccessControlHandler::checkAccess()
   +    return parent::getExpectedUnauthorizedAccessCacheability()
   +      ->addCacheTags(['taxonomy_term:1']);
   +  }
   +
    }
   

   I love it when a plan comes together 😀

   I think this may be the first time that updating core REST is now slowing down @amateescu!

This needs a reroll since #2987084: Convert EntityReferenceSelectionAccessTest to a kernel test was committed.

This is looking almost ready to go!

1. +++ b/core/modules/taxonomy/taxonomy.install
   @@ -126,3 +128,43 @@ function taxonomy_update_8503() {
   +  $entity_keys['published'] = 'status';
   ...
   +  $definition_update_manager->installFieldStorageDefinition('status', 'taxonomy_term', 'taxonomy_term', $status);
   

   I think we should support the case where a status field already exists and skip the update in that case, as we did in system_update_8501(). We'll need also a similar change record.

2. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyTermUpdatePathTest.php
   @@ -0,0 +1,126 @@
   +    $this->assertTrue($term->isPublished());
   

   Can we also add an assertion to check the term can be unpublished as well?

Latest interdiff looks good to me. #48 is addressed so back to RTBC.

Saving credits and crediting people from #2880149: Convert taxonomy terms to be revisionable.

Since this will allow us to test taxonomy with Workspaces in contrib via the Multiversion module, it's valuable to go ahead and commit this patch even if the parent one does not make it into 8.6.

Pushed 543b3f4 to 8.7.x and backported it as 8081895 to 8.6.x.

Thank you all, see you in #2880149: Convert taxonomy terms to be revisionable!

We still need to update the CR with the instructions to write a manual DB update in case of name collisions.

Is there any disruption or important update information that needs to be mentioned in the release notes, or is it just a highlighted feature? If the latter the tag should be "8.6.0 highlights". Otherwiwse, if there is some important update information please document it here on the issue so the information can be included in the final release notes. Thanks!

That there is no UI yet and it has been worked on in #2899923: UI for publishing/unpublishing taxonomy terms

Just FYI for anyone else that comes across this, I did *NOT* have any status fields on my taxonomy terms, but this change broke every entity reference field to terms on my site for non-admins. Took me about 2 weeks to track this down and figure out the cause.

Essentially the status field was created but with null values, basically preventing anyone without admin access from seeing them.

Since there's no UI yet, what I had to do was create a route that would loop through all terms and run ->setPublished() on them. Which was easy and quick (once I understood the issue) for me, but would be really hard for someone who's not a developer.

Should i install #35 if i am running 8.6x?

Thanks for the clarification amateescu. 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

Ok, thanks again amateescu