Problem/Motivation

Procedural handler access callbacks have been deprecated.
Default to false if they exist afterwards to prevent leaking data.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3547724

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

nicxvan created an issue. See original summary.

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.

smustgrave’s picture

smustgrave commented

This may be good to start?

nicxvan’s picture

nicxvan commented
Title: [pp-d12] Set the default access callback unsupported procedural functions to FALSE » Set the default access callback unsupported procedural functions to FALSE

Yes

smustgrave opened merge request !15272

smustgrave’s picture

smustgrave commented
Status: Active » Needs review
Related issues: +#3553503: [meta] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules from Drupal 12

Is that what you meant? Wasn't entirely sure what needed to be removed.

starting to go back through any deprecations we moved to follow ups and this was one.

nicxvan’s picture

nicxvan commented
Issue tags: +Needs subsystem maintainer review

Looking at the intent I think we need to also drop: && function_exists($this->definition['access callback'])
The access callback looks like it only supported procedural implementations, and we need to not accidentally grant access where we didn't previously.

I may be mistaken here so I tagged for subsystem review too.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Lets do the approach @berdir mentioned.

smustgrave’s picture

smustgrave commented
Status: Needs work » Needs review
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Needs work
Issue tags: -Needs subsystem maintainer review

The test should have been using $view->$type['access_callback'] and not $view->field['access_callback'], that way it would have actually been doing what it was supposed to do, check it for all handler types. I've tried that locally and it works and it's green.

If we'd want to keep the coverage for now, we should probably use a dataprovider to supply the $type and iterate through them that way, so we can catch all the deprecation messages and not just the first one. That would work I think.

Since we are dealing with 'access' stuff, I'm not sure removing the coverage is the best way to go here, if using a dataprovider does actually work it should be simple enough a change to keep the coverage (didn't check that yet).

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> Since we are dealing with 'access' stuff, I'm not sure removing the coverage is the best way to go here, if using a dataprovider does actually work it should be simple enough a change to keep the coverage (didn't check that yet).

That's fair. The problem is that the fundamental mechanism that this uses to test access is being removed. Without it, we no longer have a mechanism to generically test every plugin type because every access logic needs to be implemented in the plugin. We'd need to have a custom test plugin for all of them to exercise this. That's quite a lot of work.

We have test coverage for actual access logic, such as EntityField and FieldLanguage, but most plugins do not currently have examples that include access restrictions.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Reviewed & tested by the community

Yeah that is a fair point, that would make it not a low effort to keep this, we have some coverage for access(), I agree removing it makes sense.

catch’s picture

catch
he/him
Primary language English
commented
Title: Set the default access callback unsupported procedural functions to FALSE » Throw an exception when unsupported procedural functions are defined in views

Re-titling for what this actually does now. Got momentarily thrown off by the return TRUE after the exception is thrown, but that's only if we're in the non-deprecated path, but I was looking for a 'return FALSE' and didn't see one.

Given this is data structure so only test coverage would actually trigger the deprecation, it does seem nicer to throw an exception here. I think we need a follow-up to remove the exception in Drupal 13 (or 14, but at some point).

catch’s picture

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

Committed/pushed to main, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !15272

  • catch committed a06956c9 on main 
    task: #3547724 Throw an exception when unsupported procedural functions...

Status: Fixed » Closed (fixed)

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