(outdated) Replace func_get_args with variable-length argument in FormBuilder

This is a nice little improvement and also solves PHPCS false positives like #3102431: PHPCS Warning - PHP7.2 Compatibility issue - func_get_args

We are adding to FormBuilderInterface here but I think that is OK - this is neither tagged @api nor @internal so we can add to it according to https://www.drupal.org/about/core/policies/core-change-policies/drupal-8... especially as FormBuilder provides a concrete implementation that users should be extending.

We're going to need to do some work here.

We need to fix coder to support proper variadic document. We the changes I recommended to the MR we get:

FILE: /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Form/FormBuilderInterface.php
-----------------------------------------------------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------
  49 | ERROR | [x] Expected "" but found "..." for parameter type
     |       |     (Drupal.Commenting.FunctionComment.IncorrectParamVarName)
  49 | ERROR | [x] Expected 1 spaces after parameter type; 0 found
     |       |     (Drupal.Commenting.FunctionComment.SpacingAfterParamType)
 165 | ERROR | [x] Expected "" but found "..." for parameter type
     |       |     (Drupal.Commenting.FunctionComment.IncorrectParamVarName)
 165 | ERROR | [x] Expected 1 spaces after parameter type; 0 found
     |       |     (Drupal.Commenting.FunctionComment.SpacingAfterParamType)
-----------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-----------------------------------------------------------------------------------------------------------------------------

Also we're going to need release manager buy-in for the interface change.

As a framework manager I'm +1 to these changes as it improves the code to match the reality of how it is used and enables better static parsing of core.

Coder is fixed already... we need to do * @param mixed ...$additional and it'll work just fine.

