add psalm to project

[vv12131415]

Please add pedantic tag here.

I think it's time to psalm, so that phpstan and psalm can work together on our code.

Since I can't edit .gitattributes I'm thinking of creating .psalm folder OR even .static-analysis folder which will contain psalm and phpstan folders. What do you think about it?

Also there is 1 issue left, I don't know how to fix it the right way

ERROR: InvalidPropertyAssignmentValue - src/Admin/FieldDescriptionCollection.php:108:27 - $this->elements with declared type 'array<array-key, Sonata\AdminBundle\Admin\FieldDescriptionInterface>' cannot be assigned type 'array<array-key|mixed, Sonata\AdminBundle\Admin\FieldDescriptionInterface|array-key>' (see https://psalm.dev/145)
        $this->elements = array_merge(array_flip($keys), $this->elements);

What I can do, is just ignore it with baseline and fix it latter (in v4.x), but that's not solving the problem.

---

What I don't like is, that xml sniff forces me to write properties to psalm.xml in one row

<psalm xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="https://getpsalm.org/schema/config" errorLevel="6" resolveFromConfigFile="true" xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" errorBaseline="psalm-baseline.xml">

vs

<psalm xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
       xmlns="https://getpsalm.org/schema/config" 
       errorLevel="6"
       resolveFromConfigFile="true"
       xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
       errorBaseline="psalm-baseline.xml"
>

Changelog

### Deprecated
- Deprecated passing `callable` that does not return `Symfony\Component\Routing\Route` as `$element` (2nd argument) to `Sonata\AdminBundle\Route\RouteCollection::addElement($code, $element)`

[greg0ire]

What I don't like is, that xml sniff forces me to write properties to psalm.xml in one row

Yeah I never liked this either, sadly that's the only xml linter we could find.

Also there is 1 issue left, I don't know how to fix it the right way

I think the right way would be

+    /**
+     * @param array<array-key> $keys
+     */
     public function reorder(array $keys)
     {
         if ($this->has('batch')) {
             array_unshift($keys, 'batch');

Since I can't edit .gitattributes I'm thinking of creating .psalm folder OR even .static-analysis folder which will contain psalm and phpstan folders. What do you think about it?

I think that would be great, but I would remove the dot: https://twitter.com/greg0ire/status/1065906445718294528

I also love the directory structure we have here: https://github.com/doctrine/dbal/tree/3.0.x

[vv12131415]

@ greg0ire
I think you meant something other then

+    /**
+     * @param array-key $keys
+     */
     public function reorder(array $keys)
     {
         if ($this->has('batch')) {
             array_unshift($keys, 'batch');

I've tried to set diffrent values, but came to conclusion that I just don't understand how it works.

if $keys is array<array-key, mixed> and $this->elements is FieldDescriptionInterface[]
then reorder just breaks types. Because array_flip($keys) means that the type of value of the array is array-key => int|string, it just can't be FieldDescriptionInterface

[greg0ire]

Sorry yeah, I fixed my message, and I realise now even that isn't enough. IMO the type on the elements property must be wrong or reorder breaks everything, as you said it just relies on how array_merge behaves when 2 keys are the same, the code is fine. Psalm just doesn't know that every key in the first array gets overwritten because it doesn't know that the input keys are also keys used in $this->elements. In fact, we never check it so it's right… I think a way to fix it would be to use foreach, and throw if a key in the input array isn't also a key used in $this->elements. We might also want to check that both arrays are the same size.

[VincentLanglet]

In fact, we never check it so it's right… I think a way to fix it would be to use foreach, and throw if a key in the input array isn't also a key used in $this->elements. We might also want to check that both arrays are the same size.

+1 for the foreach. It will allow to check that we don't try to reorder a non-existant key.

[vv12131415]

@greg0ire

I think that would be great, but I would remove the dot: twitter.com/greg0ire/status/1065906445718294528

I would like to not publish phpstan/psalm configs/baseline on packagist this can be achiveed with folder with dot notation. Do you know any other way how can I do this?

[greg0ire]

I would like to not publish phpstan/psalm configs/baseline on packagist this can be achiveed with folder with dot notation. Do you know any other way how can I do this?

Contribute something here: https://github.com/sonata-project/dev-kit/blob/master/templates/project/.gitattributes.twig ?

We might want to add a ci directory? Or maybe static-analysis?

[greg0ire]

I think a way to fix it would be to use foreach, and throw if a key in the input array isn't also a key used in $this->elements. We might also want to check that both arrays are the same size.

Maybe that should be done on master since it is kind of a BC-break, and the error should be ignored for the moment? You can have a look at doctrine/dbal if you want examples of how to ignore errors.

[vv12131415]

@greg0ire thank you for showing how to ignore it, I would like to add it to baseline, because I don't want config file to grow big. Most of them will be fixed in v4 (because currently it's BC break)

[vv12131415]

the test fails because of discussion in #6288 (comment)

[greg0ire]

I would like to add it to baseline

Ok to me since it comes with comments, it still looks maintainable.

[vv12131415]

I've removed phpstan/phpstan-symfony from PHPStan config since it's randomly failing on CI (see phpstan/phpstan-symfony#94 for details)

[vv12131415]

I don't understand how I broke tests in HelperControllerTest.

Somewhy it was caused by phpdocumentor/reflection-docblock I've added it as dev dependency to make sure the lowest version is the one where tests/code don't fail.

[VincentLanglet]

Somewhy it was caused by phpdocumentor/reflection-docblock I've added it as dev dependency to make sure the lowest version is the one where tests/code don't fail.

I would love to avoid doing this if we don't directly use this library.
If we used https://packagist.org/packages/icanhazstring/composer-unused, the library you added would be reported.

The error you got is indeed because of 4.0.0 of phpdocumentor/reflection-docblock, it was fixed in 4.0.1.
phpDocumentor/ReflectionDocBlock#111

The question would be, why does it install 4.0.0 ?

composer why phpdocumentor/reflection-docblock
felixfbecker/advanced-json-rpc  v3.0.3  requires  phpdocumentor/reflection-docblock (^4.0.0)

Then

composer why felixfbecker/advanced-json-rpc
vimeo/psalm  3.13.1  requires  felixfbecker/advanced-json-rpc (^3.0.3)

Seems like psalm had the same issue: vimeo/psalm#3967

I made a PR to fix this: felixfbecker/php-advanced-json-rpc#41
Then I will make a PR to psalm.

[vv12131415]

@VincentLanglet thanks a lot! Should I just wait?

[jordisala1991]

@greg0ire do you have access to the scrutinizer configuration? this could be a problem

[greg0ire]

I have access to the config for the repository, but not the one for the global config which is shared with it. Anyway, not a huge deal since Scrutinizer is not required for merges. I'm not sure what the issue is by the way… maybe it's using an outdated version of PHP?

[jordisala1991]

To me it looks like an outdated version of composer. The thing is: if we merge this (which is a great pr btw) we wont have scrutinizer working anymore until someone with access to the global config makes some changes to it. Maybe @rande has access or can give access to someone else?

[vv12131415]

By the way, I have emailed to [email protected] and have no response from them

[rande]

is scrutinizer-ci.com still a thing ? we can remove that, I will log into it I check if there are any options to upgrade composer.

[vv12131415]

As I can see, last blog post was in 2018. What benefits it provided? Asking so that we can substitute it with some other tool.

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

The only remaining question, is what do we do about scrutinizer...

[core23]

The only remaining question, is what do we do about scrutinizer...

Is this an open topic for this PR? Maybe we should create a new issue (on dev-kit) to resolve this problem.

[VincentLanglet]

The only remaining question, is what do we do about scrutinizer...

Is this an open topic for this PR? Maybe we should create a new issue (on dev-kit) to resolve this problem.

Currently scrutinizer is working.
If we merge this PR, scrutinizer will fail for every new PR.

Either we drop scrutinizer, either we fix the build here.

[vv12131415]

I'm for deprecating the Scrutinizer because if you look at (as an example)
https://scrutinizer-ci.com/g/sonata-project/SonataAdminBundle/inspections/98c50ba5-823c-4f6d-a8a7-eec7cb78d35e/issues/files/src/Admin/AbstractAdmin.php?status=all&orderField=path&order=asc&fileId=src/Admin/AbstractAdmin.php&honorSelectedPaths=0

This is the list of the reasons for deprecating this service

• it can't read array shapes (see https://scrutinizer-ci.com/g/sonata-project/SonataAdminBundle/inspections/98c50ba5-823c-4f6d-a8a7-eec7cb78d35e/issues/files/src/Admin/AbstractAdmin.php?status=all&orderField=path&order=asc&fileId=src/Admin/AbstractAdmin.php&honorSelectedPaths=0&issueId=44063378)
• it recommends to use empty, and tags it with Best practice, which is (in my opinion) bad practice, since it's (empty) is not strictly typed thing (it's like using == instead of === in your code). I enforce to not write it in my code.
• it doesn't understand that trigger_error is used on purpose
• it says that The parameter %s is not used and could be removed which is not true since the class can be (and it's intended to) inherited, so it's just analyses one file and not the whole project (as PHPStan/Psalm does)
• there is no answer to my support email, which interpret it is deprecated my it's creators.
• the last blog post was in 2018, a lot changed from that time.

[jordisala1991]

Lets remove it then, It can be done without changing code so @rande or @greg0ire can you remove the integration for all the projects on the organization? we will need to PR on dev-kit then, to remove it from the READMEs

[jordisala1991]

Thank you @vladyslavstartsev

[greg0ire]

@vladyslavstartsev Thanks!

@jordisala1991 I'm not sure how to achieve that, so for now, I will just remove the Scrutinizer webhook from this bundle, and we'll see how things go. If it's enough I will do that for all repos, otherwise I suppose I will have to remove repositories from the Scrutinizer UI.