Problem/Motivation

The file validation logic changes in #3409599: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "webform_file_validate_extensions" plugin does not exist broke backwards compatibility.

Prior to this change, any item defined in #upload_validators would be checked to see if it was a callback function, after that logic finished all of the values were passed through to hook_file_validate().

With this change, any item defined in #upload_validators is first checked to see if it is a callback, if it is not it is assumed to be a constraint plugin and it is passed to $this->constraintManager->create($validator, $options) inside \Drupal\file\Validation\FileValidator::validate(). This has caused problems with Webform, IMCE, and possibly others, along with possible custom file uploaders.

Steps to reproduce

Add arbitrary data to the '#upload_validators' form array key. A PluginNotFoundException will be thrown.

Proposed resolution

Catch PluginNotFoundException and trigger a deprecation warning that only valid plugin names will be allowed in the '#upload_validators' form array in Drupal 11.

Remaining tasks

Update change record as per #11

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3410126

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:

Comments

DamienMcKenna created an issue. See original summary.

larowlan’s picture

I think the solution should be that we catch the plugin not found and trigger a deprecation error and carry on.

Webform isn't using it for file validation, its using it for theming.

It probably should use something else, but yes, this is a change in behaviour

larowlan’s picture

The code from 10.1

foreach ($validators as $function => $args) {
    if (function_exists($function)) {
      array_unshift($args, $file);
      $errors = array_merge($errors, call_user_func_array($function, $args));
    }
  }

As you can see it silently ignored non existent functions

larowlan’s picture

Priority: Normal » Major

kim.pepper made their first commit to this issue’s fork.

kim.pepper’s picture

Status: Active » Needs review
damienmckenna’s picture

Is there a cleaner way of doing it than a try/catch block?

smustgrave’s picture

Status: Needs review » Needs work

For the open thread to add a trigger_error, not sure if we can reuse an existing CR since this was covered somewhere else?

kim.pepper’s picture

Status: Needs work » Needs review
kim.pepper’s picture

I think we can add something about passing arbitrary data in the form array under '#upload_validators' is deprecated to the existing CR https://www.drupal.org/node/3363700. As that CR is published, do we need to do it after this is committed?

smustgrave’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Just for consistency can the issue summary be updated to include steps, proposed solution, etc.

kim.pepper’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Updated the issue summary.

catch’s picture

I think given the thing we're deprecating here already doesn't work in 10.2.x, it's fine to update the CR before commit here.

catch’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I think the deprecation message needs to mention the string being used as a plugin ID so that this is easier to debug.

catch’s picture

Status: Needs work » Needs review

Applied both those suggestions.

larowlan’s picture

Issue tags: +Patch release target
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears feedback has been addressed.

Since this is a regression assumed it's fine that deprecation is 10.2

larowlan’s picture

Issue credits

  • larowlan committed bb42fa92 on 10.2.x
    Issue #3410126 by kim.pepper, catch, larowlan, DamienMcKenna, alexpott:...

  • larowlan committed bd37e393 on 11.x
    Issue #3410126 by kim.pepper, catch, larowlan, DamienMcKenna, alexpott:...
larowlan’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Patch release target

Committed to 11.x and backported to 10.2.x

Verified the CR changes https://www.drupal.org/node/3363700/revisions/view/13228435/13360530

Thanks all

Status: Fixed » Closed (fixed)

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