Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.0 (or later) | Named parameter support for PHPCS code #392

Open
3 tasks
jrfnl opened this issue Mar 13, 2024 · 5 comments
Open
3 tasks

4.0 (or later) | Named parameter support for PHPCS code #392

jrfnl opened this issue Mar 13, 2024 · 5 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Mar 13, 2024

Setting the scene

PHP 8.0 introduced support for using named parameters in function calls.

While the minimum supported PHP version for PHP_CodeSniffer itself is currently below PHP 8.0 (and will still be with PHPCS 4.0), external standards may choose to have a higher minimum supported PHP version and may like to start using named parameters in function calls.

At this time, PHP_CodeSniffer does not explicitly support the use of named parameters in function calls and supporting this feature would mean that any changes to parameter names will become a backward compatibility break, which can only be made in a major version.

Proposal

I'd like to propose supporting named parameters as of PHPCS 4.0 (?) for the explicit public API.

In practice this means that the following tasks will need to be executed:

  • Annotate what code is considered part of the public API and what code is considered internal API.
  • Review the method signatures of all methods in the public API to have descriptive parameter names and come up with a more detailed proposal of which parameters will be renamed and to what names.
    In particular, I'd advocate to rename parameters which mirror the names of PHP keywords to different names to prevent confusing function calls.
  • Create a patch with the parameter renames for the target major.

Detailed proposal

To be filled in

Opinions ?

I'd welcome opinions on this proposal. In particular, I'd like to get answer to the following questions:

  • Should PHPCS support named parameters ?
  • Should this support be as of PHPCS 4.0 or can this be delayed to PHPCS 5.0 or later ?
  • Are there particular method parameters which come to mind as prime candidates for renaming ?

/cc @asispts @dingo-d @fredden @GaryJones @greg0ire @kukulich @michalbundyra @Ocramius @sirbrillig @stronk7 @weierophinney @wimg

@GaryJones
Copy link
Contributor

Should PHPCS support named parameters ?

Sniff developers have lasted this long without needing named parameters in PHPCS (or they've gone ahead and used them already). I'd consider the work in this task to be very much in the "nice to have when there's time" category - i.e. lower priority than some of the other tasks already on the list to get done.

@Ocramius
Copy link

Ocramius commented Mar 13, 2024

Named parameters massively expand the API BC surface: avoid if possible.

Prefer small DTOs over named parameters: better/polyfillable API.

someFunc(a: $b, c: $d) is better as someFunc(SomeDTO::from(['a' => $b, 'c' => $d]));, since this allows you to add/migrate SomeDTO named constructors.

The named argument version may be tempting, but it's really just a trap.

EDIT: obviously, both only make sense when signatures are large enough to warrant such complexity 😁

@sirbrillig
Copy link

I have no particular desire to get named parameters. I think they can wait.

@stronk7
Copy link
Contributor

stronk7 commented Mar 18, 2024

Personally I be not felt the need to use named params within PHP_CodeSniffer, the only methods that come to my brain are the findNext() and findPrevious() ones, that, sometimes, you’ve to fill with defaults until “arriving” to the one you want to change.

But it’s not the end of the world and the named alternative would be still longer (lazy typer here).

So I would consider this low priority, although marking public/private APIs and reviewing if there is any “blatant” wrong param name (I don’t remember any right now) are good things, I think.

Hence, my preference is v5. Taking this as sort of “distraction” compared with the pile of other impeding tasks.

Ciao :)

@jrfnl
Copy link
Member Author

jrfnl commented Mar 18, 2024

@GaryJones @Ocramius @sirbrillig @stronk7 Thank you for your input so far.

I'm going to leave this issue open a little longer to give more people the chance to pitch in, but based on the received input, I'm leaning towards not supporting named parameters or, if they will be supported, only supporting them for a small subset of the PHPCS functionality and as of v 5.0 at the earliest.

In practice and for the foreseeable future, this means that parameters may still be renamed at will and without changelog notice (when it makes sense to rename them).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants