Skip to content

Conversation

@pascalheidmann
Copy link

@pascalheidmann pascalheidmann commented Mar 14, 2022

Why this PR

Given following code:

class EnumType {
    /** @var class-string<\MabeEnum\Enum> */
    protected string $enumClass = \MabeEnum\Enum::class;

    public function convertToPHPValue($value) {
        if (empty($value)) {
            return null;
        }

        if (!$this->enumClass::hasValue($value)) {
            throw new InvalidArgumentException(
                sprintf(
                    'The value "%s" is not valid for the enum "%s". Expected one of ["%s"]',
                    $value,
                    $this->enumClass,
                    implode('", "', $this->enumClass::getValues())
                )
            );
        }

        return $this->enumClass::byValue($value);
    }
}

will crash as MabeEnumPhpstan\EnumDynamicReturnTypeExtension::getTypeFromStaticMethodCall() tries to call $staticCall->class->toString();. This happens cause $staticCall->class can be both PhpParser\Node\Name (no problem) but also PhpParser\Node\Expr which doesn't have the toString() method.

What does this PR do

While not feeling like the perfect solution this PR makes sure that we use the default Enum::getValues() if we can't statically determine which Enum will be used

…to fall back to default return type of enums
@marc-mabe
Copy link
Owner

@pascalheidmann Thanks for your PR.
Please could you create a test case out of it?

@pascalheidmann
Copy link
Author

pascalheidmann commented Mar 15, 2022

@marc-mabe I added a minimal test but not 100% sure if it works all as intended cause I had to change more lines in the test cases than I anticipated 🤔 Maybe changing the $staticCall->class instanceof Expr condition to !method_exists($staticCall->call, 'toString') might help.
edit: I tried the variant with method_exists but same result as with instanceof

@pascalheidmann
Copy link
Author

@marc-mabe any update on this?

@codecov-commenter
Copy link

codecov-commenter commented Apr 2, 2022

Codecov Report

Merging #13 (4c84136) into master (0a20a9f) will decrease coverage by 1.22%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master      #13      +/-   ##
============================================
- Coverage     60.82%   59.59%   -1.23%     
- Complexity       39       40       +1     
============================================
  Files             3        3              
  Lines            97       99       +2     
============================================
  Hits             59       59              
- Misses           38       40       +2     
Impacted Files Coverage Δ
src/EnumDynamicReturnTypeExtension.php 24.52% <0.00%> (-0.97%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@marc-mabe marc-mabe merged commit 4c84136 into marc-mabe:master Apr 2, 2022
@marc-mabe
Copy link
Owner

@pascalheidmann Sorry I totally forgot your PR. Now I found time to test your changes and everything looks good.
It's released as v2.0.1 already.

Thank you 👍

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.

4 participants