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

IfStatementAssignment thrown incorrectly #661

Closed
6 tasks done
leewillis77 opened this issue Aug 13, 2019 · 9 comments · Fixed by #671
Closed
6 tasks done

IfStatementAssignment thrown incorrectly #661

leewillis77 opened this issue Aug 13, 2019 · 9 comments · Fixed by #671
Labels
Milestone

Comments

@leewillis77
Copy link

  • PHPMD version: 2.7.0
  • PHP Version: 7.1.30
  • Installation type: composer
  • Operating System / Distribution & Version: MacOS Mojave

Current Behavior

Sample code below throws two IfStatementAssignment errors.

<?php

function bar($thing, $ids)
{
    if ($thing->status === 'CREATED') {
        $ids->each(function ($id) use ($thing) {
            $allocation         = find($id);
            $allocation->status = 'paid';
        });
    }
}

The errors reported are:
test.php:3 Avoid assigning values to variables in if clauses and the like (line '13', column '7').
test.php:3 Avoid assigning values to variables in if clauses and the like (line '13', column '8').

Accounting for the incorrect line/column numbers (see #655), this is complaining about the two assignments within the closure.

Expected Behavior

No IfStatementAssignment errors should be thrown as is the case if the wrapping function is removed, e.g. the code below throws no IfStatementAssignment errors.

<?php

if ($thing->status === 'CREATED') {
    $ids->each(function ($id) use ($thing) {
        $allocation         = find($id);
        $allocation->status = 'paid';
    });
}

Steps To Reproduce:

See examples above.

Checks before submitting

  • Be sure that there isn't already an issue about this. See: Issues list
  • Be sure that there isn't already a pull request about this. See: Pull requests
  • I have added every step to reproduce the bug.
  • If possible I added relevant code examples.
  • This issue is about 1 bug and nothing more.
  • The issue has a descriptive title. For example: "JSON rendering failed on Windows for filenames with space".
@ravage84 ravage84 added the Bug label Aug 13, 2019
@ravage84 ravage84 added this to the 2.7.1 milestone Aug 13, 2019
@kylekatarnls
Copy link
Member

I tried to get a minimum code chunk to reproduce the bug and get it with:

function bar()
{
    if (true) {
        array_filter(function ($foo) {
            $bar = $foo;
            echo $bar;
        });
    }
}

Minimum tokens needed seem to be: a statement, a function/method call with a closure parameter, an assignation inside the closure.

@wernerdweight
Copy link

Is there any estimation on when 2.7.1 (fixes this) could be released?

@kylekatarnls
Copy link
Member

kylekatarnls commented Aug 19, 2019

We just started to work on it: #668

@tvbeek tvbeek closed this as completed in ecd3015 Sep 3, 2019
tvbeek added a commit that referenced this issue Sep 3, 2019
…s-in-closures

Fix #661 Exclude FunctionPostfix from IfStatementAssignment
@kylekatarnls
Copy link
Member

Thanks for your help @leewillis77 it will be fixed in the next release, you can test it with the dev-master version.

@leewillis77
Copy link
Author

While the test cases posted here seem no longer to throw the error, I'm still seeing this error generated with my actual code.

I'm going to see if I can produce an alternative test case, would you prefer to re-open this issue, or would you prefer me to open a fresh one?

@leewillis77
Copy link
Author

Examples below:

Does NOT throw the error

<?php

function bar($ids)
{
    if ('CREATED' === 'CREATED') {
        $ids->each(function () {
            $allocation = find(1);
            $allocation->status = 'paid';
            $allocation->potato = 'round';
        });
        return true;
    }
}

Throws an error, but should not

<?php

function bar($ids, $allocation)
{
    if ('CREATED' === 'CREATED') {
        $ids->each(function () use ($allocation) {
            $allocation->status = 'paid';
            $allocation->potato = 'round';
        });
        return true;
    }
}

Cut-down version of real code that throws error, but should not

<?php

namespace App\Services;

use App\Models\Allocation;

class PayoutService
{
    public function payoutAllocations($allocationIds)
    {
        if ('CREATED' === 'CREATED') {
            $allocationIds->each(function ($allocationId) use ($payout) {
                $allocation                     = Allocation::find($allocationId);
                $allocation->status             = 'paid';
            });
        }
    }
}

@kylekatarnls
Copy link
Member

I found the actual problem. I will fix it.

@kylekatarnls kylekatarnls reopened this Sep 4, 2019
kylekatarnls added a commit to kylekatarnls/phpmd that referenced this issue Sep 4, 2019
tvbeek added a commit that referenced this issue Sep 5, 2019
…s-in-closures

Fix #661 Scan only the statement inner expression (first one)
@tvbeek
Copy link
Member

tvbeek commented Sep 5, 2019

@leewillis77 with thanks to @kylekatarnls there is a new fix on master. Can you verify it?

@leewillis77
Copy link
Author

Perfect - that no longer throws those errors on my test code, or the real code that triggered this initially. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

5 participants