Problem/Motivation

In #3382510: Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms we introduced a new value object ConfigTarget that has callables to change a value form a form value to a config value and vice versa. The callables must be strings because we save the ConfigTarget objects to form state but maybe we don't need this restriction.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3399295

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

alexpott created an issue. See original summary.

alexpott opened merge request !5251

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
bircher’s picture

bircher
Location 🇨🇿
commented

very cool!

Could we revert the r/rw => 0/1 conversion ?

I think it would serve as a neat example.

alexpott’s picture

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

I've addressed @bircher and @andypost's reviews. This is ready for another round.

alexpott credited andypost.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

This is a very useful changes and the change itself looks very readable as well, what I don't understand is that we disallowed callables before because of serialisation.

This is the old comment, is that no longer true?

 If these could be arrays, then they could be
    // in the form of [$object, 'method'], which would break serialization if
    // $object was not serializable. This is also why we don't type hint these
    // parameters as ?callable, since that would allow closures (which can't
    // be serialized).
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work
Issue tags: +Configuration schema, +validation, +DX (Developer Experience)
Parent issue: » #3382510: Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms

#4++

#8: That is indeed no longer true. See the ::fromForm() method that @alexpott introduced. I left a comment about it on the MR that may be helpful in understanding it.

Just left a few nits/questions, but this is 90% ready IMHO.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Addressed all review points.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

AFAICT 

      2x: Creation of dynamic property
    Drupal\Core\Form\ConfigTarget::$elementName is deprecated

is causing the test failures.

But I see no more occurence? 😳 WTF!

EDIT: LOL, @alexpott just pushed the fix at https://git.drupalcode.org/project/drupal/-/merge_requests/5251/diffs?co... — that's why 🤣

This looks like an excellent improvement! 👍

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

  • catch committed 3229a9fe on 10.2.x 
    Issue #3399295 by alexpott, Wim Leers, bircher, andypost, borisson_:...

  • catch committed 09b18e5e on 11.x 
    Issue #3399295 by alexpott, Wim Leers, bircher, andypost, borisson_:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Very nice. Had to have a couple of read throughs to figure out what was going on, but very nifty. Committed/pushed to 11.x and cherry-picked to 10.2.x, thanks!

alexpott’s picture

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

I've updated https://www.drupal.org/node/3373502

Status: Fixed » Closed (fixed)

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

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

It looks like this may have caused the \Drupal\Core\Form\ConfigTarget to not be fully serializable: #3408120: \Drupal\Core\Form\ConfigTarget is not fully serializable.