- Issue created by @kksandr
- last update
almost 2 years ago Patch Failed to Apply - last update
almost 2 years ago Patch Failed to Apply - last update
almost 2 years ago 30,139 pass, 1 fail - last update
almost 2 years ago 30,141 pass, 2 fail - Merge request !4739Issue #3386313: The entity link label formatter does not check URL access. β (Closed) created by Unnamed author
- last update
almost 2 years ago 30,141 pass, 2 fail - last update
almost 2 years ago 30,146 pass - Status changed to Needs review
almost 2 years ago 3:47pm 10 September 2023 - Status changed to RTBC
almost 2 years ago 2:33pm 12 September 2023 - πΊπΈUnited States smustgrave
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
- last update
almost 2 years ago 30,148 pass - last update
almost 2 years ago 30,154 pass - last update
almost 2 years ago 30,161 pass - last update
almost 2 years ago 30,164 pass - last update
almost 2 years ago 30,168 pass - last update
almost 2 years ago 30,205 pass - last update
almost 2 years ago 30,205 pass - last update
almost 2 years ago 30,360 pass 7:16 3:03 Running- last update
almost 2 years ago 30,360 pass - last update
almost 2 years ago 30,371 pass - last update
over 1 year ago 30,379 pass - last update
over 1 year ago 30,377 pass - Status changed to Needs work
over 1 year ago 5:07am 9 October 2023 - π³πΏNew Zealand quietone
@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.
- Merge request !8447Issue #3386313 by kksandr: Fixed access check in entity label formatter β (Closed) created by Unnamed author
- Status changed to Needs review
about 1 year ago 3:49pm 18 June 2024 - Status changed to RTBC
12 months ago 4:15pm 8 July 2024 - πΊπΈUnited States smustgrave
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.
- Status changed to Needs work
12 months ago 4:01am 19 July 2024 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.
- First commit to issue fork.
- Status changed to Needs review
11 months ago 9:13am 14 August 2024 - Status changed to RTBC
11 months ago 11:23am 14 August 2024 - πΊπΈUnited States smustgrave
Missed the additional threads after I marked RTBC but believe feedback has been addressed
- Status changed to Needs review
10 months ago 11:53am 26 August 2024 - Status changed to RTBC
10 months ago 12:28pm 26 August 2024 - π¬π§United Kingdom longwave UK
Similar to the fix in π StringFormatter always displays links to entity even if the user in context does not have access Needs work this needs a change record to explain the behaviour change to users (and tests) that might be relying on the current behaviour.
- ππΊHungary mxr576 Hungary
It may feels copy-paste... because admittedly, it is! :)
The "Label" entity reference field formatter now restricts links for inaccessible destinations β
- π¬π§United Kingdom longwave UK
Thanks, the change record reads well and the code changes look good too.
-
larowlan β
committed 85dc67bf on 11.x
Issue #3386313 by kksandr, mxr576, smustgrave, longwave, catch: The...
-
larowlan β
committed 85dc67bf on 11.x
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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
-
larowlan β
committed e6525b08 on 11.1.x
Issue #3386313 by kksandr, mxr576, smustgrave, longwave, catch: The...
-
larowlan β
committed e6525b08 on 11.1.x
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
I backported this to 11.1.x
Setting to 10;5.x, but also needs backport for 10.4.x
- πΊπΈUnited States bkosborne New Jersey, USA
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.
- ππΊHungary mxr576 Hungary
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.
- πΊπΈUnited States bkosborne New Jersey, USA
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.
- Status changed to Downport
6 months ago 4:10pm 14 January 2025 - π·πΊRussia Chi
- πΊπΈUnited States smustgrave
Think 10.4 has been missed but could still do 10.5
- Merge request !12190Issue #3386313 by kksandr, mxr576, smustgrave, larowlan, longwave, catch,... β (Open) created by smustgrave
- πΊπΈUnited States xjm
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.
- πΊπΈUnited States xjm
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.
- πΊπΈUnited States xjm
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.
- πΊπΈUnited States xjm
I reran the job with the same result. Someone could try it locally?
- πΊπΈUnited States xjm
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.
- πΊπΈUnited States xjm
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. Testing Drupal\Tests\media\Functional\MediaAccessTest
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.
- πΊπΈUnited States xjm
Followup is π Improve EntityReference/EntityReferenceFormatterTest::testLabelFormatter() to fail in an explicit way when the fix is reverted Active .
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!