Skip to content

Named arguments in refactorings #9087

Closed

Description

@mamazu

Question

I have the following code I want to refactor:

function getRequestParameter(Request $request, string $parameter, bool $force = false, $default = null);

// Example:
$this->getRequestParameter($request, 'locale', true);
$this->getRequestParameter($request, 'page', false, '1');

This should be converted to Symfony's $request->get function.

The current approach

My current approach is to reorder and reuse the arguments based on their position in the call.=

        /** @var MethodCall $node */
        if ($this->nodeNameResolver->isName($node->var, '$this') || (string) $node->name !== 'getRequestParameter' ){ return; }

        $request = $node->args[0]->value;
        $parameterName = $node->args[1]->value;
        $force = $node->args[2]?->value ?? $this->nodeFactory->createFalse();
        $default = $node->args[3]?->value ?? $this->nodeFactory->createNull();

But this raises the question: What is the best way to cover named arguments. Calls like this:

$this->getRequestParameter($request, 'locale', default: 'en');

Is there already code in rector to handle this better? If not would be happy to provide an implementation.

Activity

  1. TomasVotruba commented on Apr 2, 2025

    @TomasVotruba
    Member

    You can set argument name like this:

    $node->args[1]->name = '...'

    See https://getrector.com/ast/defc4808206c4161b76f5bbff542c6a7dc90e4ec

  2. mamazu commented on Apr 2, 2025

    @mamazu
    Author

    The problem is not the setting. The problem is getting the parameters from the old code. Because depending on the value of the "force" parameter in the function the rector should work different. But with named arguments I can't garantuee that in the old code the "force" parameter exists or is in the third position. Technically the old code could look like this:

    $this->getRequestParameter(force: true, parameter: 'locale', default: 'en', request: $request);
  3. mamazu commented on Apr 2, 2025

    @mamazu
    Author

    This could then also allow us to make this work: Removing parameters by name instead of positions: https://getrector.com/demo/a0594762-5100-4e64-a761-74adc9eed071

  4. TomasVotruba commented on Apr 2, 2025

    @TomasVotruba
    Member

    In that case, you need to do a reflection on getRequestParameter() method. Using PHPStan\Reflection\ReflectionProvider service

  5. samsonasik commented on Apr 2, 2025

    @samsonasik
    Member

    You can enforce to remove named argument first, by using DowngradeNamedArgumentRector rule first, then it always sorted argument, see

    https://getrector.com/demo/70dddd06-9db6-442b-a58a-4fc7a49a059f

    after it sorted, use your custom rule to remove specific position.

  6. mamazu commented on Apr 2, 2025

    @mamazu
    Author

    I don't understand how you would use the ReflectionProvider. If I do I get a MethodReflection where most of the important things are private: https://apiref.phpstan.org/1.8.x/PHPStan.Reflection.Php.PhpMethodReflection.html

    And as far as the DowngradeNamedArgumentRector that sounds like a doable solution. However I think having a service that could just take a list of arguments and convert them into a map, would make things easier. I also found other code which requires named arguments.

  7. mamazu commented on Apr 3, 2025

    @mamazu
    Author

    If you're interested what solution I went with it's something like this:

        /**
         * Turns a list of parameters into a hash map of named parameters.
         * If the parameter is not specified, it will use the default value from the definitions array.
         *
         * @param array<Arg> $arguments
         * @param array<string, Expr|null> $definitions
         *
         * @return array<string, Expr|null>
         */
        private function arguments(array $arguments, array $definitions): array
        {
            $currentArgumentPosition = 0;
            $names = \array_keys($definitions);
            $result = [];
            foreach ($arguments as $argument) {
                if (null === $argument->name) {
                    // Get the argument name based on the position
                    $name = $names[$currentArgumentPosition++];
                } else {
                    // Get the argument name from the named argument
                    $name = $this->getName($argument->name);
                }
                $result[$name] = $argument->value;
                \unset($definitions[$name]);
            }
    
            // Concatenate the remaining defaulted arguments
            return [...$definitions, ...$result];
        }

    and you can use it like that:

            $arguments = $this->arguments($args, [
                'request' => null,
                'parameterName' => null,
                'force' => $this->nodeFactory->createFalse(),
                'default' => null,
            ]);

    The keys are the parameter names the function should have and the values are the default values of this function. I'm sure this could be generalized with Reflection, but as I said I couldn't figure out how this would work with the phpstan reflections.

  8. staabm commented on Apr 6, 2025

    @staabm
    Contributor

    You might be interested in PHPStan ArgumentsNormalizer which primary job is to re-write a named arguments list into a regular one

    https://github.com/phpstan/phpstan-src/blob/2.1.x/src/Analyser/ArgumentsNormalizer.php

  9. locked and limited conversation to collaborators on Apr 8, 2025
  10. converted this issue into a discussion #9101 on Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions