🇭🇺Hungary @mxr576

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.

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

Should be outdated since this was fixed in https://github.com/drupal/core/commit/182399dd9a73ca6ad05d7aeae4b2d6c9c0...

See #3030477: Views filter "Published status or admin user" not checking "View any unpublished content" permission →

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".

Resolved via https://git.drupalcode.org/project/key_apigee/-/commit/5e11371436dbe19e2... and https://git.drupalcode.org/project/key_apigee/-/commit/17beed4e5b23e92ed....

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!

mxr576 → changed the visibility of the branch master to hidden.

mxr576 → created an issue.

mxr576 → created an issue.

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.

mxr576 → created an issue.

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.

mxr576 → created an issue.

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 →

mxr576 → created an issue.

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.

mxr576 → created an issue.

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! :)

greggles → credited mxr576 → .

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?

Thanks!

First: Why would you expect that? It's not how Composer works unless the other packages have declared tighter constraints. If you get the error message, you'll figure out that you need to specify a version. Speaking of tighter constraints....

At the very least, these packages should be tied together at the minor version level. Based on past experiences, it might even be beneficial to tie them at the patch level as well. For more details, see this Slack thread about how the 10.2 -> 10.3 upgrade broke tests because Composer installed incompatible versions of Drupal core packages: https://drupal.slack.com/archives/C223PR743/p1719331712444049

Drupal core is a monorepo that is only partially managed as such. If we could guarantee that split packages in the subpackage repositories are always compatible with each other, this problem wouldn't exist—but we can't.

The drupal/core-dev package defines the dependencies needed to run tests, while drupal/core contains the tests that might need adjustments as core-dev dependencies evolve... which is not a problem in a monorepo because update and adjustments could happen in the same MR.

However, the butterfly effect is more visible for downstream projects that require drupal/core-dev, and they often only realize changes are needed when a minor (or, in the worst-case scenario, a patch) update to drupal/core-dev breaks their test suite.

Clean up GIT history in the MR, now we just need to figure out why the remaining tests fail...

Yep, those three could be still automated. https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...

This should be it... now we have 3 meaningful commits in the branch.
https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...

I may saw some other areas in ResourceTestBase still with hardcoded Dynamic Cache Header values... checking those.

Opened ✨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review for the improvements that I made on \Drupal\Tests\jsonapi\Functional\ResourceTestBase1 to ensure the test suite is less fragile and more intuitive to use in context of Dynamic Page Cache assertions.

I am going to reorganize existing changes in MR#8198 to separate changes that belongs to here from those that belongs to 3473374.

mxr576 → created an issue.

Copy-pasting from Drupal Slack:

mxr576
Today at 12:49 PM
@acbramley
Maybe you will find some inspiration in my MR for fixing these test failures. testRevisions() killed my nerves so many times...
https://git.drupalcode.org/project/drupal/-/merge_requests/8198

mxr576
Just now
Pushed those changes from the other MR to yours, you can decide if you keep those or not. If we need these here as well either my PR should be merged first or these changes on ResourceTestBase has to be merged independently.
Test went far further now (previously it failed on core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php:2930), but it is still going to fail on something that should be work...
I also had to commit two changes in a dedicated commit over the ported changes and I would like to understand why those were necessary.
Generally speaking, the changes on core/modules/media/src/MediaAccessControlHandler.php because that makes many things uncacheable by adding the user context.
The ported changes on ResourceTestBase only warrants that assertions are aligned with expectations and not hardcoded on the header value.

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

No blockers anymore! \o/ just need to rebase this branch tomorrow.

Thanks @catch for the merge and delaying with opening the follow up issue.

I haven't opened it yet, I was consider it, but that issue is two folded, one is the sub-request, but the first blocker is \Drupal\Core\PageCache\RequestPolicy\CommandLineOrUnsafeMethod that makes POST requests uncacheable dynamic cache. (IMO does not change anything on this https://www.drupal.org/node/3420901 → )

https://git.drupalcode.org/project/drupal/-/merge_requests/8221#note_368194

So I can register this finding as an issue, but that is going to be blocked by making POST requests cacheable by Dynamic Page Cache, which I do not know yet if it is on the roadmap or not.

Gitlab complained about a merge conflict so I had to rebase, No manual steps were needed locally.

Added the latest version of https://git.drupalcode.org/project/drupal/-/merge_requests/8221 to the top of the commit change.
https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...

@quietone Thanks for your review, back to you!

I have also updated Unpublished nodes are no longer hidden on Content overview page when a node access module is enabled → CR and created "Published status or admin user" Views filter becomes inactive when a node access module is enabled → CR to ensure granular and precise notification about the change.

Unrelated FunctionalJS failures, let's see if rebase from master helps...

It did.

Unrelated FunctionalJS failures, let's see if rebase from master helps...
https://git.drupalcode.org/issue/drupal-3449181/-/pipelines/277201/test_...

Asked the UX team on Slack if we still need the "usability" tag on this issue after #35.