Fix(deprecation) Optional parameter declared before required parameter is implicitly treated as a required parameter

[thirsch]

This pr aims to fix the deprecation warnings raised by having required parameters after optional parameters. The given change would have the least possible impact instead of introducing a BC by changing the signature to e. g. require the parameter $parent or change the order.

wdyt? As named parameters have been introduced in PHP 8.0, there might be no call without the parameter being explicitly set to null and we could consider it safe to change the signature to public function __construct(sfWidgetForm $widget, ?sfFormField $parent, $name, $value, ?sfValidatorError $error = null)?

I'd prefer the changed signature, as it is much cleaner to not introduce a pseudo-optional name and value argument.

[connorhu]

I didn't go into the details, but one question came up: can value and name be null? What does the code do if it is null? Shouldn't InvalidValueException be thrown for null? The name property is '' by default. For both fields @param phpdoc says should not be null.

Actually, I would like to point out that you have to do the same after PR as before, if only the $widget variable is specified.

[thirsch]

As far as I understood, they are non nullable. That's the reason, why I'd prefer to not introduce the pseudo nullable arguments. On the other hand, it's the least BC-risking fix to the current situation.

[connorhu]

symfony1/lib/widget/sfWidgetForm.class.php

Lines 164 to 172 in 0c9ba11

	public function renderTag($tag, $attributes = [])
	{
	if (empty($tag)) {
	return '';
	}
	$attributes = $this->fixFormId($attributes);
	return parent::renderTag($tag, $attributes);

Here it is decided that neither the name nor the value will be printed. They can both be null. It is possible that other widgets exclude null values, but it is sure that phpdoc is almost always wrong.
The tests don't test the null case anywhere, and I can't find an sfFormField test anywhere (the sfFormField constructor is not tested by the test case that is supposed to do this).

[connorhu]

I would like to do a breaking change.
For me, a sympathetic solution would be to put the parent before after the error and look at this rabbit hole. What problem does this change cause? Can we provide a solution to make this change painless for anyone (rector?). What would be the update path in this case? If a breaking change is made at this level, how is it reflected in the version number?
The change you suggest is safe, but in time we will get to the point where my questions need to be answered.

[thirsch]

I'd prefer not changing too much. The changes in this pr are no BC at all, but IMO at least $name shouldn't be nullable. The next best approach seems to be removing the default null of parameter $parent to ?sfFormField $parent, which in theory is a BC, but as named parameters have only been around since PHP 8.0, it shouldn't hurt to much while migrating, if at all. Changing the whole signature on the other hand will introduce a hard BC for all users.

[connorhu]

I have the feeling that we are pushing the problem in front of us.

I'd prefer not changing too much. The changes in this pr are no BC at all, but IMO at least $name shouldn't be nullable. The next best approach seems to be removing the default null of parameter $parent to ?sfFormField $parent, which in theory is a BC, but as named parameters have only been around since PHP 8.0, it shoudn't hurt to much, if at all while migrating. Changing the whole signature on the other hand will introduce a hard BC for all users.

In that case, we should add tests for all cases (null, non-null, int, string, array) directly to the class. Once we have those and have clarified the operation:

1. Add the default null value to the argument.
2. Clarify the phpdoc.

[thirsch]

Although I do understand your points and wishes for clean code, I don't have the time to take care of it now. But I'm happy if anyone else could take it up from here or if we just merge this to master to get rid of "declaring mandatory arguments after optional arguments is deprecated" and take care of the proper type later, as this probably isn't the last occurrence of type issues.

I think both proposals are minimal changes and, at least the change of this pr, does not bring any new allowed values, as null has already been possible.

[thirsch]

@thePanz wdyt? While upgrading our codebase to 8.4, we keep running into this warning.

[thePanz]

Thanks @thirsch and @connorhu for mentioning here, this is a tough one.
I quickly checked the code, and the $name property is used in widgets's code on string functions substr() as an example.

I would thus suggest to put its default value as an empty string (as it is already initialized in the class itself).

Regarding the $value: other methods expect it to be mixed not just a string

WDYT about this signature for the sfFormField::__construct() method? The same would be applied to sfFormFieldSchema too.

     * @param sfWidgetForm          $widget A sfWidget instance
     * @param sfFormField|null      $parent The sfFormField parent instance (null for the root widget)
     * @param string                $name   The field name
     * @param mixed                 $value  The field value
     * @param sfValidatorError|null $error  A sfValidatorError instance
     */
    public function __construct(sfWidgetForm $widget, ?sfFormField $parent = null, $name = '', $value = null, ?sfValidatorError $error = null)

1. name is default to ''
2. value is a mixed defined in the php docblock
3. adjusted $parent and $error docblock to have null as value too

[thirsch]

WDYT about this signature for the sfFormField::__construct() method? The same would be applied to sfFormFieldSchema too.

LGTM, I've updated the branch.

[thirsch]

Thanks for your approve @thePanz. Do you know, why merging is blocked? If I switch back to "the classic merge experience", everything looks fine.

[thePanz]

I used the "Rebase and merge", worked now 👍