This is a followup of #2824640: Views cached results are not taking full cacheability metadata into account.

When performing access checks we are returning a boolean value to indicate the result of the check. We have inherited this approach from the Drupal 7 version of the module, but in Drupal 8 the result of an access check should be contained in an AccessResult object.

The problem with the Drupal 7 approach is that we only communicate the result of the check: access is either granted or denied. In doing so we are losing all the cacheability metadata of the entities that are involved in determining the access. In effect we are losing all information regarding the cache tags, contexts and max age of the objects that determine access.

CommentFileSizeAuthor
#7 3058300-7.patch7.61 KBdrunken monkey
#7 3058300-7--interdiff.txt6.28 KBdrunken monkey
#2 3058300-2.patch7.06 KBpfrenssen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pfrenssen created an issue. See original summary.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen as a volunteer commented
FileSize
3058300-2.patch7.06 KB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented
Status: Active » Needs work 
+++ b/src/Datasource/DatasourceInterface.php
@@ -137,9 +137,28 @@ interface DatasourceInterface extends IndexPluginInterface {
+   * @deprecated in search_api:8.x-1.13 and will be removed from

/s/1.13/1.14/

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented

Should we do this patch before #2824640 to make it simpler or can we do this later?

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen as a volunteer commented

No this is now completely decoupled from #2824640: Views cached results are not taking full cacheability metadata into account and can be worked on independently.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented

Good to know, the deprecation message should mention the correct version, otherwise +1.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Component: General code » Framework
Status: Needs work » Needs review
FileSize
3058300-7--interdiff.txt6.28 KB
3058300-7.patch7.61 KB

Thanks a lot for splitting this off and reposting it here!
I just updated the deprecation version and fixed a few other nit-picks. Please test/review, then I guess we can already commit. (Though it’s a bit weird that we’re now completely ignoring this newly available information. But yeah, taking care of that in the other issue, of course.)

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ at Calibrate commented
Status: Needs review » Reviewed & tested by the community

Looks solid. Thanks for fixing the nits :)

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Great to hear, thanks for reviewing!
Committed.
Thanks again, everyone!

Status: Fixed » Closed (fixed)

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