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

[Downgrade PHP 7.3] Trailing commas in function calls #4273

Conversation

quisse
Copy link
Contributor

@quisse quisse commented Sep 22, 2020

@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch 3 times, most recently from 0123b42 to 99b5e0c Compare September 22, 2020 14:57
@quisse quisse marked this pull request as draft September 22, 2020 15:01
@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch from 99b5e0c to 31d7749 Compare September 22, 2020 15:06
@quisse quisse marked this pull request as ready for review September 22, 2020 15:12
@quisse quisse marked this pull request as draft September 22, 2020 16:02
@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch from 31d7749 to bf0dcf7 Compare September 23, 2020 08:50
@quisse
Copy link
Contributor Author

quisse commented Sep 23, 2020

After some digging I can conclude that this is not possible using php-parser because php-parser isn't aware of the trailing comma AFAIK.
Any help would be appreciated.

@TomasVotruba
Copy link
Member

TomasVotruba commented Sep 23, 2020

I came to similar conslussion when PHP 7.3 was released.

Maybe php-cs-fixer is better for this job

@quisse
Copy link
Contributor Author

quisse commented Sep 23, 2020

So is including php-cs-fixer as a dependency an option? It would be nice if rectorphp makes downgrading every php version possible.

@quisse
Copy link
Contributor Author

quisse commented Sep 23, 2020

Triggering regeneration removes the trailing commas. The only side effect I'm aware of is that when having a method call inline in a string in a method call, the curly brackets are doubled.

    public function refactor(Node $node): ?Node
    {
        if($node->args){
            $node->setAttribute(AttributeKey::ORIGINAL_NODE,null);
        }
        return $node;
    }

Makes

        $this->setOnClick("[Zip ID: {$modelid}]  {$e->getMessage($modelId,)}");

results in

        $this->setOnClick("[Zip ID: {$modelid}]  {{$e->getMessage($modelId)}}");

A possible workaround could be

    public function refactor(Node $node): ?Node
    {
        if($node->args){
            if(! $node->getAttribute(AttributeKey::PARENT_NODE) instanceof Node\Scalar\Encapsed){
                $node->setAttribute(AttributeKey::ORIGINAL_NODE,null);
            }
        }
        return $node;
    }

But the trailing comma isn't removed then.

@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch 3 times, most recently from 122eeb9 to 838d315 Compare September 23, 2020 22:21
@TomasVotruba
Copy link
Member

Removing original node usually lead to reprint without format. Then it depends on default printer setting.

What could work is do is hack around BetterStandardPrinter and remove comma there.

@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch from 838d315 to 4f0a8a8 Compare October 31, 2020 09:57
@quisse
Copy link
Contributor Author

quisse commented Nov 7, 2020

With this one I am waiting for nikic/PHP-Parser#732.
That issue causes that "[Zip ID: {$modelid}] {$e->getMessage($modelId)}" is rewritten as "[Zip ID: {$modelid}] {{$e->getMessage($modelId)}}"

@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch 2 times, most recently from f24d0b0 to 5846e21 Compare November 8, 2020 13:58
@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch from 5846e21 to b245d36 Compare December 3, 2020 19:03
@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch from b245d36 to 369fc93 Compare December 3, 2020 19:17
@quisse quisse force-pushed the downgrade-php73-downgrade-trailing-commas-in-function-calls branch from 369fc93 to 0c39734 Compare December 3, 2020 19:32
@quisse
Copy link
Contributor Author

quisse commented Dec 3, 2020

@TomasVotruba updating nikic/phpparser doesn't seem sufficient, do I need to wait for phpstan to include the latest phpparser or am I missing something?

@TomasVotruba
Copy link
Member

I'll look into this

@TomasVotruba
Copy link
Member

These 2 last details are ok. Do not change the code meaning and solves the comma.

Merged in: #4796

Thank you 👍

@quisse quisse deleted the downgrade-php73-downgrade-trailing-commas-in-function-calls branch December 7, 2020 14:52
TomasVotruba added a commit that referenced this pull request Jun 18, 2023
rectorphp/rector-src@3806bb8 [NodeManipulator] Reduce parent lookup on PropertyManipulator under Unset_ (#4273)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants