Taxonomy load functions filter for access control

This will break TAC. Period. :)

Also this is a BC break generally; I don't understand what the point of this patch against 7.x is?

I'd like to mark this wontfix unless there's an actual manifest bug here.

No, it doesn't ;) That there is no test coverage of term_access doesn't mean that it can be removed, see #1947270: Add test coverage for the term_access query tag.

Yes, the term_access in buildQuery() was removed because that was incorrect. The issue above was opened as a follow-up when we removed it from there.

Those other methods aren't simple loader functions that load based on id's. They're query + load, so they have to respect term_access. There is no other way to apply access control for those. If other places don't, then that needs to be fixed.

8: drupal-2073663-taxonomy-term-load-8.patch queued for re-testing.

Just unassigning issues that haven't been developed for a bit in the D8 queue.

I had a discussion with @xjm regarding this task.

I believe this is not a bug, but rather a new task. Indeed, the taxonomy load functions in Drupal 8 (Version 8.0.x - development) use the term access tag when loading terms. Given below is one example of such a load function. Hence, changing the type of this issue to task.

/**
   * {@inheritdoc}
   */
  public function loadTree($vid, $parent = 0, $max_depth = NULL, $load_entities = FALSE) {
    $cache_key = implode(':', func_get_args());
    if (!isset($this->trees[$cache_key])) {
      // We cache trees, so it's not CPU-intensive to call on a term and its
      // children, too.
      if (!isset($this->treeChildren[$vid])) {
        $this->treeChildren[$vid] = array();
        $this->treeParents[$vid] = array();
        $this->treeTerms[$vid] = array();
        $query = $this->database->select('taxonomy_term_field_data', 't');
        $query->join('taxonomy_term_hierarchy', 'h', 'h.tid = t.tid');
        $result = $query
          ->addTag('term_access')
          ->fields('t')
          ->fields('h', array('parent'))
          ->condition('t.vid', $vid)
          ->condition('t.default_langcode', 1)
          ->orderBy('t.weight')
          ->orderBy('t.name')
          ->execute();
        foreach ($result as $term) {
          $this->treeChildren[$vid][$term->parent][] = $term->tid;
          $this->treeParents[$vid][$term->tid][] = $term->parent;
          $this->treeTerms[$vid][$term->tid] = $term;
        }
      }

      // Load full entities, if necessary. The entity controller statically
      // caches the results.
      $term_entities = array();
      if ($load_entities) {
        $term_entities = $this->loadMultiple(array_keys($this->treeTerms[$vid]));
      }

      $max_depth = (!isset($max_depth)) ? count($this->treeChildren[$vid]) : $max_depth;
      $tree = array();

      // Keeps track of the parents we have to process, the last entry is used
      // for the next processing step.
      $process_parents = array();
      $process_parents[] = $parent;

      // Loops over the parent terms and adds its children to the tree array.
      // Uses a loop instead of a recursion, because it's more efficient.
      while (count($process_parents)) {
        $parent = array_pop($process_parents);
        // The number of parents determines the current depth.
        $depth = count($process_parents);
        if ($max_depth > $depth && !empty($this->treeChildren[$vid][$parent])) {
          $has_children = FALSE;
          $child = current($this->treeChildren[$vid][$parent]);
          do {
            if (empty($child)) {
              break;
            }
            $term = $load_entities ? $term_entities[$child] : $this->treeTerms[$vid][$child];
            if (isset($this->treeParents[$vid][$load_entities ? $term->id() : $term->tid])) {
              // Clone the term so that the depth attribute remains correct
              // in the event of multiple parents.
              $term = clone $term;
            }
            $term->depth = $depth;
            unset($term->parent);
            $tid = $load_entities ? $term->id() : $term->tid;
            $term->parents = $this->treeParents[$vid][$tid];
            $tree[] = $term;
            if (!empty($this->treeChildren[$vid][$tid])) {
              $has_children = TRUE;

              // We have to continue with this parent later.
              $process_parents[] = $parent;
              // Use the current term as parent for the next iteration.
              $process_parents[] = $tid;

              // Reset pointers for child lists because we step in there more
              // often with multi parents.
              reset($this->treeChildren[$vid][$tid]);
              // Move pointer so that we get the correct term the next time.
              next($this->treeChildren[$vid][$parent]);
              break;
            }
          } while ($child = next($this->treeChildren[$vid][$parent]));

          if (!$has_children) {
            // We processed all terms in this hierarchy-level, reset pointer
            // so that this function works the next time it gets called.
            reset($this->treeChildren[$vid][$parent]);
          }
        }
      }
      $this->trees[$cache_key] = $tree;
    }
    return $this->trees[$cache_key];
  }

Thanks @aditya.ghan!

(Updating issue credit for the major triage and adding the relevant tags, since we've verified the issue does still exist.)

This needs a major issue summary update, since the issue has been completely inverted from taxonomy_term_load() checking access in 6.x or 7.x (which should never have been the case), to taxonomy_get_tree() checking it (which has to be the case, but I can see a use case for getting the full tree in API calls without access checking).

Just updating the title for now.

I've confirmed that the tag is added for loadParents(), loadChildren(), loadTree(), and getNodeTerms() in D8, as well as on the relevant Views plugins, but not for loading taxonomy terms themselves.

Based on the fact that this cannot be backported in a BC way, and that it has been fixed in D8, I'm actually going to mark this wontfix against D7. If we want a followup for what @catch describes we can do that in another issue I think, since the current summary is confusingly out of date. Tagging for that, just in case.