Problem/Motivation

(From #2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes.)

FormModeAccessCheck and ViewModeAccessCheck have docblocks which read:

Allows access to routes to be controlled by an '_access' boolean parameter.

This looks like an accidental copypaste from the default access check.

Proposed resolution

Correct the class docblocks to describe what the classes actually do.

Remaining tasks

CommentFileSizeAuthor
#9 interdiff-4-9.txt1.79 KBmartin107
#9 accessCheck-2267571-9.patch1.31 KBmartin107
#4 accessCheck-2267571.patch1.79 KBmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Related issues: +#2222719: Use parameter matching via reflection for access checks instead of pulling variables from request attributes
martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: Unassigned » martin107
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes
martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
accessCheck-2267571.patch1.79 KB

Anodyne first draft...

@xjm Did you have something more specific in mind just let me know.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: accessCheck-2267571.patch, failed testing.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Needs review

4: accessCheck-2267571.patch queued for re-testing.

A patch which changes only comments ... Oh testbot you are drunk.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Needs work

Thanks for the patch @martin107! A few comments:

  1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php
@@ -14,7 +14,14 @@
+ * An access check that determines access rules based on the entity form mode.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php
@@ -14,7 +14,12 @@
+ * An access check that determines access rules based on the entity view mode.

    Our docs standards indicate that the one-line summary should start with a third-person verb: https://drupal.org/node/1354#classes
    (A lot of the other access checker classes are missing verbs too, but let's keep this one here correct.)

    Also, the docblock is not quite correct... these classes don't determine access rules per se. They simply perform the access check for the routing system. So perhaps something more like:

    Defines an access check for entity form mode routes.

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php
@@ -14,7 +14,14 @@
+ * The Form mode are the equivalent of view modes applied to entity forms.

    We don't need to explain what form modes are here; this is just an access checker and the concept of form modes is discoverable through the UI. 

  3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php
@@ -14,7 +14,14 @@
+ * @see \Drupal\Core\Entity\EntityManagerInterface::getAllFormModes()
+ * @see \Drupal\Core\Entity\EntityManagerInterface::getFormModes()
+ * @see hook_entity_form_mode_info_alter()

    I think we can actually omit these three @see, since they're not as directly relevant and they're also available from the EntityFormMode class (and EntityViewMode similarly). 

  4. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php
@@ -14,7 +14,12 @@
+ * @see \Drupal\entity\Entity\EntityViewMode.

    Let's remove that period at the end of the @see.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Needs review
FileSize
accessCheck-2267571-9.patch1.31 KB
interdiff-4-9.txt1.79 KB

So I want that axe, but change the blade and change the handle :-)

Thank you for the review.. less is more - it reads much better...

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community

That looks good I think!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks! We can't commit this until after the next Alpha (critical/major only through Wednesday this week).

martin107’s picture

martin107 CreditAttribution: martin107 commented
Issue tags: +Quick fix

Nominated this issue for a quick fix as it will be affected by the looming deadline on May 27 ( ie will need reroll )

https://drupal.org/node/2247991

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 8.x.

  • Commit 6c4b0bd on 8.x by jhodgdon: 
    Issue #2267571 by martin107, xjm: Fix docs for field UI access classes

Status: Fixed » Closed (fixed)

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

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned