Ignore parent parameter name mismatch

[franmomu]

This could lead to problems using named arguments in PHP 8

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

Psalm was failing because of https://psalm.dev/docs/running_psalm/issues/ParamNameMismatch/

UPDATE: It would be BC break to change the parameter as pointed out in #2315 (comment), so ignoring the issue for now.

[SenseException]

Psalm is listing the mismatch because of interchangeable classes that should use the same variable names, but the current renaming would AFAIK also be a BC break.

[franmomu]

the current renaming would AFAIK also be a BC break.

Makes sense, I've ignored the psalm issues instead.

[SenseException]

I'm not sure how to handle this in the next major release though.

[malarzm]

Not sure if this is a standard, but lately I've seen @no-named-arguments in PHPUnit (https://github.com/sebastianbergmann/phpunit/blob/564e78a6c088916467cd4bc0af2e1bb9da9b1a0c/src/TextUI/ResultPrinter.php#L16). Personally I'd be happy to slap this on most of our public API, named arguments should be used where it makes sense, not everywhere

[franmomu]

In fact, in this case are methods with just one argument which I'm not sure if it makes sense to use named parameters.

About @no-named-arguments 👍, to read more about it: Roave/BackwardCompatibilityCheck#264, but I guess that should be decided internally if apply it or not in general, I'll open an issue in doctrine/persistence.

[alcaeus]

IMO:

• Named parameters are a new feature, which highlights inconsistencies in APIs
• Such inconsistencies should either be fixed before named args are widely used, or fixed in a backwards compatible way once thy are

Considering that all of these arguments are single, required method arguments, I'll argue that most people won't be using named arguments for that; and if they are then the impact would be caught early (static analysis should catch using named args that don't exist).

Sorry for the back and forth, but it's preferable to rename the arguments for consistency and be done with this. I've left additional comments regarding no-named-arguments in the linked persistence PR, but for ODM I'd definitely prefer cleaning up the API rather than telling people not to use named args.

Stale review on different patch.

[alcaeus]

Thanks @franmomu!

[franmomu]

Sorry for the back and forth, but it's preferable to rename the arguments for consistency and be done with this. I've left additional comments regarding no-named-arguments in the linked persistence PR, but for ODM I'd definitely prefer cleaning up the API rather than telling people not to use named args.

No worries! It's something that has to be discussed.