Problem/Motivation

To modernize the FormBuilder code it was suggested in #3236391: (outdated) Replace func_get_args with variable-length argument in FormBuilder to use variadic functions.

We cannot just change the method signatures, it would cause BC breaks.

Proposed resolution

Here for D10, preliminarily add new method arguments, commented out to allow the DebugClassloader throw deprecations if not implemented, and silence the deprecations.

In the parent, in D11, implement the changes.

CommentFileSizeAuthor
#19 230726a.patch1.55 KBmondrake
#2 3355839-2.patch4.72 KBmondrake

Issue fork drupal-3355839

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

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
StatusFileSize
new 3355839-2.patch4.72 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#3354672: [Meta] Adjust parameters in interfaces

Looks a good place to apply variadics!

It could use CR to notify module developers

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -Needs release manager review, -Needs framework manager review

This looks like a good plan to me, and good to start using this technique for other issues eventually too.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs change record

Thanks @catch and @andypost

Moving to NW for the change record mentioned in #3

Can mark after that.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake opened merge request !4455

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review

Changed to MR worflow.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs review » Needs work

Did not mean to change status.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Needs work » Needs review
Issue tags: -Needs change record

Added draft CR, https://www.drupal.org/node/3376455

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented 
+++ b/core/.deprecation-ignore.txt
@@ -10,12 +10,15 @@
+%FormBuilder::getForm\(\).* will require a new "mixed \.\.\. \$args" argument in the next major version of its interface%
+%FormBuilder::submitForm\(\).* will require a new "mixed \.\.\. \$args" argument in the next major version of its interface%

Does the class name need to be fully qualified here?

In the case that someone has extended FormBuilder as \Drupal\custom_module\FormBuilder and swapped it in (or even \Drupal\custom_module\CustomFormBuilder), I think this will match and accidentally skip the deprecation? We don't always add tests for this sort of thing any more, but this is new (to us) so maybe we should here.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Done

smustgrave’s picture

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

With CR added think this is good to add for 10.2

  • longwave committed 29dc14b2 on 11.x 
    Issue #3355839 by mondrake: Prepare FormBuilder for variadic functions
longwave’s picture

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

Committed 29dc14b and pushed to 11.x. Thanks!

Also published the change record. This seems to be the only sane way to add new arguments to interfaces, so let's hope we don't run into any problems with it.

mondrake closed merge request !4455

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Hi 👋
If the deprecation is ignored, will contrib and custom projects see it?

Thanks

mondrake’s picture

mondrake
Location 🇮🇹
commented

#17 well for sure if a custom/contrib class implements the interface, without the to-be signature, then the DebugClassLoader will throw the deprecation.

What I'm not sure about is what happens when a custom/contrib class extends a core base class that's ignored (e.g. Drupal\Core\Form\FormBuilder).

mondrake’s picture

mondrake
Location 🇮🇹
commented
StatusFileSize
new 230726a.patch1.55 KB

Re #18, it looks like that works. See test patch attached.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

That's why I asked for the fully qualified class name in #11, as we can't implement this change in the core implementation yet, but all other users of the interface should be notified.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Filed #3377356: Make DebugClassLoader ignored deprecations more accurate for a possible follow-up.

catch’s picture

catch
he/him
Primary language English
commented

I missed the last week of progress here, but really nice to see this land and the first ever time we've been able to do this with the debug classloader doing what it's supposed to do!

Status: Fixed » Closed (fixed)

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