Skip to content

Add an ReplaceArgumentsRector#33

Draft
zoglo wants to merge 5 commits intocontao:mainfrom
zoglo:feat/add-argument-rector
Draft

Add an ReplaceArgumentsRector#33
zoglo wants to merge 5 commits intocontao:mainfrom
zoglo:feat/add-argument-rector

Conversation

@zoglo
Copy link
Copy Markdown
Member

@zoglo zoglo commented Aug 7, 2025

Description

Similar to the replaceNestedArrayItemRector, this allows you to simply configure all arguments that you would want within a static method call. We could open it up for other node types than just of type StaticCall but that's not necessary right now.
Or should we open it for MethodCall and ClassMethod as well 👀 ?

-\Contao\Input::stripTags(null, '');
+\Contao\Input::stripTags(null, '', \Contao\Config::get('allowedAttributes'));
$rectorConfig->ruleWithConfiguration(AddArgumentsRector::class, [
    new AddArguments(Input::class, 'stripTags', [
        2 => new StaticCall(new FullyQualified(Config::class), 'get', [new Arg(new String_('allowedAttributes'))])
    ]),
]);
  • See tests for more examples
  • Also added the same magic as the replaceNestedArrayItemValue so strings are parsed as such

Edit

This one would have just added arguments on any run, fb82b31 changes this to apply a key to the array values, also allows a string as the array key to allow replacing / or adding named parameters.

The updated tests should explain the behavior

@zoglo zoglo requested a review from aschempp August 7, 2025 17:45
@zoglo zoglo self-assigned this Aug 7, 2025
@zoglo zoglo added the feature label Aug 7, 2025
@zoglo
Copy link
Copy Markdown
Member Author

zoglo commented Aug 7, 2025

Additionally, we could replace Input::stripTags($value) (just one argument) with a simple strip_tags() call, since that would be faster and the same use case.

See #30 (comment) by @ausi why this wouldn't work 😊

@aschempp
Copy link
Copy Markdown
Member

wouldn't that add the argument to any method call as the last, regardless of the number of existing parameters?

@zoglo
Copy link
Copy Markdown
Member Author

zoglo commented Aug 19, 2025

wouldn't that add the argument to any method call as the last, regardless of the number of existing parameters?

Yes. Do you want us to modify it in a way to allow passing the position? (Unsure if named arguments should be part as well, might be overkill).

Hence why I asked for an example. This one would work with the given one but there may be more.

@zoglo zoglo marked this pull request as draft August 19, 2025 15:11
@zoglo zoglo changed the title Add an AddArgumentsRector Add an ReplaceArgumentsRector Aug 19, 2025
@zoglo zoglo marked this pull request as ready for review August 19, 2025 19:50
@zoglo
Copy link
Copy Markdown
Member Author

zoglo commented Aug 19, 2025

The previous version would have just added arguments on any run (thanks @aschempp for pointing that out, just out of habit, add a rule, refactor, remove the rule 🐒).

fb82b31 changes this to consider the array key, also allows a string as the array key to allow replacing / or adding named parameters.

The updated tests should explain the behavior.

@aschempp
Copy link
Copy Markdown
Member

aschempp commented Oct 9, 2025

If I understand the code correctly, this would now always override the argument at the given position, correct? In case of Input::stripTags, it should only add the argument if it is not present. If there already is an argument – whatever the developer decided on – it should (obviously) not be changed.

Is that currently possible somehow? Maybe a fourth $force argument ModifyArguments would do? Even though I can't really imagine a case where I want to replace an existing argument, I only ever want to add new defaults that should be necessary 🤔

@zoglo
Copy link
Copy Markdown
Member Author

zoglo commented Oct 9, 2025

Is that currently possible somehow? Maybe a fourth $force argument ModifyArguments would do? Even though I can't really imagine a case where I want to replace an existing argument, I only ever want to add new defaults that should be necessary 🤔

We did agree on adding a fourth argument with $replace = true|false, I didn't have the time to add this yet 😁

@zoglo zoglo marked this pull request as draft November 27, 2025 19:24
@aschempp aschempp removed their request for review December 23, 2025 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants