The entity link label formatter should check URL access

Thank you for including test-only patch.

Only question would be the todo linking to follow up issues but will see if committers think that's an issue.

LGTM

@kksandr, thanks for issue and the MR!

I'm triaging RTBC issues. I read the IS, which is missing several sections. I have restored those in case they are needed and so we have the 'remaining tasks' section. I read the one comment that gives thanks for the test only path and leaves a question for the committer team. I do not see evidence of a code review.

I applied the diff and read the comment with the @todo. The comment should be wrapped correctly and instead of referring to a file it should refer to a class, or method. That said, I then ran the test without the added user to see the failure. That was at /var/www/html/core/lib/Drupal/Core/ParamConverter/EntityConverter.php:149 which also has a todo. And that todo refers to https://www.drupal.org/node/2934192 which is removing the block of code where the test is failing. It seems to me the @todo should point to that issue. Unless, I am missing something. Then, when that is fixed the creation of the user can be removed. (I tried that by applying that patch and removing the creation of the user and the test passed). Also, the anonymous user should only be created in the test where it is needed, instead instead of in the setup.

In testLabelFormatter, the value build[0]['#url'] is always unset. Surely, there should be a test for when it is set.

Setting to needs work for the above.

Seems all the todos have been removed/addressed

https://git.drupalcode.org/issue/drupal-3386313/-/jobs/1896646 shows the current test coverage is still there

The test chunk added in https://git.drupalcode.org/project/drupal/-/merge_requests/8447/diffs?co... believe addressed the test scenario where the URL is not unset.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Missed the additional threads after I marked RTBC but believe feedback has been addressed

One question on the MR.

Thanks that looks better to me.

Similar to the fix in #3336994: StringFormatter always displays links to entity even if the user in context does not have access this needs a change record to explain the behaviour change to users (and tests) that might be relying on the current behaviour.

It may feels copy-paste... because admittedly, it is! :)

The "Label" entity reference field formatter now restricts links for inaccessible destinations

Thanks, the change record reads well and the code changes look good too.

Committed to 11.x
I think it is worth back-porting this to 10.4 for API compatibility and because its a bug.
I don't think we can back port it to 10.3/11.0 because the behaviour change could be disruptive.

Needs re-roll for 10.4.x

Published the change record.

Thanks

I backported this to 11.1.x

Setting to 10;5.x, but also needs backport for 10.4.x

Isn't there a legit use case for still linking to pages a user does not have access to? Like if the entity being linked to is not available to anonymous users, but is available to authenticated users. We'd still want the link to that entity because we can log the user in automatically when they encounter a 403 and are logged out.

Yes, you may want to suggest the user to log in because afterwards they **may** get access to something, but at the same time, you may not want to disclose information about the entity behind the scenes, like the title/label could already hold such information.

So in this cases you actually would like to expose a Call To Action based on a deliberate decision and driving the user.

So in this cases you actually would like to expose a Call To Action based on a deliberate decision and driving the user.

But I can't do that anymore with this change, right? At least, not using this field formatter.

Does this duplicate #3293287: EntityReferenceLabelFormatter returns a link even if the user only have "view label" access?

Think 10.4 has been missed but could still do 10.5

Backport was clean.

I agree that this is a good choice for a 10.5.x backport. We're down to the wire here on the minor and technically the commit freeze has started, but maybe someone can look at it today.

This is major.

I did a code review with the context that this has already been committed to D11. I also compared the 10.5.x MR to the 11.x commit and confirmed they match.

Waiting for the test-only pipeline.

Hm, the test-only pipeline seems to have barfed:

Running with gitlab-runner 18.1.0 (0731d300)
  on gitlab-runner-d5d4547b4-sczcg s8ex1X2yJ, system ID: r_UpW78fmd96Qf
  feature flags: FF_NETWORK_PER_BUILD:true
Resolving secrets
Preparing the "kubernetes" executor
00:00
Using Kubernetes namespace: gitlab-runner
Using Kubernetes executor with image registry.gitlab.com/drupal-infrastructure/drupalci/drupalci-environments/php-8.3-apache:production ...
Using attach strategy to execute scripts...
Using effective pull policy of [] for container database
Using effective pull policy of [] for container build
Using effective pull policy of [] for container helper
Using effective pull policy of [] for container init-permissions
Using effective pull policy of [] for container chrome
Preparing environment
00:30
Using FF_USE_POD_ACTIVE_DEADLINE_SECONDS, the Pod activeDeadlineSeconds will be set to the job timeout: 1h0m0s...
Waiting for pod gitlab-runner/runner-s8ex1x2yj-project-119853-concurrent-0-ndusb59w to be running, status is Pending
Running on runner-s8ex1x2yj-project-119853-concurrent-0-ndusb59w via gitlab-runner-d5d4547b4-sczcg...
Getting source from Git repository
00:04
Gitaly correlation ID: 3900399024
Fetching changes with git depth set to 50...
Initialized empty Git repository in /builds/issue/drupal-3386313/.git/
Created fresh repository.
Checking out b59c089f as detached HEAD (ref is 3386313-backport-10.5)...
Skipping Git submodules setup
Executing "step_script" stage of the job script
00:01
$ [[ $_TARGET_DB == sqlite* ]] && export SIMPLETEST_DB=sqlite://localhost/$CI_PROJECT_DIR/sites/default/files/db.sqlite?module=sqlite # collapsed multi-line command
$ echo "SIMPLETEST_DB = $SIMPLETEST_DB"
SIMPLETEST_DB = mysql://drupaltestbot:drupaltestbotpw@database/mysql?module=mysql
$ $CI_PROJECT_DIR/.gitlab-ci/scripts/server-setup.sh
Starting Apache httpd web server: apache2.
chown: cannot access './vendor': No such file or directory
$ $CI_PROJECT_DIR/.gitlab-ci/scripts/test-only.sh
ℹ️ Changes from 3d80c32dd2e49078f041d144a0a6634a707bcc27
core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceLabelFormatter.php
core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php
core/modules/media/tests/src/Functional/MediaAccessTest.php
If this list contains more files than what you changed, then you need to rebase your branch.
1️⃣ Reverting non test changes
grep: warning: * at start of expression
grep: warning: * at start of expression
↩️ Reverting core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceLabelFormatter.php
grep: warning: * at start of expression
2️⃣ Running test changes for this branch
sudo: ./vendor/bin/phpunit: command not found
sudo: ./vendor/bin/phpunit: command not found
Exiting with EXIT_CODE=1
Running after_script
00:01
Running after script...
$ sed -i "s#$CI_PROJECT_DIR/##" ./sites/default/files/simpletest/phpunit-*.xml || true
sed: can't read ./sites/default/files/simpletest/phpunit-*.xml: No such file or directory
Uploading artifacts for failed job
00:00
Uploading artifacts...
WARNING: ./sites/default/files/simpletest/phpunit-*.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-3386313) 
WARNING: ./sites/simpletest/browser_output: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-3386313) 
WARNING: *.log: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-3386313) 
ERROR: No files to upload                          
Uploading artifacts...
WARNING: ./sites/default/files/simpletest/phpunit-*.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-3386313) 
ERROR: No files to upload                          
Cleaning up project directory and file based variables
00:01
ERROR: Job failed: command terminated with exit code 1

Will try a requeue.

I reran the job with the same result. Someone could try it locally?

I requeued a full pipeline job as per @fjagrlin our test-only job expired after a week. So hopefully with a new pipeline we can run it.

OK, the test-only job worked this time and failed this time, with:

1) Drupal\Tests\field\Kernel\EntityReference\EntityReferenceFormatterTest::testLabelFormatter
Undefined array key "#plain_text"
/builds/issue/drupal-3386313/core/modules/field/tests/src/Kernel/EntityReference/EntityReferenceFormatterTest.php:426
/builds/issue/drupal-3386313/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
ERRORS!
Tests: 8, Assertions: 76, Errors: 1.
PHPUnit 9.6.23 by Sebastian Bergmann and contributors.

That's... not exactly the fail I was expecting, and it also demonstrates that the custom assertion message is not useful because it will never be displayed since it fails on an undefined array key rather than the actual assertion. It would probably be better to assert the existence of it first, I think, since (upon careful review), that is what is being added.

But changing that is out of scope for a backport, so tagging for a followup.

Followup is #3532932: Improve EntityReference/EntityReferenceFormatterTest::testLabelFormatter() to fail in an explicit way when the fix is reverted.

Committed to 10.6.x. I also backported it to 10.5.x as a major bugfix. Changing access behavior is slightly disruptive, but in this case the benefit outweighs the disruption IMO.

Thanks everyone!

Unfortunately, this update removed links of taxonomy term entity references which I have permission to view, but the $uri_access->isAllowed() always returns with FALSE
I still can reach the taxonomy/term/{term-id} pages without any issues and according to the view which handles the taxonomy pages, it gives full access to the taxonomy listing page.
I've been debugging it for a few hours but I haven't found it out yet what's wrong.

UPDATE: it seems I have a conflict with the patch I used from https://www.drupal.org/project/drupal/issues/2778345