Problem/Motivation

ElementInfoManager::buildInfo() calls each form element plugin's getInfo(), then unconditionally overwrites #value_callback for any FormElementInterface implementation with [$definition['class'], 'valueCallback']. Any element that declares its own #value_callback in getInfo() has that value silently discarded.

This masked a separate bug in \Drupal\Core\Render\Element\File::getInfo(). It declared #value_callback as [[static::class, 'valueCallback']], an incorrectly double-wrapped array. Because the overwrite above always ran first, this invalid value was never actually used, so the bug had no visible effect until now.

This surfaced while wiring \Drupal\file\Element\ManagedFile::getInfo() to declare its own #value_callback (see #3616654: [PP-2] Convert ManagedFile::valueCallback() logic into an injectable service), since that declaration was likewise being discarded.

Steps to reproduce

Proposed resolution

Change ElementInfoManager::buildInfo() to only assign the default #value_callback when the element's own getInfo() has not already declared one. Remove the redundant, malformed #value_callback declaration from File::getInfo(), since File::valueCallback() already exists and is exactly what the default assignment would wire up.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3616663

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

kim.pepper created an issue. See original summary.

kim.pepper’s picture

Status: Active » Needs review
nicxvan’s picture

I think this deserves a CR. I took a quick pass, but it feels pretty bare, so it could use another pass.

This is a pretty straightforward fix and has a test so I think it should be ready once we square away the CR.

kim.pepper’s picture

Added a CR. We don't usually add them for bug reports, but there might be some unexpected callback being executed after this change where it wasn't previously.

nicxvan’s picture

Status: Needs review » Reviewed & tested by the community

Yes, but if you squint it's also kind of a new feature too.

We can leave it unpublished if the committers don't think we need one.

I marked my cr obsolete, yours is much better.

I think this is ready! L

  • catch committed d93e6f01 on 11.x
    fix: #3616663 ElementInfoManager overwrites a form element's own #...
catch’s picture

Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

I think the CR is more useful than some other ones we've made, went ahead and published.

Committed/pushed to main and 11.x, 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 committed 7ce49f57 on main
    fix: #3616663 ElementInfoManager overwrites a form element's own #...

  • catch committed 07af474f on 11.x
    Revert "fix: #3616663 ElementInfoManager overwrites a form element's own...
catch’s picture

Status: Fixed » Patch (to be ported)

It looks like this caused phpstan failures on 11.x, reverting for now - let's do a backport MR to confirm.

  • catch committed fc8ed46c on 11.x
    fix: #3616663 ElementInfoManager overwrites a form element's own #...
catch’s picture

Status: Patch (to be ported) » Fixed

Backport looks good now.

Committed/pushed to 11.x, 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 committed 6e95a1d0 on 11.x
    Revert "fix: #3616663 ElementInfoManager overwrites a form element's own...
catch’s picture

Status: Fixed » Patch (to be ported)

Or not let's try that again.