ClosureExpressionVisitor > Don't duplicate the accessor when the field already starts with it

[ruudk]

This PR tries to fix an issue I am facing in my application that uses Doctrine2 ORM.

I have the following setup:

class Platform {
    // ..

    /**
     * @ORM\OneToMany(targetEntity="PaymentMethod", mappedBy="platform")
     */
    private $paymentMethods;
}

class PaymentMethod {
    // ...

    /**
     * @ORM\Column(type="boolean")
     */
    private $isActive = true;
}

I'm trying to get all PaymentMethods from the Platform that are active:

class Platform {
    // ..

    public function getActivePaymentMethods() : Collection
    {
        $criteria = Criteria::create()
            ->where(Criteria::expr()->eq('isActive', true));

        return $this->paymentMethods->matching($criteria);
    }
}

When I call $platform->getActivePaymentMethods() I get:

  [Symfony\Component\Debug\Exception\FatalThrowableError]
  Cannot access private property PaymentMethod::$isActive

I can fix this of course by changing my Criteria to Criteria::expr()->eq('active', true) but then I will get in troubles when it tries to run the expression on the PersistentCollection instead of the ArrayCollection.

This fix makes the getter a bit smarter. When you prefix the field with is* it doesn't re-apply is as an accessor. Instead of isIsActive it will just do isActive.

[ruudk]

Is there anything I can do to bring this PR forward? I'm currently using this fork in my projects, would be great to rely on a stable tag again :D 🙏

[malarzm]

I'm afraid this check on its own will lead to false positives: issue, island, isolation. Better heuristic would be to look for is followed by a capital letter

[malarzm]

But then it can be worse performace-wise, not sure if it's worth the hassle

[ruudk]

@malarzm I implemented a different approach: f965654

This way, it will always call $object->$field() when the method exists.

[ruudk]

I added another approach, using a nice regex, in 7fa9e35. I think this is the winner. What do you think?

I will cleanup/squash the commits once its ok.

[ruudk]

@malarzm what does it take to get this merged? Thanks :)

[ruudk]

Rebased the PR, squashed it and applied Doctrine CS.

[ruudk]

@Ocramius do you think this can be merged? If not, what is needed to get it to a mergable state?

[SenseException]

I would like to have this PR merged to prevent keeping it longer on hold for a second year. Except my question on the Regex, there's nothing to add from my side.

On a side note: method_exists also returns true when the method is private (https://3v4l.org/4pBgc). This part of the library was always prone to bugs, but maybe we can handle this differently in another PR, if it is desired.

[SenseException]

@Ocramius Do we need this Regex check? Having only the method_exists(), we could even use accessor-methods that haven't a is or get prefix.

[alcaeus]

I agree, let's get this merged. TBH, I would keep the logic as-is and rather refactor this in a separate pull request.

@ruudk could you rebase one last time? Thanks!

[ruudk]

Done. Should I squash all the commits into 1 or do you do this on merge?

[alcaeus]

If you don't mind I'd appreciate a squash before merging - less work for me 😉

[ruudk]

Done :)

[alcaeus]

phpcs is still complaining: https://travis-ci.org/doctrine/collections/jobs/501915949#L558

You can fix this by running vendor/bin/phpcbf lib/Doctrine/Common/Collections/Expr/ClosureExpressionVisitor.php.

[ruudk]

Fixed it manually...

PHPCS doesn't work really nice, see:

phpcbf lib/Doctrine/Common/Collections/Expr/ClosureExpressionVisitor.php
ERROR: Referenced sniff "Doctrine" does not exist

Run "phpcbf --help" for usage information

                                                                                                                                                                                                                   0.000001d 0.000019h 0.001133m 0.068s | 18:06
/w/collections (expression-visitor|✔) $ vendor/bin/phpcbf lib/Doctrine/Common/Collections/Expr/ClosureExpressionVisitor.php
PHP Fatal error:  Uncaught Error: Class 'PHP_CodeSniffer\Runner' not found in /Volumes/CS/www/collections/vendor/bin/phpcbf:17
Stack trace:
#0 {main}
  thrown in /Volumes/CS/www/collections/vendor/bin/phpcbf on line 17

Fatal error: Uncaught Error: Class 'PHP_CodeSniffer\Runner' not found in /Volumes/CS/www/collections/vendor/bin/phpcbf:17
Stack trace:
#0 {main}
  thrown in /Volumes/CS/www/collections/vendor/bin/phpcbf on line 17
                                                                                                                                                                                                                    0.000001d 0.000019h 0.001167m 0.07s | 18:06
/w/collections (expression-visitor|✔) $ composer install


  [Symfony\Component\Process\Exception\ProcessFailedException]
  The command "'/Volumes/CS/www/collections/vendor/bin/phpcs' '--config-show' 'installed_paths'" failed.

  Exit Code: 255(Unknown error)

  Working directory: /Volumes/CS/www/collections

  Output:
  ================

  Fatal error: Uncaught Error: Class 'PHP_CodeSniffer\Runner' not found in /Volumes/CS/www/collections/vendor/bin/phpcs:17
  Stack trace:
  #0 {main}
    thrown in /Volumes/CS/www/collections/vendor/bin/phpcs on line 17


  Error Output:
  ================
  PHP Fatal error:  Uncaught Error: Class 'PHP_CodeSniffer\Runner' not found in /Volumes/CS/www/collections/vendor/bin/phpcs:17
  Stack trace:
  #0 {main}
    thrown in /Volumes/CS/www/collections/vendor/bin/phpcs on line 17

Nothing works.

[alcaeus]

Thanks for the heads up, and sorry you had to fix it manually. Let me take another look at that after taking care of your PR 👍

[alcaeus]

FWIW, build completed successfully but build result wasn’t reported back correctly: https://travis-ci.org/doctrine/collections/builds/502138875.

Thanks @ruudk!

[ruudk]

Wow, thanks for merging it 🙌🥳

[alcaeus]

Don’t celebrate too soon, need to dig into the changes before releasing a new version. I hope to be able to do it shortly.