Insufficient cacheability information bubbled up by UserAccessControlHandler

I'll submit a patch for this later today.

If the intent was to manually optimize 'user.permissions' into 'user', don't. VariationCache needs all of the cache contexts, even if they will eventually be optimized away because, during said optimization (cache context folding), extra cache tags might surface that need to be added to the cache entry.

So in this case, the first if-statement can vary by user.permissions, the second by ['user', 'user.permissions'] and the third one too. Again, do not manually optimize this, you're introducing bugs and potential security issues.

mxr576 → changed the visibility of the branch 3452426-insufficient-cacheability-information-with-accessresult-orif to hidden.

Changes look great, would RTBC but I think we should explain why the seemingly unrelated tests were changed. If you could add a few notes with your findings there, then all good to me.

why the seemingly unrelated tests were changed. If you could add a few notes with your findings there, then all good to me.

I doubled check it and I haven't found any unrelated tests, those are test base classes that used by the user related REST/JSONAPI endpoint tests.

Sorry I should have been more specific: Test code that shouldn't at first sight have been affected.

if ($sparse_fieldset === NULL || !empty(array_intersect(['mail', 'display_name'], $sparse_fieldset))) {

Could you briefly explain why this is needed now?

Could you also elaborate on the mitigating lines? Why weren't they needed before and are they now?

What about the fix you copied from REST?

What about the fix you copied from REST

I assume you were referring to this one, which was copied from JSONAPI: https://git.drupalcode.org/project/drupal/-/merge_requests/8285#note_321681

It seems there are no tests in REST - which concerns me - that would have hit the simple "let's normalize user.permission user to user" requirement so far.

Thanks, looks good to me.

Looks good to me too, both the actual fixes and trying to keep the scope appropriate vs. the other pre-existing and newly opened issues. Also thanks for the annotations on the MR for the test changes, I probably would have asked about at least one so saved a round trip of issue statuses.

Committed/pushed to 11.x, cherry-picked to 11.0.x, 10.4.x, 10.3.x, thanks!

catch → closed merge request !8285

Automatically closed - issue fixed for 2 weeks with no activity.