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

Bump psalm level #6977

Merged
merged 6 commits into from
Apr 11, 2021
Merged

Bump psalm level #6977

merged 6 commits into from
Apr 11, 2021

Conversation

VincentLanglet
Copy link
Member

No description provided.

@VincentLanglet
Copy link
Member Author

@sonata-project/contributors There is the following psalm error:

ERROR: InvalidNullableReturnType - src/FieldDescription/TypeGuesserChain.php:54:73 - The declared return type 'Symfony\Component\Form\Guess\TypeGuess' for Sonata\AdminBundle\FieldDescription\TypeGuesserChain::guess is not nullable, but 'Symfony\Component\Form\Guess\TypeGuess|null' contains null (see https://psalm.dev/144)
    public function guess(FieldDescriptionInterface $fieldDescription): TypeGuess


ERROR: NullableReturnStatement - src/FieldDescription/TypeGuesserChain.php:62:16 - The declared return type 'Symfony\Component\Form\Guess\TypeGuess' for Sonata\AdminBundle\FieldDescription\TypeGuesserChain::guess is not nullable, but the function returns 'Symfony\Component\Form\Guess\TypeGuess|null' (see https://psalm.dev/139)
        return TypeGuess::getBestGuess($guesses);

It's right, the TypeGuesserChain::guess could return null, for example if this class was created with no guesser.
But our persistent bundle are using it as there will always be a guess.

Should I add null to the return type and then the persistence bundle will throw an error if there is no guess (asking the developer to explicitely passing the type). Or should we add an exception in the TypeGuesserChain if no guess is found ?

@VincentLanglet VincentLanglet requested a review from a team March 28, 2021 23:38
@VincentLanglet VincentLanglet force-pushed the psalm branch 2 times, most recently from d18a63a to 4fb91e4 Compare March 31, 2021 21:28
@VincentLanglet VincentLanglet force-pushed the psalm branch 4 times, most recently from 6f97ce9 to 04ebda1 Compare April 1, 2021 08:27
@VincentLanglet VincentLanglet marked this pull request as ready for review April 1, 2021 10:16
@VincentLanglet
Copy link
Member Author

The PR is now ready @sonata-project/contributors :)

src/Admin/AbstractAdmin.php Show resolved Hide resolved
src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
@SonataCI
Copy link
Collaborator

SonataCI commented Apr 1, 2021

Could you please rebase your PR and fix merge conflicts?

src/Action/SetObjectFieldValueAction.php Outdated Show resolved Hide resolved
src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Member Author

@wbloszyk I just discover that setTemplateRegistry is called in the AddDependencyCallsCompilerPass
I think setTemplates and setTemplate are called too.

The TaggedAdminInterface was meant to define all the setter called by this compiler pass.
Extending AbstractTaggedAdmin was supposed to be enough to work with the sonata.admin tag.

Should we move the methods from the MutableTemplateRegistryAwareInterface to the TaggedAdminInterface and from AbstractAdmin to AbstractTaggedAdmin ?
Or at least should the TaggedAdminInterface extends MutableTemplateRegistryAwareInterface ?

src/Model/AuditManager.php Outdated Show resolved Hide resolved
@@ -55,6 +55,8 @@ public function findOneBy(string $class, array $criteria = []): ?object

/**
* @return Foo|null
*
* @psalm-suppress ImplementedReturnTypeMismatch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return phpdoc is wrong imo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    public function find(string $class, $id): ?object
    {
        return $this->repository->byId($id);
    }

Since the repository is a Foo repository, it always return Foo or null.

I had some trouble with this issue since the modelManager is suppose to return a value corresponding to the class, but is not doing this job in the tests...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But did you try to remove the phpdoc? the interface already have that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove the Phpdoc, I end with another error, from phpstan maybe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the phpdoc I get

phpstan: Method Sonata\AdminBundle\Tests\App\Model\ModelManager::find() should return T of object|null but returns Sonata\AdminBundle\Tests\App\Model\Foo|null.
psalm: InvalidReturnStatement: The inferred type 'Sonata\AdminBundle\Tests\App\Model\Foo|null' does not match the declared return type '(T:fn-sonata\adminbundle\model\modelmanagerinterface::find as object)|null' for Sonata\AdminBundle\Tests\App\Model\ModelManager::find

tests/Datagrid/DatagridMapperTest.php Show resolved Hide resolved
@VincentLanglet VincentLanglet requested a review from a team April 6, 2021 23:32
@VincentLanglet VincentLanglet requested review from jordisala1991 and a team April 8, 2021 17:59
jordisala1991
jordisala1991 previously approved these changes Apr 10, 2021
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@VincentLanglet VincentLanglet requested a review from a team April 10, 2021 10:07
@OskarStark OskarStark merged commit 84c87f8 into sonata-project:master Apr 11, 2021
Comment on lines +17 to +19
<InternalClass errorLevel="suppress"/>
<InternalMethod errorLevel="suppress"/>
<InternalProperty errorLevel="suppress"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why suppressing these? can we add a comment?

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.

5 participants