TermStorage::loadParents()/getChildren()/getTree() need an option to specify accessCheck(FALSE)

Initial patch

VocabularyStorage::getTopLevelTids() has the same issue.

Hear a Rerolled patch.

Please could you update this:
Determine usage of access check in the further storage queries.
To:
Determine the usage of "access check" in the further storage queries.

Made the change suggested in #11. Please review.

Updating tags. The issue summary is not complete and if this is a bug it will require test cases.

Based on comment from #16, adding optional $access_check parameter.
I've included the kernel test to validate whether taxonomy_term_access_check query tag is added or not.
I think based on that we can identify whether access_check is added.

I haven't included interdiff considering it is different approach altogether.

Let's see how it goes 🤞

#8 can be a follow-up.

+++ b/core/modules/taxonomy/src/TermStorageInterface.php
@@ -39,11 +39,13 @@ public function updateTermHierarchy(EntityInterface $term);
-  public function loadParents($tid);
+  public function loadParents($tid, $access_check = TRUE);

@@ -63,11 +65,13 @@ public function loadAllParents($tid);
-  public function loadChildren($tid, $vid = NULL);
+  public function loadChildren($tid, $vid = NULL, $access_check = TRUE);

Given this is an API addition we can only really make this change in 10.1.x. We can do this under the 1 to 1 interface to implementation rule. However, storage classes are interesting because we know that alternate DB implementations often have to implement them. Therefore I think we should go for Symfony's method of doing this... which is to do this:

public function loadChildren($tid, $vid = NULL /**, bool $access_check = TRUE **/);

on the interface but then implement it on the concrete class. That way we can given alternate implementations time to change the signature in Drupal 10 before enforcing it in Drupal 11.

I also think this should be a task - I've checked the issue comments and I've not seen any reasoning for it being a bug.

We might need to disable a PHPCS rule... see \Drupal\Core\Database\StatementInterface::fetchObject for an example of where we have done something similar.

Addressed point in #28 and #29

Re #28 I looked into contrib, and found that config_terms will indeed be broken if we change this under the 1:1 rule: it provides an alternative storage that extends ConfigEntityStorage but that implements TermStorageInterface, see https://git.drupalcode.org/project/config_terms/-/blob/8.x-1.x/src/TermS...

However, with the commented out parameter, how will we notify config_terms that it needs to change in future to accommodate this? Can the debug classloader help us here?

Ah, I remember where I've seen this before: we explicitly skip this in deprecation-ignore.txt:

%The "Drupal\\[^"]+" method will require a new "[^"]+" argument in the next major version of its interface "Drupal\\[^"]+", not defining it is deprecated%

Sorry dropped the ball here. So is any changes needed to the latest patch?

I think the next steps here @smustgrave is to confirm that the debug classloader reports this as a deprecation when running tests for config terms module.

If it doesn't we'll need to revisit why we have that entry in deprecation-ignore.txt

I will admit I'm not sure how to go about doing that..

Should it be moved to NW then?

Per #36.

I'm not sure how to do that though unfortunately.

I've run into this issue as well while working on the Access policy contrib module

My use case:
In access policy I can define different access rules that allow me to dynamically add conditions via `hook_query_TAG_alter`
I've created a Node access policy with an access rule called "Compare node tags with tags on current user (with depth)". This allows me to restrict content by ensuring the tags on the node either match the tags on the user or are a child of one of the tags on the user.

This works fine for restricting view access. However, it does not work when I attempt to limit what tags are available in the entity reference field on the node edit form. It triggers an infinite loop.

This is because, in order to restrict taxonomy terms, I also add an access policy for those terms. This also invokes hook_query_TAG_alter() but it also calls TermStorage::loadTree()which also calls hook_query_TAG_alter()

This is a contrib module so including a core patch with the contrib module is really not feasible.

So I'm left with a few options:

• Either create a new EntityReferenceSelection plugin just for that field. (Not ideal)
• Or decorate `TermStorage` with a new loadTreeBypassAccess() method. (Also not ideal).

It would be great to get this in.