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".
Thanks for the review @larowlan!
Thanks for the review, let me start with a the rebase since there is work to be done it seems.
❯ git rebase origin/11.x
Successfully rebased and updated refs/heads/3278759-access-cacheability.
Roger, thank you!
Thanks for the review!
Let me start with a no-brainer rebase to ensure this MR remains up to date.
❯ git rebase origin/11.x
Successfully rebased and updated refs/heads/3473374-improve-resourcetestbase.
No conflicts.
however the content did not present. It wasn't until we removed the "Content Published status or admin user" filter and re add it did the content get presented as normal.
My questions:
* If the patch is to remove the additional Criteria on the queuery, why after apply the patch does the content not show up?
* Why does the content show up after I remove and re add the filter?
My gut says that this is some kind of stale cache or the lack of cache clear between the patch is being applied and site was visited. But as @morvaim said, some more context could came handy for reproducing this, for example, the source of the View block.
Currently I do not believe that the reported issue is blocking the merge of the MR.
kewl! This is fixed, now only crediting is missing and issue can be closed.
Thank you!
Failed to clone https://github.com/composer/installers.git via https, ssh p
rotocols, aborting.
- https://github.com/composer/installers.git
Cloning into bare repository '/root/.composer/cache/vcs/https---github.co
m-composer-installers.git'...
fatal: unable to access 'https://github.com/composer/installers.git/': Fa
iled to connect to github.com port 443 after 130169 ms: Couldn't connect to
server
- git@github.com:composer/installers.git
Cloning into bare repository '/root/.composer/cache/vcs/https---github.co
m-composer-installers.git'...
error: cannot run ssh: No such file or directory
fatal: unable to fork
Ahh, so previously the pipeline failed due to transient errors.
Interesting, there were no conflicts. Back to Needs review.
Thanks for your feedback @nathanhealea, I have asked my colleague to try to reproduce the issue you reported. Thanks to the test coverage in the MR I am quite confident that the solution works, but there is always a chance for unknown issues.
❯ git checkout -b '3449181-content_overview_access_fix' --track drupal-3449181/'3449181-content_overview_access_fix'
Branch '3449181-content_overview_access_fix' set up to track remote branch '3449181-content_overview_access_fix' from 'drupal-3449181'.
Switched to a new branch '3449181-content_overview_access_fix'
❯ git fetch origin
remote: Enumerating objects: 2408, done.
remote: Counting objects: 100% (1905/1905), done.
remote: Compressing objects: 100% (743/743), done.
remote: Total 960 (delta 575), reused 516 (delta 178), pack-reused 0 (from 0)
Receiving objects: 100% (960/960), 523.33 KiB | 1.27 MiB/s, done.
Resolving deltas: 100% (575/575), completed with 285 local objects.
From https://git.drupalcode.org/project/drupal
4e0ce97be7..75ea8e8bf9 10.2.x -> origin/10.2.x
72b94a8a0e..b53484abb1 10.3.x -> origin/10.3.x
d2ca6921a8..7bbceff8c4 10.4.x -> origin/10.4.x
0701817796..70c1b4f59a 11.0.x -> origin/11.0.x
49a3c28eaf..2ad947f2f3 11.x -> origin/11.x
❯ git rebase origin/11.x
Successfully rebased and updated refs/heads/3449181-content_overview_access_fix.
Also, it looked like page cache became more chatty in X-Drupal-Cache since that change and probably this was the reason behind those changes in ResourceTestbase originally, but they weren't aligned with the original design,see my previous comment.
A recently merged changed the original behavior in the assert function, including what false means. More details are here:
https://git.drupalcode.org/project/drupal/-/merge_requests/9465#note_386330
So I decided it is better if I clean this up as well to avoid future confusion and replace FALSE|STRING with NULL|STRING.
Rebased and brought in latest changes from [##3473374] which also had to be rebased, but we found other regressions in 11.x in that: https://git.drupalcode.org/project/drupal/-/merge_requests/9465#note_386330
Moving back to needs review due to additional changes performed after rebase.
❯ 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 5ae7c67a39... Introduce generateDynamicPageCacheExpectedHeaderValue
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Rebase only impacted ResourceTestBase but due to this inherited problem more changes became necessary. all changes since the previous RTBC on ResourceTestBase are here: https://git.drupalcode.org/project/drupal/-/merge_requests/9465/diffs?st...
Tests to be written in a later phase.
But still in this issue, am I right?
LGTM
mxr576 → made their first commit to this issue’s fork.
Left a suggestion.
I also checked and it seems to me that the modified code has no test coverage, so there is nothing to extend. Covering TextField processor with tests should be a dedicated issue/effort.
Hiding the old patch.
MR failed on automated qa, back to needs work.
Hiding the redundant patch then.
(It looks like UUID vs revisioning supported is tracked under this Drupal core issue: ✨ Add UUID support for entity revisions Needs work . Bumped that with a quote from comment 9.)
Thank you for reporting this; turns out Paragraph entities do not have proper revisioning support: as Paragraph items are referenced not only by their entity ID, but also their revision ID, and revisions do not have UUIDs, there is no way to properly import references to Paragraphs' revisions. Because of that, even exporting Paragraph revisions is useless.
Based on this comment in #3421812-9: Support exporting/importing revisions → , Paragraphs and import/export solutions especially could also benefit from UUID support in revisioning.
It may feels copy-paste... because admittedly, it is! :)
The "Label" entity reference field formatter now restricts links for inaccessible destinations →
I do not think there is a use case for unsetting, only overriding.
Let's see if this idea and solution stands before test writing.
I take the opportunity that after so many back and forth code reviews with Kristiaan, mark this as RTBC. My changes since his last changes mainly impacted failing tests after his changes, that were changed by me previously.
My changes after Kristiaan's changes: https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?st...
Did a check on the changes and I generally agree with those. I hope nobody is going to raise scope creep just because for consistency reasons not on the view operation was touched.
There are some tests that needs to be fixed, I am going to working on that.
The issue here is that we're dealing with a passed in account rather than the current user so any user.foo cache context is technically not correct here.
YES...YES...YES!!! And I always feel bad when I have to bubble up cache per current user in anyways in an entity access handler method just to ensure things "do not get broken" but semantically that is completely incorrect. That said, this is what we have currently... :(
Thanks for the review, to be honest, I have lost a bit now... :) Could you please commit the expected change to the MR?
And before you do that, let's remember that we try to avoid cache per user as long as we can, this is the reason why user.roles:antonymous or user.roles:authenticated (in the current version) s the primary driver to differentiate access for logged in and non-logged in users instead of returning something that varies per user. We only do this as long as we can, so when a node_access module enabled than we have add user.node_grants: that basically makes every result vary by user in a different way.
✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review got an RTBC \o/
Could you RTBC this then? Thanks!
Thanks @bbrala! :)
user.roles:authenticated context bugs me since the latest change... maybe I can delay that and first check instead whether the user has the given permission, if yes, I can double check whether it is anonymous or not and only add that context, otherwise just vary per permissions.
Do you have concrete suggestions for code adjustments? I know those new methods will bring improvements but with the current tooling I tried hard in the latest commits to return as early as possible, but also keeping collected cacheability instead of ignoring them. But the chance is there that something can be still further improved
I see a new container_rebuild_required in the MR#6290, that needs a change record, maybe a documentation update about info.yml keys as well.
May I recommend this feature to 10.4.0 release notes?