Currently the field_access checks on entity form / entity view are done by WidgetBase::form() / FormatterBase::view().

- There are cases when you might want to use a widget even if the current user has no permissions to edit the field - e.g in a exposed view filter. (less sure about use cases for using a formatter while you have no permissions to view the field...)
- This being done in base methods, there is no guarantee that some widgets / formatters don't override it - possible holes.

In Vienna, @fago argued that #access should be the responsibility of the caller, not of widgets / formatters themselves.

Thoughts ?

CommentFileSizeAuthor
#5 widget_formatter_access_check-2151699-5.patch2.49 KBjsbalsera
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#access should be the responsibility of the caller, not of widgets / formatters themselves.

+1 - That makes perfect sense in terms of separation of concerns.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented

Hi,
Here we are almost in the same situation than #2151693: Widgets / formatters should return unwrapped $elements. This could be an easy fix for FormatterBase::view(), but we should do a bit tricky code in field_invoke_method(), creating an exception when $method == 'form' for WidgetBase::form()

Should we postpone this on #2095195: Remove deprecated field_attach_form_*(), given that this patch will cause conflicts with both #2151693: Widgets / formatters should return unwrapped $elements and #2095195: Remove deprecated field_attach_form_*()? Or maybe merge this issue with #2151693: Widgets / formatters should return unwrapped $elements and modify field_invoke_method() only once.

yched’s picture

yched CreditAttribution: yched commented
Status: Active » Postponed

Postponing on #2095195: Remove deprecated field_attach_form_*() makes sense, yeah.

jsbalsera’s picture

jsbalsera
he/his
Primary language Spanish
Location Córdoba, Spain
CreditAttribution: jsbalsera commented
Assigned: Unassigned » jsbalsera

Working on this

jsbalsera’s picture

jsbalsera
he/his
Primary language Spanish
Location Córdoba, Spain
CreditAttribution: jsbalsera commented
Assigned: jsbalsera » Unassigned
Status: Postponed » Needs review
FileSize
widget_formatter_access_check-2151699-5.patch2.49 KB

First patch, moving the #access keys to the caller functions.

plopesc’s picture

plopesc
he/him
Primary language Spanish
CreditAttribution: plopesc commented
Status: Needs review » Reviewed & tested by the community

Cool.
Looks good to me.
Thanks @jsbalsera!!

yched’s picture

yched CreditAttribution: yched commented

RTBC +1. Thanks @jsbalsera !

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Can we get a better issue title for the commit message?

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Title: Should widgets / formatters enforce #access / field_access checks ? » Move #access responsibility from formatters/widgets to EntityViewDisplay/EntityFormDisplay

Sure.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit ca56067 on 8.x by catch: 
    Issue #2151699 by jsbalsera: Move #access responsibility from formatters...

Status: Fixed » Closed (fixed)

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