While looking at #2287071: Add cacheability metadata to access checks I noticed a few glitches and a wrong OR in the interface AND documentation. Also, I ran into some documented usages of:

// No opinion.
return AccessResult::create();

Having to add a comment to each of that calls shows that it misses a nicely named one. So this quickly adds AccessResult::noOpinion() as well, what is in particular important as this is the new required default return value for all entity access hooks.

CommentFileSizeAuthor
#9 interdiff.2341399.8.9.txt1.61 KBduaelfr
#9 2341399-9.patch7.32 KBduaelfr
#8 2341399-8.patch6.12 KBrpayanm
#3 d8_access_result.patch4.84 KBfago
d8_access_result.patch9.36 KBfago

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

See also #2340507: Make the new AccessResult API and implementation even better

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Closed (duplicate)

Yeah let's mark this as a duplicate.

fago’s picture

fago
Primary language German
Location Vienna
commented
Title: Follow-up: AccessResult documentation fixes and DX improvement » Follow-up: AccessResult documentation fixes
Status: Closed (duplicate) » Needs review
StatusFileSize
new d8_access_result.patch4.84 KB

ah, thanks. Then I'm re-scoping this to the docu-fixes at those are not taken care in the other one afaiks.

chx’s picture

chx commented

Almost. The orIf/andIf doxygen is definitely fixed (mostly rewritten from scratch :P )

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented

Is this still needed? I'm confused by #4.

chx’s picture

chx commented

What I meant in #4 is that the orIf / andIf documentation fixes are no longer relevant. The rest might be needed; needs probably a reroll to see what applies still.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Priority: Major » Normal
Status: Needs review » Needs work
Issue tags: +Needs reroll, +HappyDays1506, +php-novice, +Novice 
  1. +++ b/core/lib/Drupal/Core/Access/AccessResultInterface.php
@@ -64,7 +64,7 @@ public function orIf(AccessResultInterface $other);
-   * When OR-ing two access results, the result is:
+   * When AND-ing two access results, the result is:

    Already fixed in #2340507: Make the new AccessResult API and implementation even better 

  2. +++ b/core/modules/system/entity.api.php
@@ -1836,6 +1836,8 @@ function hook_entity_field_access($operation, \Drupal\Core\Field\FieldDefinition
+  // No opinion.
+  return AccessResult::create();

    Already fixed in #2099137: Entity/field access and node grants not taken into account with core cache contexts

All the other changes are needing a reroll but they are definitely not Major.

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
StatusFileSize
new 2341399-8.patch6.12 KB

Please review.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
StatusFileSize
new 2341399-9.patch7.32 KB
new interdiff.2341399.8.9.txt1.61 KB

@rpayanm Your patch was good but it was just missing a tiny paragraph that also had an extra space at the beginning.

nitesh sethia’s picture

nitesh sethia commented
Status: Needs review » Reviewed & tested by the community

Things look good. So changing the status to Reviewed and tested by community.

nitesh sethia’s picture

nitesh sethia commented
Issue tags: -Needs reroll

Removing need reroll tag from the issue.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2341399-9.patch, failed testing.

DuaelFr queued 9: 2341399-9.patch for re-testing.

dcmul queued 9: 2341399-9.patch for re-testing.

dcmul queued 9: 2341399-9.patch for re-testing.

DuaelFr queued 9: 2341399-9.patch for re-testing.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Needs work » Reviewed & tested by the community

Testbot is happy so let's RTBC this again

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Follow-up: AccessResult documentation fixes » Follow-up: entity.api.php documentation fixes

Reflecting the current scope.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed e3ea8e6 and pushed to 8.0.x. Thanks!

  • alexpott committed e3ea8e6 on 8.0.x 
    Issue #2341399 by fago, DuaelFr, rpayanm: Follow-up: entity.api.php...

Status: Fixed » Closed (fixed)

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