🇭🇺Hungary @mxr576

There is a simpler way to handle potential null

mxr576 → made their first commit to this issue’s fork.

Maybe the excerpt generator could also respect the "highlight returned field data" settings and just skip fields that are not returned by the server...

mxr576 → created an issue.

mxr576 → created an issue.

mxr576 → created an issue.

(Original Slack thread with the problem statement: https://drupal.slack.com/archives/C079NQPQUEN/p1744729430123259)

mxr576 → created an issue.

Yeah, well, that is part of the original description which I haven't changed, I have only fixed some typos and links.

That said, it is also true, since unless something changed in contrib since the issue was opened, the only code that calls the function and uses the $field parameter is in the test that proves that issue exists.

have search in contrib and was unable to find any attempt to use a non NULL value for this param.

Functional JS tests fail for no valid reason on the CI, so that can be ignored.

I think the proposed fix for 🐛 file_get_file_references does not return correct results when using $field param Active in https://git.drupalcode.org/project/drupal/-/merge_requests/11831 could also address this problem, please verify if you have time.

I have also bumped into this issue recently and after some forensic analyses my gut says that the $field based filtering was never worked as expected in 8.x.

Back to the history:
* https://github.com/drupal/core/commit/8aa449c8248e87e3f675503be1da462c8c... - nothing to see here, just use DI
* https://github.com/drupal/core/commit/de8bb2efaa607caf1d913288ca160f64c4... - Maybe this is where it all got broken, or even before...?

And we also have this CR related to the last commit: https://www.drupal.org/node/2260037 →

I have proposed a fix for it, I am uncertain at this point if this should be dropped or not, but should be fixed before # 📌 [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service Postponed if it stays.

mxr576 → made their first commit to this issue’s fork.

Tried the latest patch from this module, still applies on Drupal core 10.4.x. I did that after I also checked https://www.drupal.org/project/file_replace → - for my use case, both of them have the same limitation which may worth considering in the scope of the final solution: How to access to the replaced file content in hook_entity_update()? Because currently it is not possible, $file->original is pointing to the same file entity when hook_entity_update() is called and the content of the file is already replaced when that hook is called - so if you would like to run some business logic based on file content changes, you cannot.

Do you have a sense of how far below your previous crawl rate this kicked in at?

My script uses concurrency 10 with Guzzle.

Thanks for fixing it so quickly! I had a workaround in place, will leave it for a time being.

Awesome! Thanks for bearing with me!

So this is not eligible anymore for backporting to 10.5?

Hopefully having this in 11.x means we get some contrib tests against it and they might reveal things we've not considered or otherwise.

We will see, I am only excepting contrib test failures in case they were asserted on something that they should not or not valid anymore.

mxr576 → created an issue.

catch → credited mxr576 → .

Needs review, and feedback welcomed on the wording.

("Please" is removed from the message since the screenshot is taken.)

Leaving 🐛 EntityReferenceSelection handlers does not consider entity view/view label operations Active here as a reference that not everything is perfect in the entity selection handlers either, or precisely probably everything works as expected but better documentation is needed about which type access check should be applied where and how...

It was also concluded some type of access checking cannot be implemented as a query level access check, only as a single entity level access check which means the $entity->access() calls remains a necessity even if they break pagination or may lead to performance issues.

For historical reference, we had the following discussion in the security issue with mainly @berdir and @larowlan - please add your correction to my summary if necessary:

Originally, I have started the security thread with these three questions after opened 🐛 Fix filtering on Node's Authored by field (and on other entity reference fields) Active :

1. Should entity references respect either of these entity operation accesses?
2. What is the intended purpose of the "view label" entity operation if not for controlling label visibility?
3. How should this interact with node access, given that hook_node_access() can override permissions granted by hook_node_access_grants() (Proof: https://git.drupalcode.org/project/view_usernames_node_author/-/blob/1.0.0/tests/src/Kernel/NodeAccessTest.php?ref_type=tags#L94-106)?

TL;DR

• Entity Reference Selection handlers should perform query level access checks and avoid listing entities that a given user does not have access to. This can be achieved through (entity) query alters or via custom selection plugins, see \Drupal\user\Plugin\EntityReferenceSelection\UserSelection::buildEntityQuery() and \Drupal\node\Plugin\EntityReferenceSelection\NodeSelection::buildEntityQuery()
• $entity->access() performs access check on a single entity. When performed on a list of entities, it breaks pagination and could lead to performance issues.
• The "view label" operation was introduced as a subset of the "view" entity operation. The primary use case in core is being able to view user display names without having access to their profile. There is some overlap with field access, but a label does not have to be a field.

In addition: node/entity access is inconsistent. List and entity level access checking in Drupal core is complex, with several open issues addressing these concerns. Developers should be aware of how to use them properly.

Potential action items

During a security team triage meeting (@greggles, @mcdruid, @drumm, @larowlan), the following suggestions were made:

1. Add additional documentation to hook_entity_access() and hook_entity_access_alter() hook documentation, noting the gotchas around selection plugins.
2. Expand the documentation to explain that hook_entity_reference_selection_alter() can be used to modify the class for a given entity-type and add custom logic as needed.

These documentation improvements would help developers, especially those new to Drupal, understand the potential pitfalls related to access controls.

Additional documentation suggestions

Extend the documentation on \Drupal\Core\Entity\EntityAccessControlHandler::$viewLabelOperation to properly explain that "view label" is a subset of the "view" operation. Clarify that even if it grants access to a field/property on an entity or sometimes a dynamically generated value (e.g., $user->getDisplayName()), it is not equivalent to field access.

Explain that when a user has "view" operation access to an entity, they also have "view label" access. However, when they only have "view label" access, they literally only have access to the entity label and nothing more. This also means that they may not have access to the entity's canonical URL (see The "Label" entity reference field formatter now restricts links for inaccessible destinations → ).

mxr576 → created an issue.

Add link to the HTML list of Recipes on Packagist

Could this be caused by #2741937: Removed paragraph entities are not deleted from database → -> ✨ [META] Remove obsolete composite entities from existing storage (garbage collection) Active ?

Thank you, too!

mxr576 → created an issue.

Is this eligible for backporting?

That makes sense. If this change is introduced in a new minor version, couldn’t we also provide an update hook to remove the unnecessary status filter from affected Views? Or would that only be feasible in a new Drupal major release? (The idea came when I checked code examples for the requirement checks and found all these update hooks in \Drupal\views\ViewsConfigUpdater.)

If the plan is to introduce this in 10.4.x and 11.1.x (which I’d love to see happen after all these iterations), then these versions could include the requirements warning, while the subsequent minor versions could handle the removal of redundant filters via an update hook.

Correct role label - this is what currently visible on GCP

Awesome improvements indeed! Maybe announcing the new API worth a change record?

Hi

What is your ETA for releasing a Drupal 11 compatible version from this module?

There was no conflict, so moving back to RTBC.

❯ git fetch origin
remote: Enumerating objects: 7322, done.
remote: Counting objects: 100% (5502/5502), done.
remote: Compressing objects: 100% (1877/1877), done.
remote: Total 2830 (delta 1874), reused 1628 (delta 854), pack-reused 0 (from 0)
Receiving objects: 100% (2830/2830), 719.19 KiB | 10.27 MiB/s, done.
Resolving deltas: 100% (1874/1874), completed with 976 local objects.
From https://git.drupalcode.org/project/drupal
   018087b1ff7..42de5b1df41  10.4.x     -> origin/10.4.x
   0cf4a33c149..60a21fa9287  10.5.x     -> origin/10.5.x
   d3b4dce5a03..ff743d3ce22  11.1.x     -> origin/11.1.x
   6dd918c9360..93ec7a93b73  11.x       -> origin/11.x
 * [new tag]                 10.4.4     -> 10.4.4
 * [new tag]                 11.1.4     -> 11.1.4
❯ git merge origin/11.x
Auto-merging core/modules/node/src/NodeViewsData.php
Merge made by the 'ort' strategy.

I am still catching up with this thread, but...

Example with cache context "entity_status:node|1"

IIRC @kristiaanvandeneynde you have suggested against doing something similar in #2628870-69: Access result caching per user(.permissions) does not check for correct user → -- and fun fact, the code that I copied the comment is available in a public repo since then: https://git.drupalcode.org/project/view_usernames_node_author/-/blob/1.x...

Fix the implementation of node_access_rebuild()'s batch mode to register only one batch set even if the function is called multiple times.

This is what I currently see in the MR.

As an alternative solution - if it turns out that there can be scenarios when running the rebuild process multiple times....

What would be those scenarios realistically? I cannot think of any at this moment... ideally there would be only one node access rebuild, triggered at the very at of the deploy process, only triggered when it is necessary.

Would be nice if you had a readme with examples in there. It's quite a lot of code to read through. Either way, the goal here is to add to AccessResult so that we can slowly move from one approach to another. Call it a band-aid.

+1, example usages and docs will come, I was just excited to have this finally published and shared with internally/externally.

The real question is indeed whether we need to completely rethink AccessResult as a whole, but this issue is not for that.

I think we should merge changes in this issue that already improves the situation and think about how to slowly kill and replace AccessResult in Drupal 14/15.

Test suite failed for unrelated reasons.

There were 2 failures:
1) Drupal\Tests\single_content_sync\Functional\SingleContentSyncImportTest::testSingleImport
Unable to install modules: module 'single_content_sync' is incompatible with this version of Drupal core.
/builds/issue/single_content_sync-3510655/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:505
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:555
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:367
/builds/issue/single_content_sync-3510655/tests/src/Functional/SingleContentSyncBrowserTestBase.php:43
2) Drupal\Tests\single_content_sync\Functional\SingleContentSyncImportUITest::testSingleImportUi
Unable to install modules: module 'single_content_sync' is incompatible with this version of Drupal core.
/builds/issue/single_content_sync-3510655/web/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:505
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:555
/builds/issue/single_content_sync-3510655/web/core/tests/Drupal/Tests/BrowserTestBase.php:367
/builds/issue/single_content_sync-3510655/tests/src/Functional/SingleContentSyncBrowserTestBase.php:43
FAILURES!

