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.

Sign up for GitHub

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

Jump to bottom

PHP 8.0: Review parameter names used in sniffs #1239

Open
35 of 42 tasks
jrfnl opened this issue Nov 12, 2020 · 1 comment
Open
35 of 42 tasks

PHP 8.0: Review parameter names used in sniffs #1239

jrfnl opened this issue Nov 12, 2020 · 1 comment
Assignees
@jrfnl
Labels
chores/QA good first issue
Milestone
10.0.0

Comments

@jrfnl
Copy link
Member

jrfnl commented Nov 12, 2020 •
edited

There are a number of sniffs which mention parameter names in the error/warning message.

As part of the preparations for PHP 8 and named parameters in function calls, a lot of parameters for PHP native functions have been renamed to more descriptive names.

As it is, the online PHP documentation has not been updated with the new names yet, though it is expected that this will happen around the release date.

To find the renamed parameters, for now, the only source would be the commits: https://github.com/php/php-src/search?q=parameter+names&type=commits and looking at the stub files in the source.

The code in existing sniffs referencing parameter names needs to be updated to use the parameter name as per PHP 8 to prevent confusing people if they use named parameters.
If there is no PR open to update the PHP docs for the renamed parameter, it is recommended to also open a PR for that: https://github.com/php/doc-en/pulls Edit: this will be handled for all docs via a script: https://github.com/php/php-tasks/issues/26#issuecomment-731939804

Task list (not necessarily complete):

To do/to review:

  • Sniff
  • PHPCompatibility.FunctionUse.ArgumentFunctionsReportCurrentValue (@jrfnl)
  • PHPCompatibility.InitialValue.NewConstantScalarExpressions (@jrfnl)
  • PHPCompatibility.ParameterValues.NewAssertCustomException
  • PHPCompatibility.ParameterValues.RemovedAssertStringAssertion
  • PHPCompatibility.ParameterValues.RemovedImplodeFlexibleParamOrder
  • PHPCompatibility.ParameterValues.RemovedMbstringModifiers

Changes prepped:

Pulled

Regarding sniff names

The names of the ParameterValues sniffs often contain a parameter name in the actual sniff name too and sometimes also in the error code.

I'd like to propose to leave the sniff name/error codes alone to minimize BC-breaks as changing those can invalidate rulesets and inline ignore annotations.
If needs be, a note can be added to the class docblock annotating that the name of the parameter has changed.

Edit: For those sniffs which have modular error codes which include the parameter name, the error code will change and this should be noted in the changelog as a BC break. Sniff names will still not be changed.
@jrfnl
Copy link
Member Author

jrfnl commented Dec 1, 2020

Related upstream PRs:

@jrfnl jrfnl self-assigned this Dec 2, 2020
@jrfnl jrfnl added this to the 10.0.0 milestone Dec 2, 2020
This was referenced Oct 26, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Oct 26, 2022
Merged
Merged
@jrfnl jrfnl mentioned this issue Nov 3, 2022
Merged
This was referenced Dec 6, 2022
Merged
Merged
Merged
Merged
@jrfnl jrfnl mentioned this issue Jan 12, 2023
Closed
@jrfnl jrfnl mentioned this issue Sep 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jrfnl jrfnl

Labels
chores/QA good first issue
Projects
None yet
Milestone
10.0.0
Development

No branches or pull requests
1 participant
@jrfnl