- @acbramley opened merge request.
- Status changed to Needs review
over 1 year ago 11:13pm 27 February 2023 - π¦πΊAustralia acbramley
Moving to an MR, rebased against 10.1.x. First pushed a failing functional test and will follow with the rest of the patch. Also updated the IS.
- Status changed to Needs work
over 1 year ago 11:47pm 27 February 2023 The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- Status changed to Needs review
over 1 year ago 11:55pm 27 February 2023 - Merge request !3556Issue #2473873: Views entity operations lack cacheability support, resulting in incorrect dropbuttons β (Open) created by acbramley
- Status changed to Needs work
over 1 year ago 5:12pm 3 March 2023 - πΊπΈUnited States smustgrave
Hiding files as fix has been moved to MR 3556
Moving to NW as there appears to be 1 failure in it though.Did not review yet
- πΈπͺSweden TwoD Sweden
We already have cache metadata for the operations indirectly through their URLs, but most list builders [and the Views Operations field] simply throw it away after evaluating the access condition (at operations build time), so the context of when an operation could be valid or not is lost.
I think this is what the failing test is actually hinting about, a responsibility creep. To fix the failing test I would have assumed the access control handler for the "disable" operation was not returning the proper [user] context. However, adding it there has no effect at all on whether the test passes or fails since that data is thrown away by the list builder when it check if access is currently allowed.
(We try to avoid early rendering, maybe we should also try to avoid early access evaluation in similar situations?)I ran into this when debugging specifically why our cache contexts added to custom entity access control handlers seemed to have odd effects despite the value of the context(s) we included was different for different users, even after applying the latest patch - because contrib's entity module's list builder has the same flaw as core's list builders currently do.
If the list builders (and Views' Operations field) always added the operations instead of evaluating access at the point of building the operation links, and the links preprocess template hook performs the actual access evaluation (storing the result in
$link['#access']
and merging in the cache metadatada into$link['#cache']
), we'd be all good.This would keep access control handlers as the ones responsible for adding the correct access related cache metadata to their responses instead of also having list builders juggle it around, and relieve them of even having to do the operations access checks they do now - they would not have to care about it all about which operations are valid or not.
For now I've had to resort to always include a custom cache context (which varies with anything that could affect access for the different groups of users we have) in the links preprocess hook, otherwise the operations access in the lists/views we have would not get rebuilt at all. If it worked as described above (and entity module always included all of its operations) I would not have had to do anything in our site specific code because the cache metadata from the access handler would have been propagated there instead.
I'll take a stab at refactoring the branch to show what I mean.
- First commit to issue fork.
- last update
about 1 year ago 29,434 pass, 1 fail - last update
12 months ago Custom Commands Failed - last update
12 months ago 29,435 pass, 8 fail - last update
12 months ago 29,440 pass - Status changed to Needs review
12 months ago 7:04pm 8 July 2023 - πΈπͺSweden TwoD Sweden
Now that all tests finally pass, I should write some notes on the changes outside the implementation of #108 itself.
There were at least two major issues discovered while working on this which could potentially be extracted and handled on their own.
I tried searching the issue queue for existing patches and came up blank, but maybe someone knows if they have already been covered elsewhere.The renderer throws away cache metadata from access results if they are not allowed. This, obviously, prevents something like #108 from working; we need that metadata to be bubbled the same was as if there was a cache hit/miss when access is granted. Without this the new
::testEntityOperationsCacheability()
fails because theuser
cache context is dropped.+++ b/core/lib/Drupal/Core/Render/Renderer.php @@ -233,6 +233,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) { if ($elements['#access'] instanceof AccessResultInterface) { $this->addCacheableDependency($elements, $elements['#access']); if (!$elements['#access']->isAllowed()) { + // Abort, but bubble new cache metadata from the access result. + $context = $this->getCurrentRenderContext(); + if (!isset($context)) { + throw new \LogicException("Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead."); + } + $context->push(new BubbleableMetadata()); + $context->update($elements); + $context->bubble(); return ''; } }
(Added
null
to the annotated return types of::getCurrentRenderContext()
as that was missing.)The content translation manager sometimes looks for existing translations on the wrong revision.
This was exposed byContentTranslationRevisionTranslationDeletionTest::testOverview()
failing because the 'Add' operation was now missing for Italian on line 142. They still had access to the translate form if the current interface language was either Italian or French, but not in English. While the Add operation [correctly?] points to the Italian version of the translation form it being added depended on an access check performed on the "active" revision - which already has an Italian translation - so the link no longer show up because the URL is inaccessible.The test performs these steps:
(This is iteration 2 of the test, with the editor role. Iteration 1 passes because access checks are bypassed.)- Create an English published node "Test 2.1 EN"
Verify translations can be added. - Add an Italian draft "Test 2.2 IT".
Verify the draft can not be deleted because it's unpublished. - Publish the Italian translation "Test 2.3 IT"
Verify it can be deleted. - Create an English draft "Test 2.4 EN".
Verify Italian translation still exists. - Delete the Italian translation. Verify the 'Add' operation for Italian reappears.
- Publish the English draft "Test 2.6 EN".
Verify the Italian translation does not reappear. - Add an Italian published translation "Test 2.t IT".
Verify it can be deleted. - Create an Italian draft "Test 2.8 IT".
Verify it can not be deleted. - Delete the Italian draft.
Verify it can be deleted again. - Delete the Italian translation.
Verify it can be added again.
When the Italian translation is deleted in step 5 the English draft "Test 2.4 EN" is the latest revision and no longer has a translation. The access manager gets its argument from the route match for the translation route, which has
load_latest_revision = TRUE
, but it loads the latest "active" revision - which the entity repository says is the last one which was translation affected.
The last English revision which was translation affected (titled "Test 2.1 EN") still had "Test 2.3 IT" as a translation, as the deletion created a new revision without any translations affected.
ContentTranslationController::add()
deals with this by explicitly loading the latest revision, so we can do the same thing in the translation access control manager and it correctly sees that there is no Italian translation there and allows the 'Add' operation.The controller does however cause a similar issue elsewhere by adding the translation to the entity it loads (after the access control manager has allowed the route). If you have the language switcher block enabled the links there are now access checked via the same manager, and it now sees the newly added translation (which isn't considered "new" since an Italian translation did exist earlier) and prevents the links from showing.
Adding aclone
to the controller prevents polluting the entity instance in the context, and the subsequent access checks then see the true "current" state of the entity and lets the language switcher links show.Other changes made to make tests pass include adding
_access: 'TRUE'
to the<current>
route, which caused a problem inCommentNewIndicatorTest<code> because <code>CommentLinkBuilder
uses<current>
for the "comment-new-comments" hidden jump link and renders it as a link list.
I can't think of any reason why this would not be safe off the top of my head, but there's also no other test which directly depends on it it being either allowed or denied.A few places in
FunctionsTest
had similar issues with the access checks but some routes could be switched to existing ones with correct access requirements or replaced with a new one. - Create an English published node "Test 2.1 EN"
- Status changed to Needs work
12 months ago 1:32pm 11 July 2023 The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- π¬π§United Kingdom catch
Splitting those out sounds good and nice one tracking them down!
- πΈπͺSweden TwoD Sweden
Thanks, working on that now.
Do you know what's up with the bot? This is my first time I see it so trying to figure out what it's trying to tell me.
The patch applies fine against 11.1.x which it wants to merge into, but I guess it's testing against 11.x? - πΈπͺSweden TwoD Sweden
This now depends on π Content translation access manager may use the wrong revision Needs work and π The renderer throws away cache metadata from access result if it is not allowed Fixed .
- last update
11 months ago 29,882 pass - Status changed to Needs review
11 months ago 3:35am 26 July 2023 - π¦πΊAustralia acbramley
I've rebased the MR onto 11.x, hopefully it went well π€
- Status changed to RTBC
11 months ago 8:41pm 1 August 2023 - π¨π¦Canada jigarius MontrΓ©al
Just tested it to be working fine with Drupal 10.0.9.
- Open on Drupal.org βEnvironment: PHP 8.2 & MySQL 8last update
11 months ago Not currently mergeable. - Status changed to Needs work
11 months ago 10:52pm 1 August 2023 - π¨π¦Canada jigarius MontrΓ©al
After leaving comment #117, I ran into a new issue. There seems to be a permission anomaly for certain operations. I am marking this as needs work.
For example: With the changes proposed in the merge request at this point, I stop seeing the "Masquerade" operation for user entities even though I'm logged in as User 1. As soon as a I remove the patch, I start seeing it again. Additionally, I implemented a hook_entity_operation() in a custom module and the patch makes that custom operation disappear as well. Removing the patch brings it back.
- πΈπͺSweden TwoD Sweden
Yes, this will need some more rebase work once the issues mentioned in #115 are done.
The changes to the Renderer have been committed to the 11.x branch, but not the 10.x or 10.1.x branches, so if you are using D10 you are likely missing the corresponding change from that issue - which is likely to cause problems like those you describe as cache metadata is lost in some situations.
- last update
11 months ago 29,947 pass - last update
9 months ago 30,379 pass, 2 fail - Status changed to Needs review
6 months ago 1:48am 2 January 2024 - π¦πΊAustralia acbramley
For example: With the changes proposed in the merge request at this point, I stop seeing the "Masquerade" operation for user entities even though I'm logged in as User 1. As soon as a I remove the patch, I start seeing it again.
I just debugged this and it's in fact correct. The masquerade operation's URL route is
entity.user.masquerade
That route has the
_csrf_token
requirement. That gets removed by thearray_diff
in AccessManager::check because there is no request, which the csrf_token requirement needs. - πΊπΈUnited States thefancywizard
I created a 10.1.x-compatible patch from the merge request changes if anyone is in need.
- πΊπΈUnited States smustgrave
Appears to have gone through a few rounds of reviews
All threads appear to have been addressed in the MR. Took a look at the MR and nothing stands out. But seems like a large change so will wait to see if anyone else chimes in.
- Status changed to RTBC
6 months ago 1:08pm 14 January 2024 - π¦πΊAustralia thursday_bw
#117 set this to reviewed and tested by the community.
#118 re-set this to needs work with a vague bug report.
#121 spoke to the bug report and reported it as correct (meaning that 'works as expected' I believe).Therefore setting this back to 'Reviewed and tested by the community'.
I have done some testing and can confirm that MR3556 as of 14 Jan 2024 on resolves the entiry operations problems reported on this issue, and also the username issues reported on Incorrect user linked on content page when switching users β .
- π¨πSwitzerland bircher π¨πΏ
Attaching also a patch for Drupal 10.2.x
- Status changed to Needs work
5 months ago 5:50am 31 January 2024 - π¦πΊAustralia larowlan π¦πΊπ.au GMT+10