Raised an MR againts 2.0.x but the fix also applies to 1.4.x and should be merged to that branch too.

The outcome after the fix is applied.

mxr576 → created an issue.

This issue inspired me to start working on something I had planned for a long time - a generic Binary and Trinary logic implementation for Drupal with built-in cacheability and lazy evaluation. The first version is now available here: https://git.drupalcode.org/project/stdlib/-/tree/1.x/src/Logic?ref_type=...

My motivation was simple: I found myself writing too much code where I either had to use `AccessResult` to propagate a binary/trinary value *with cacheability* or resorted to constructs like this:

* @return array{result: bool, cacheability: \Drupal\Core\Cache\CacheableDependencyInterface}
*   The boolean result with cacheability.

This new approach aims to streamline such logic while maintaining Drupal’s caching best practices and also incorporates DX tweaks and improvements for the sake of higher cache hit rates like what this issue is about. Feedback and contributions are welcome!

Thanks @longwave for the suggestions and @prabha1997 applying them. I have also bumped versions on related CR-s.

Should be ready to be merged, right? :)

Also repeating my comment from the other thread, could be useful here, should not be lost.

Adding #1144644: Enable specifying the collation when creating a database table → because comment 1 explains to potential root cause perfectly (@catch called my attention to it on Slack).

@nico.b told me that my 🐛 User names uniqueness is no longer accent-sensitive Active is actually is a duplicate of this one - I do not know how I did not find this one back at the time, but thanks for intel Nico.

