Problem/Motivation

Currently if an element contains an invalid #access value, it will be silently evaluated to true.
This could make some security bugs hard to detect.

Steps to reproduce

Add something like this to an element:

'#access' => new \stdClass(),

The element is displayed and processed.

Proposed resolution

We could log a warning if the value is not a boolean or an AccessResultInterface object.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3526250-notify-the-developer-as-warning.patch1.3 KBkalpanajaiswal

Issue fork drupal-3526250

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff’s picture

prudloff commented
Component: forms system » render system
Issue summary: View changes
cilefen’s picture

cilefen commented

I think an exception throw is the best way to notify the developer..

kalpanajaiswal made their first commit to this issue’s fork.

kalpanajaiswal opened merge request !12480

kalpanajaiswal’s picture

kalpanajaiswal commented
StatusFileSize
new 3526250-notify-the-developer-as-warning.patch1.3 KB

I have created a patch to adda warning.

prudloff’s picture

prudloff commented

I think an exception throw is the best way to notify the developer..

I agree but I wonder if it could be a breaking change if some custom code relies on passing something that is currently evaluated to true?

kalpanajaiswal’s picture

kalpanajaiswal commented

prudloff changed the visibility of the branch 3526250-notify-the-developer to hidden.

prudloff’s picture

prudloff commented
Status: Active » Needs review

I added a deprecation that we can later turn into an exception.

prudloff’s picture

prudloff commented
Issue summary: View changes
prudloff’s picture

prudloff commented

Note that the current behavior is inconsistent between FormBuilder::isElementAccessible() and Renderer::doRender().
If #access on a form input is a string:
Renderer::doRender() will display the input (because it hides it only if #access === FALSE).
FormBuilder::isElementAccessible() will not process the input (because it processes only if #access === TRUE).

This would be fixed by only allowing valid values.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Had this ticket open for a few days but wanted to make sure still good to do deprecations in 11.3 and we are good!

Code wise pretty straight forward. Test coverage is there https://git.drupalcode.org/issue/drupal-3526250/-/jobs/7195672

LGTM

quietone’s picture

quietone commented
Title: Notify the developer if #access is anything other than an allowed boolean or access result object. » Deprecate use of "#access" as anything other than an allowed boolean or an AccessResultInterface object

I think a better title would say that this is a deprecation. I didn't see any comment that the change record has been reviewed, that is really part of the process to set an issue to RTBC.

I took a look and thought it could use some wording changes and an 'after' example. I have made those changes.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

Code and CR LGTM too. Great work!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

Too late for 11.3.0, sorry. Can't decide whether this can go in 11.4 for removal in 12, or if we should wait until Drupal 13 - though this feels like a programming error so perhaps 12 is ok?

prudloff’s picture

prudloff commented
Status: Needs work » Needs review

I updated the deprecation with 11.4/13.0.

dcam’s picture

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

All instances of the deprecation and removal versions have been updated to 11.4.0 and 13.0.0, respectively. So I'm setting this back to RTBC.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

  • catch committed e2e2a572 on 11.x 
    task: #3526250 Deprecate use of #access as anything other than an...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

I'm not sure this is a case where we want a silenced @trigger_error - it would only affect code with automated tests with deprecations switched on, which is less and less likely when it's most likely to affect custom site-specific code.

Given we won't start throwing an exception until 13.0.0, let's open a follow-up to trigger E_USER_WARNING for this case from 12.0 onwards.

The MR itself is fine so I have gone ahead and committed to main and 11.x.

Moving to needs work for the follow-up to be opened.

  • catch committed 71d11fd0 on main 
    task: #3526250 Deprecate use of #access as anything other than an...

catch closed merge request !12480

prudloff’s picture

prudloff commented

I created a followup: #3593606: Trigger warning when #access has an invalid value