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.
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.
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
... 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 →
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
The MR still applies.
Thanks all, especially @mxr576 for sticking with this one.
:beers:
A backport to 10.3.x or 11.0.x would also require a backport of ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review which as a task isn't eligible for backport, so marking this as fixed.
Does this highlight an edge case in the backporting policy? Here’s the dilemma:
This fix cannot be backported to 11.0.x due to its dependency on a non-backportable issue. As a result, the upgrade path from 10.4.x/10.5.x to 11.0.x could lead to unexpected and unforeseeable feature or bugfix losses unless projects skip directly to 11.1.x.
10.4.x backport is ready in the related MR.
🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work can be only backported to 11.0.x is this also gets bacported.
It seems ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review has not been backported to 11.0.x yet, so that should be backported first.
Can we solve this by implementing a \Drupal\symfony_mailer\Processor\EmailProcessorInterface and enabling the bypasser in build() and deactivating it in postSend()?
Exciting news! Thanks for everyone who got involved, this is an important step to fix the dependent issue.
Thank you to everyone following this issue and to those who join the conversation in the future!
We’ve just published a blog post that explains why we believe this topic is critically important. The post also serves as a release announcement for two contributed modules we developed in-house to address this challenge:
These modules do not require any API changes in Drupal core but aim to provide immediate, practical solutions. Ideally, a comprehensive solution would involve robust new APIs for managing personally identifiable information (PII), including but not limited to usernames.
We hope these contributed modules can help move the discussion forward and pave the way for the necessary enhancements in core.
Green again! Kept it on RTBC...
Rebased again, due to random test failure fixed in upstream: https://www.drupal.org/project/drupal/issues/3478621#comment-15875919 📌 Add a filecache-backed attribute parser Active
Looks good so far, we only have those changes in the MR that we should be, no changes on ResourceTestBase that was fixed in the spin-off issue.
❯ git rebase origin/11.x
Auto-merging core/.phpstan-baseline.php
Auto-merging core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
CONFLICT (content): Merge conflict in core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
error: could not apply 07987b49a5... Cherry-picked "Improve Dynamic Page Cache header assertions in JSON:API tests"
I am going to use git merge origin/11.x instead of rebase in a hope that we do not need to restart the review process here.
Now that ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review is in 11.x, this needs a rebase because the MR does not apply anymore, because it had a cherry picked version. Maybe I just drop the cherry picked commit and see what happens...
+ langcode:
+ type: string
+ label: Language
Do we need to update existing plugin instances in an update hook due to this config schema change? Will the stored config schema remain valid?
Ack, thanks for the clarification.
Moving back to needs work since the "needs test" tag is on the issue and I haven't seen any test coverage in MR#3891 so far.
isNew() can be tricked, it most likely does not help PHPStan to narrow down the actual type of $node->id(), but I agree, let's follow the Drupal Way here -- funny, I wanted to use that originally, but I ditched the idea for some reasons, I hope not because tests started to fail :)
public function isNew() {
return !empty($this->enforceIsNew) || !$this->id();
}Yes they were random failures, restart make them disappear.
Test fails seem to be random ones, so if you can ping @bbrala or @larowlan for a final approval then it's RTBC in my book.
+1
No longer a Starshot blocker now that WASM is out.
For other curious ppl like /me who would like to what this means and why: 📌 Remove the WebAssembly trial Active
Thanks for the additional feedbacks, I have pushed back on some with answers and accepted one. Back to review
mxr576 → created an issue.
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.
> I was able to open the unpublished node in view mode, but attempting to edit it triggered the error.
This behavior is indeed possible because the entity parameter converter basically relies on `Node::load()` with the given node ID, rather than performing an entity query that would respect query-level access control (`node_access`). Specifically, entity access handler and even `hook_entity_access()` implementation can grant access to a node even if hook_node_grants()/hook_node_access_records() does not. In practice, permissions like "view own/any unpublished node" are evaluated earlier than node access checks. If access is granted at this stage, node acess won’t be called.
And I assume the path based access checking relies on entity query... but I haven't checked.
I think it should be fairly simple setting up a failing test in this issue as a next step. That should also eliminate questions like which contribs or other Drupal patches were contributed to this problem on this and that project.
Based on what I have read so far my Spidey-senses say that the issue could be related to hook_node_grants()/hook_node_access_records() because when an node access module (like view_unpublished) is enabled then access control logic applied to nodes changes "drastically" on a site. Nodes that the current user does not have (view) access gets filtered out from query results so Node::load()/Node::loadMultiple() does not even get those to load. The current repro steps also does not say anything about whether the given user had _view_ access to the unpublished node or not, it just say they had edit access, which is an other clue for me, especially since I dealing with node access related wtf for a long time (
✨
Grant query level access to own unpublished nodes
Active
).
Steps to reproduce
* Create a role with permissions to edit nodes.
* As a user with that role, create a new node with a path alias
* Edit the node again and remove the "Published" checkbox. Try to save.
Should be outdated since this was fixed in https://github.com/drupal/core/commit/182399dd9a73ca6ad05d7aeae4b2d6c9c0...
There are no merge conflicts atm but this branch is behind HEAD with 100+ commits, so I am rebasing to ensure whoever reviews the latest changes can review an up to date state.
#goForTenDotFourDotZero
🐛 hook_node_grants implementations lead to a 'URL Alias' validation error when saving translated nodes. Fixed should have fixed this, or should not? 10.2.3 is the first release with that fix.
So I don't think we can remove or warn about its usage.
+1
For changing the world, please join this other issue to brainstorm on how to make node grants and entity access work better together (less confusing to use): https://www.drupal.org/project/drupal/issues/3452449 ✨ Grant query level access to own unpublished nodes Active
+1 on #17, we should not reinvent the super admin definitionan. Maybe your findings in #18 means that the is admin parameter can be marked as deprecated and removed in D12.
What about user selection widgets with autocomplete? They use to show the username
Those are tricky ones indeed. Most likely they can only list usernames of users that the given user (current user) has view access to their usernames. For stored values, when the given user (current) user does not have access to view the other user's username it must display a placeholder (or just the user id, this is what we have in our solution).
The outcome of this issue could make the following one "closed, won't fix".