Skip internal variadic callables and fix variadic argument naming handling#20
Merged
Conversation
…DME and tests ### Motivation - Prevent generating unknown named arguments for internal/native callables (e.g., `sprintf`) which would be rejected by PHP while still allowing named variadic synthesis for user-defined callables. - Make the behavior around `ALLOW_NAMED_VARIADIC_ARGUMENTS` explicit in the documentation. - Add/adjust fixtures to cover both internal variadic and user-defined variadic cases. ### Description - Add detection of function reflection via `Reflection::getFunctionReflection` and pass a `shouldKeepVariadicArgsPositional` flag into `addNamesToArgs` when the callable is internal. - Modify `addNamesToArgs` to keep variadic arguments positional for internal callables and continue naming variadic entries for user-defined callables (using incremental names like `values1`, `values2`). - Rework argument naming flow to centralize naming logic and ensure the node's `args` are replaced only when changes were made. - Update `README.md` to clarify the default behavior of `ALLOW_NAMED_VARIADIC_ARGUMENTS` and add an explanation of differences between user-defined and internal callables, and add/modify test fixtures (`tests/DefaultStrategy/Fixture/internal_variadic.php.inc` and `tests/DefaultStrategy/Fixture/variadic.php.inc`). ### Testing - Ran the test suite with `vendor/bin/phpunit` which executed the updated fixtures and rule tests successfully.
…dling ### Motivation - Prevent generating invalid named arguments for internal/native variadic callables (e.g. `sprintf`) while preserving naming for user-defined variadic parameters. - Fix a bug where variadic arguments had their `name` set but were not appended to the resulting args list or marked as a change. - Clarify the README behavior for `ALLOW_NAMED_VARIADIC_ARGUMENTS` so users know when internal callables are skipped. ### Description - In `AddNamedArgumentsRector::refactor` detect if the call has variadic arguments and obtain the function reflection via `Reflection::getFunctionReflection`, and skip refactoring when the variadic call is to an internal function (`isInternal`). - Reworked `addNamesToArgs` to correctly handle variadic parameters by assigning an `Identifier`, appending the argument to `$namedArgs`, and setting `$hasChanges` before continuing the loop, and moved non-variadic naming into the correct branch. - Updated `README.md` to explain the nuanced behavior for `ALLOW_NAMED_VARIADIC_ARGUMENTS` and the difference between user-defined and internal variadic callables. - Added `tests/DefaultStrategy/Fixture/internal_variadic.php.inc` and adjusted `tests/DefaultStrategy/Fixture/variadic.php.inc` to reflect that internal variadic functions like `sprintf` are no longer converted. ### Testing - Ran the test suite with `vendor/bin/phpunit` and all tests completed successfully. - The added/updated fixtures under `tests/DefaultStrategy/Fixture` were exercised and matched expected outcomes.
Owner
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
sprintf) while preserving naming for user-defined variadic parameters.nameset but were not appended to the resulting args list or marked as a change.ALLOW_NAMED_VARIADIC_ARGUMENTSso users know when internal callables are skipped.Description
AddNamedArgumentsRector::refactordetect if the call has variadic arguments and obtain the function reflection viaReflection::getFunctionReflection, and skip refactoring when the variadic call is to an internal function (isInternal).addNamesToArgsto correctly handle variadic parameters by assigning anIdentifier, appending the argument to$namedArgs, and setting$hasChangesbefore continuing the loop, and moved non-variadic naming into the correct branch.README.mdto explain the nuanced behavior forALLOW_NAMED_VARIADIC_ARGUMENTSand the difference between user-defined and internal variadic callables.tests/DefaultStrategy/Fixture/internal_variadic.php.incand adjustedtests/DefaultStrategy/Fixture/variadic.php.incto reflect that internal variadic functions likesprintfare no longer converted.Testing
vendor/bin/phpunitand all tests completed successfully.tests/DefaultStrategy/Fixturewere exercised and matched expected outcomes.