We can add methods to interfaces under the 1-1 rule (since if you extend the core class you get the new method and it probably won't break), but adding parameters to methods isn't covered.

Having said that, while there are several classes extending form builder in contrib: http://grep.xnddx.ru/search?text=extends+FormBuilder&filename=, none appear to override the method, so in practice this change might be fine.

Leaning towards this being 10.x-only though.

This looks good to me.

Nitpick: The similar patch at #3102431: PHPCS Warning - PHP7.2 Compatibility issue - func_get_args calls the parameter $args. I think that is clearer than $additional, since they are placed in the form build info as 'args'.

#10 is compelling and 10.x is not open yet so let's make the change.

LGTM.

Overall, this is a good DX improvement. There are lots of places in core that might need a similar change. To see a full list of potential places, do:

grep -rl "func_get_args" * | grep -v "vendor"

I considered whether this should be done all in one mega cleanup, or as part of a meta (plan) issue. Since it is a BC break, we should do a meta issue.

This is also too disruptive as-is for a minor branch, which is why @catch was considering this to be a D10-only change in #7. Demonstration of the BC break:
https://3v4l.org/rEU6t

However, we can deprecate the old func_get_args() pattern in 10.1.x, and convert to ...$args in the main method signature in D11. Here's how... EDIT. Previous link was some "True value is true". Here's the real snippet:
https://3v4l.org/FNdOm

Let's create a working patch for that in this issue to prove how it should work.

Then, we can:

1. Create a meta issue
2. Document the deprecation pattern
3. Possibly enable a PHPCS rule to prefer the variadic pattern (not sure about the last part since maybe there are still legit uses of func_get_args(), but we can at least discuss it).

Tagging "Needs followup" for those things.

My previous example above was checking the wrong argument (derp) and would have printed the deprecation message regardless of whether the code were correct or not. I edited my comment to link the correct demo code:
https://3v4l.org/FNdOm

Thank you @jidrone for correctly translating my example into a deprecation! Going to ask the framework managers and the other release managers for thoughts on this approach.

...Oh, of course this is failing the deprecation listener every time core hits it. It'd be the correct deprecation pattern for the caller, but the problem isn't the callers, which are what are failing currently. By definition, the callers can pass any parameters they want, or none. It's the child methods' signatures at issue.

We also can't add it to the skipped deprecation list to conditionally skip fails for core tests but not for contrib, because now DrupalCI defaults to using the core file. Reference: https://www.drupal.org/node/3285162

I think we'll also need framework manager feedback on the best solution here.

  public function getForm($form_arg/* , ...$args */) {

With #3055194: [Symfony 5] The signature of the "Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::dispatch()" method should be updated to "dispatch($event, string $eventName = null)", not doing so is deprecated since Symfony 4.3. I was pretty sure Symfony's debug classloader was picking this up (or via the doxygen for $args and comparing it to the method parameters), but I can't from reading the classloader figure out if/how it does that. Even if it doesn't, I think we could manually add a phpstan rule that would warn subclasses without the parameter.

Seems to have some open points in the threads.

Also was tagged for a followup in #15 have those points been documented?

Could someone explain what the thing of commenting out single function parameters is meant to do and how it works:

  public function getForm($form_arg/* , ...$args */) {

I don't think the deprecation notice in the MR can ever be triggered successfully but #3236391: (outdated) Replace func_get_args with variable-length argument in FormBuilder solves this problem and I think it's ready to go.

@catch I think you meant #3354595: Remove the debug classloader deprecation silencing for Drupal interfaces. I am attaching here the patch from there and close the other as dupe of this.

#24 from https://drupal.slack.com/archives/C1BMUQ9U6/p1681721410656849 (my additions in square brackets):

[...] Symfony lets you add commented out arguments to interfaces that trigger a deprecation error [via Symfony's debug classloader] when they aren't implemented [...]

+++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
@@ -46,20 +46,22 @@ public function getFormId($form_arg, FormStateInterface &$form_state);
    *   - An instance of a class that implements \Drupal\Core\Form\FormInterface.
-   * @param ...
+   * phpcs:disable Drupal.Commenting
+   * @param mixed ...$args
    *   Any additional arguments are passed on to the functions called by
    *   \Drupal::formBuilder()->getForm(), including the unique form constructor
    *   function. For example, the node_edit form requires that a node object is
    *   passed in here when it is called. These are available to implementations
    *   of hook_form_alter() and hook_form_FORM_ID_alter() as the array
    *   $form_state->getBuildInfo()['args'].
+   * phpcs:enable
    *

One more thing - do we need a note saying that defining this will be required in Drupal 11. There is otherwise no way to add a custom message anywhere afaict.

How about opening an issue for the D11 queue listing all the interfaces to be changed, and refer such cases in code to that issue. Would also solve issues like https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/... where an addition was made in D9 but then everyone forgot about fixing it for D10 and so now it's too late.

Done #29.

Typo in "method" picked up by cspell.

Strange, attached files got lost in the x-post with @longwave

1. +++ b/core/lib/Drupal/Core/Database/StatementInterface.php
   @@ -120,7 +120,8 @@ public function fetchField($index = 0);
   +   * @see https://www.drupal.org/project/drupal/issues/3354672.
   
   +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -46,20 +46,25 @@ public function getFormId($form_arg, FormStateInterface &$form_state);
   +   * @see https://www.drupal.org/project/drupal/issues/3354672.
   
   @@ -158,7 +163,11 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form
   +   * @see https://www.drupal.org/project/drupal/issues/3354672.
   

   @see shouldn't use dot at the end

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -211,13 +211,12 @@ public function getFormId($form_arg, FormStateInterface &$form_state) {
   -  public function getForm($form_arg) {
   +  public function getForm($form_arg, mixed ...$args) {
   ...
   +    if (!empty($args)) {
   +      $form_state->addBuildInfo('args', array_values($args));
   

   Maybe check for emptiness is not needed as it's ok to use empty array or even NULL if passed

For the pints in #34

Also was tagged for followups in #15.

On this.

Addressed #34.

Follow-up re. #15, #3355757: [meta] Replace usage of func_get_args with variadic functions.

We still have a BC break as noted in #15 - we can't add the args to the concrete method until Drupal 11, in case someone has already extended FormBuilder::getForm(): https://3v4l.org/nblth

Symfony EventDispatcher did not add them either: https://github.com/symfony/event-dispatcher/blob/4.4/EventDispatcher.php...

#39 if I add the new parameter commented out in the concrete class too, though, we get the deprecation error:

#3257824-101: Ignore, testing issue

https://dispatcher.drupalci.org/job/drupal_patches/179671/

We have to add that exact deprecation to the ignore file until 11.x opens, probably in.a new section labelled Drupal 11. This means the deprecation will be skipped for core, but if anyone else is extending the class then they will still get the deprecation notice now - if we keep the regex deprecation instead then downstream users will not be notified.

This will also serve as another reminder to actually fix this in the D11 cycle.

Right.

#41 changes the scope significantly, so I will postpone this one (the actual change) to D11, and file #3355839: Prepare FormBuilder for variadic functions for the D10 deprecation.

Updating title, tags and version number based on recent announcement at https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-...

Fixing attribution.

Opened #3431285: Replace func_get_args with variable-length argument in FormBuilder since the issue fork was too old here to be able to place a MR against 11.x.