In that issue I had a very similar fix that is being proposed here and because I saw test failures on MR for other database engines than MariaDB and MYSQL, I have also triggered test runs on those db engines here and they failed too. So the proposed fix actually needs work:

core/tests/Drupal/KernelTests/Core/Validation/UniqueValuesConstraintValidatorTest.php
Drupal\KernelTests\Core\Validation\UniqueValuesConstraintValidatorTest::testUsernameUniqueValidation
Failed asserting that actual size 0 matches expected size 1.

core/tests/Drupal/KernelTests/Core/Validation/UniqueValuesConstraintValidatorTest.php:359

(source: https://git.drupalcode.org/issue/drupal-3456964/-/pipelines/433653/test_...)

Yes, this is a duplicate of 🐛 Unhandled exception when trying to register a duplicate username with equality mismatch between php and database layer Active . Thanks for letting me know!

Let's continue in that ticket.

Increasing to major just like the issue that introduced this problem, because user registration is blocked by this.

Just referenced this issue on 🐛 User names uniqueness is no longer accent-sensitive Active because 14 years later I guess we are still suffering the consequence of general_ci collation described in comment #1.

Adding #1144644: Enable specifying the collation when creating a database table → because comment 1 explains to potential root cause perfectly (@catch called my attention to it on Slack).

It seems the fix that was merged caused an unexpected side-effect, referencing the new issue in case anybody from the old crew would like to jump on the new one as well: 🐛 User names uniqueness is no longer accent-sensitive Active .

Since this is not my expert area, I have asked one of the AI-s how to fix the \Drupal\Core\Validation\Plugin\Validation\Constraint\UniqueFieldValueValidator::caseInsensitiveArrayIntersect() implementation:
* \Normalizer::normalize($value, \Normalizer::FORM_NFKD) was suggested but that adds a dependecy on intl: https://www.php.net/manual/it/class.normalizer.php - could not make it work on my ddev local, but did not invested too much time either
* https://packagist.org/packages/wikimedia/utfnormal which introduces a new package dependency, haven't checked
* iconv("UTF-8", "ASCII//TRANSLIT", $text) as a quick solution, currently used in MR.

and this is just one way to address the problem, I am unsure at this moment if the idea behind the \Drupal\Core\Validation\Plugin\Validation\Constraint\UniqueFieldValueValidator::caseInsensitiveArrayIntersect() should be reconsidered after this finding.

FTR, originally we thought this is a collision issue but actually it is not, because the db returned the duplicates.

The root of the issue lies in the database collation, which is set to utf8mb4_general_ci. This collation is both case-insensitive and accent-insensitive, meaning that "René" and "Rene" are treated as the same username. As a result, when the user attempted to register with the username "René," the site encountered an unexpected error because "Rene" already existed in the database.

(Original Slack thread: https://drupal.slack.com/archives/C079NQPQUEN/p1739955491188989)

mxr576 → created an issue. See original summary → .

Wow!

Tbh, I am a bit hesitanr about introducing a new entity operation, although I am fully understanding why this is feels a compelling solutions. Why am I hesitant? Because I have been involved several private conversations about what *view label" is and what not; and I am still not convinced that the majority of devs aware of it.

Would "view linked label" become a generic entity operation or username only? Should it be handled properly by all logic in core/contrib?

What I can also add that I have an extensive test coverage for username access in my module with a test controller that could be also useful foe debugging.

https://git.drupalcode.org/project/view_usernames/-/blob/1.x/tests/modul...

Moving this to needs work since test coverage is missing.

Pipeline on the MR is currently in a failed state, could be due to transient errors but since the build log has expired, I am going to rebase and see what happens.

updated IS

mxr576 → created an issue.

if there is missing entity access check
in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::getReferenceableEntities() - because it seems it only relies on query level access checking...

Indeed, that is the reason.

Chris arrived here sooner than myself :) I was also syncing with my CISO @akosh about your questions,.

So we believe access to the username should be denied by default and only gracefully opened up on a need-to-know basis.

Where would the username of a non-author being ever exposed to non-admins?

Yes, JSON:API as a default answer in Drupal as we explain that in our blog post with @akosh.

Isn't it important to e.g. disclose the author name of e.g. blog posts? I see lot's of cases where the missing author name on blog posts is an issue when someone else wants to give credit when quoting them.

Yes, in certain cases, such as blog posts, displaying the author's name can be useful, especially to enable proper attribution and give credit. However, this functionality should be configurable to suit different use cases and privacy considerations. Additionally, any configuration related to author name visibility must be consistent across all interfaces, including user interfaces (UIs) and APIs, to ensure clarity and alignment with permissions.

View Usernames Node Author → module addresses this challenge effectively. It works alongside View Usernames → , which modifies Drupal Core's behavior to a deny-by-default approach for username visibility. This extension explicitly ensures that when you author content on the site, your username becomes publicly available for attribution purposes, aligning with the principle of controlled, use-case-specific access.

Same thing may apply to many other scenarios, where attribution may be relevant and the username not being perceived as PII.

When attribution is necessary and usernames are used for that purpose, it is important to recognize that usernames are considered personal data if they can identify an individual, even if they are not strictly classified as PII in all contexts. As with the previous point, ensuring configurability and maintaining consistency across all interfaces (UIs and APIs) remains critical to appropriately handle such scenarios.

And if we move on with this, as the user_privacy_cms recipe is fairly simple, would you mind if we installed and configured the 2 extra modules directly from one of the existing recipes rather than adding yet 2 more dependencies?

Sure, it’s relatively small for now, and since we don’t yet know how it might evolve, it could make sense to install and configure the View Usernames modules directly without adding a dependency on the recipe.

Hm, reminder to me to check if there is missing entity access check
in \Drupal\Core\Entity\Plugin\EntityReferenceSelection\DefaultSelection::getReferenceableEntities() - because it seems it only relies on query level access checking... - and that could be a reason why the "Author" autocomplete works interesting in node edit form when View Usernames and friends are enabled.

The MR was still mergeable, but merged 11.x to keep the MR up to date.

Add User Privacy Core → and User Privacy CMS →

Configure entity fields by marking those with personal data as sensitive

Would be a real invention! Usernames should be marked as sensitive by default. See: 🌱 [policy] Treat username enumerations as security bugs that require Security Advisories Active

mxr576 → created an issue.

mxr576 → created an issue.

... and now these modules are also available in Recipes.

https://www.drupal.org/project/user_privacy_core →
https://www.drupal.org/project/user_privacy_cms →

mxr576 → created an issue.

mxr576 → created an issue.

Good catch and regular issue, although the fix leads to the infamous N+1 problem of ORMs

I would rather recommend implementing something like I did in an other module, load multiple with a maximum amount:

https://git.drupalcode.org/project/view_usernames_node_author/-/blob/1.0...

(tricks like this is necessary until a stable solution lands in Core for #2577417)

Related Slack thread: https://drupal.slack.com/archives/C1BMUQ9U6/p1735897088083149

Also attached the textual representation created via Slack dump.

AI summary:

Participants discuss the problems, including:

1. **State Service Issues**: It has become problematic to access the `state` service during container build time due to caching mechanisms and service dependencies.

2. **Race Conditions and Alternatives**: Mention of related issues like race conditions in development, alternative storage options like key-value storage, and container parameters for performance and minimal complexity.

3. **Testing and Failures**: Observations that the update system and session storage break when `state` is used in compiler passes. Failed tests highlight specific issues triggered by `state` usage.

4. **Recommendations and Challenges**: Advice is given to use key-value storage cautiously and ensure cache clearance. However, achieving this in compiler passes presents challenges due to potential outdated information being cached.

5. **Codebase Observations**: A null return from `$this->requestStack->getCurrentRequest()` was identified as a potential unhandled issue in Drupal core, exacerbated by using `state` in compiler passes.

6. **Discussion on Feasibility**: Some participants recommend changing direction entirely due to the complexities and risks involved.

Could you please also opt-in tokenuuid to SA coverage?

This project is not covered by Drupal’s security advisory policy.

mxr576 → created an issue.

Removing the tag, since this has not been merged before 10.4.0 got released. :-(

🐛 The content overview Views view filters out unpublished content Active is still on RTBC, but hopefully the final-final-final stab was made on it minutes ago.

A related Slack thread: https://drupal.slack.com/archives/C079NQPQUEN/p1733181222760849