Problem/Motivation

In EntityAccessCheck::access(), the route parameter $entity is checked for instanceof EntityInterface, to see whether ::access() can be called on it.

However, this interface actually inherits that method from AccessibleInterface::access(). So the instanceof check is more strict than it might need to be.

Proposed resolution

1) Determine which interface the instanceof should check [edit: it's checking the right one, but needs to be made more explicit why].
2) Write a test to affirm the right ones of e.g. EntityInterface::access(), AccessibleInterface::access(), stdClass::access() are called or not called.
3) Modify the instanceof if required [edit: it wasn't].

Remaining tasks

1. Write patch
2. Iterate and approve patch
3. See #1699206: Methods in BookOutlineStorage need node access warnings in docblocks, whether that documentation should be changed there.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#8 entityaccesscheck-2847687-8.patch5.82 KBjp.stacey
#4 entityaccesscheck-2847687-4.patch4.68 KBjp.stacey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jp.stacey created an issue. See original summary.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Priority: Normal » Minor
jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Issue summary: View changes
jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Status: Active » Needs review
FileSize
entityaccesscheck-2847687-4.patch4.68 KB

Patch attached, including a new test for the different interfaces (and a control test using stdClass.)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

I'm not convinced by this particular change. The code here is really about entities, well, if it should be able accessibles the entire class should be clearly labeled for it, like moving it outsider of the entity namespace.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented

@dawehner thanks - happy to go with what you say: I think the main thing is then to focus on the new test, which both covers this code and captures the semantics of what you've said, so it's clear for future developers.

I'll look at this again today as part of the sprint weekend.

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Status: Needs review » Needs work
jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Status: Needs work » Needs review
FileSize
entityaccesscheck-2847687-8.patch5.82 KB

Patch attached. I don't think an interdiff reveals much so I've not provided that.

This patch makes no changes to running code, but instead:

1) improves the docblock of EntityAccessCheck to explain things more clearly (esp. which information is a route requirement, versus which is a route match parameter.)
2) clarifies the inline comment just before the instanceof.
3) adds a test which mandates the behaviour described in comment #5.

Feedback appreciated!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you. I really like those small improvements, because they really matter on a day to day base.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.php
@@ -16,28 +16,32 @@ class EntityAccessCheck implements AccessInterface {
-   * And this will check a dynamic entity type:
+   * And this will check 'delete' access to a dynamic entity type:

Nice improvement

jp.stacey’s picture

jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commented
Issue summary: View changes

@dawehner thanks: great news!

So that future developers understand what's happened on this issue, I've reworded the description, so that it more clearly matches the result (i.e. we didn't change the interface after all.) I've also checked #1699206: Methods in BookOutlineStorage need node access warnings in docblocks and the patch there doesn't need modifying either so I've updated that.

  • catch committed 5b2cb03 on 8.4.x 
    Issue #2847687 by jp.stacey, dawehner: EntityAccessCheck mandates...

  • catch committed 1c16b76 on 8.3.x 
    Issue #2847687 by jp.stacey, dawehner: EntityAccessCheck mandates...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: EntityAccessCheck mandates instanceof EntityInterface when it only needs AccessibleInterface » Improve test coverage for EntityAccessCheck

Hmm we should've improved the title before commit as well, mea culpa, fixing it here for now at least.

Status: Fixed » Closed (fixed)

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