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

Psalm level 5 #2408

Merged
merged 1 commit into from
Feb 18, 2022
Merged

Psalm level 5 #2408

merged 1 commit into from
Feb 18, 2022

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Feb 16, 2022

Q A
Type task
BC Break no
Fixed issues

Summary

Replaces #2329

Since it's quite difficult and time consuming to fix all possible issues found by static analysis, I think it's better if we use a baseline to ignore some of the issues for now and we set a higher level so new code has to comply with that level.

I've targeted 2.3.x since there are some phpdoc fixed and it's not adding new features.

Copy link
Member

@malarzm malarzm left a comment

Choose a reason for hiding this comment

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

Awesome 🎉

@@ -571,7 +571,7 @@ private function addIndex(ClassMetadata $class, SimpleXMLElement $xmlIndex): voi
}

/**
* @return array<string, array<string, mixed>|scalar>
* @return array<string, array<string, mixed>|scalar|null>
Copy link
Member

Choose a reason for hiding this comment

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

null doesn't look like an acceptable value for the array but I guess it comes from convertXMLElementValue so let's live with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, exactly

* @psalm-return (
* $arrayAccess is false
* ? T|null
* : T|null|true
Copy link
Member

Choose a reason for hiding this comment

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

oh boy, now we're programming in comments :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😬 this can be really interesting when adding generics for example to Query, that depending on the hydrate value, maybe we change the return type of Query::execute() to just Iterator<TDocument> which would mean removing quite assert calls.

@@ -27,13 +26,6 @@ public function getRealClass(string $class): string
return $this->resolveClassName($class);
}

/**
* @psalm-param class-string<RealClassName>|class-string<ProxyInterface<RealClassName>> $className
Copy link
Member

Choose a reason for hiding this comment

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

These are not needed anymore or just problematic?

Copy link
Contributor Author

@franmomu franmomu Feb 16, 2022

Choose a reason for hiding this comment

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

This is interesting, we were getting:

ERROR: MoreSpecificImplementedParamType - lib/Doctrine/ODM/MongoDB/Proxy/Resolver/ProxyManagerClassNameResolver.php:37:45 - Argument 1 of Doctrine\ODM\MongoDB\Proxy\Resolver\ProxyManagerClassNameResolver::resolveClassName has the more specific type 'class-string<ProxyManager\Proxy\ProxyInterface<RealClassName>>|(class-string<RealClassName:fn-doctrine\odm\mongodb\proxy\resolver\proxymanagerclassnameresolver::resolveclassname as object>)', expecting 'class-string<Doctrine\Persistence\Proxy<T>>|(class-string<T:fn-doctrine\persistence\mapping\proxyclassnameresolver::resolveclassname as object>)' as defined by Doctrine\Persistence\Mapping\ProxyClassNameResolver::resolveClassName (see https://psalm.dev/140)
    public function resolveClassName(string $className): string

so I've removed it, but didn't pay attention that in doctrine/persistence, the interface is using Proxy:

<?php

namespace Doctrine\Persistence\Mapping;

use Doctrine\Persistence\Proxy;

interface ProxyClassNameResolver
{
    /**
     * @psalm-param class-string<Proxy<T>>|class-string<T> $className
     *
     * @psalm-return class-string<T>
     *
     * @template T of object
     */
    public function resolveClassName(string $className): string;
}

we are usingocramius/proxy-manager, so I'll leave it and ignore it since it adds the correct type.

It adds a baseline to reach level 5
@malarzm
Copy link
Member

malarzm commented Feb 18, 2022

I think I've removed correct checks from the list of required ones, let's hope next PRs can be merged not only by me 👍

@malarzm malarzm merged commit 9d22a8f into doctrine:2.3.x Feb 18, 2022
@malarzm malarzm added the Task label Feb 18, 2022
@malarzm malarzm added this to the 2.3.2 milestone Feb 18, 2022
@franmomu franmomu deleted the phpstan_level_6 branch February 18, 2022 10:34
@franmomu franmomu mentioned this pull request Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants