Fix issues detected by psalm on 2.7 #7989

orklah · 2020-01-16T19:50:44Z

This PR is basically #7986 but targetted on 2.8 as suggested.

I'll copy the text from there:

Hi,

This PR is the result of fixing easy issues from the two first levels of Psalm.

This is the starting point of trying to begin #7762. If this goes smoothly, my goal is to continue fixing errors by slowly increasing the Psalm's levels. Eventually, the idea would be to add Psalm to the CI and to add template annotation to interfaces and exposed classes to help tools like phpstan and psalm understand Doctrine without the need of stubs or plugins.

I'm always trying to learn so don't hesitate to ask for changes or anything!

Thanks!

I didn't expect such difference between master and 2.8! Great job guys, I can't wait for future versions!

It seems that the CI is broken on 2.8. I tried to run phpcbf locally but there are massive changes, I didn't dare apply them here, so there may be some CS errors. If the CI is fixed and the errors are still there in the future, I'll correct them.

I'll close #7986, some changes there were specific to the master branch due to code movements but let's fix that later.

greg0ire · 2020-01-16T23:17:18Z

Restarting the build

lib/Doctrine/ORM/Configuration.php

orklah · 2020-01-17T16:58:41Z

Issues in Scrutinizer are correct but were not introduced by this PR. Can this be merged as is?

lib/Doctrine/ORM/Configuration.php

greg0ire · 2020-01-17T18:18:09Z

Yeah don't worry about Scrutinizer

SenseException

Scrutinizer needs a restart sometimes and it gets back to be green again.

orklah · 2020-01-25T13:12:52Z

Thanks for approving. What's the next step on this? Should we wait for more approval or is there something to do?

Thanks

greg0ire · 2020-01-25T14:52:06Z

More approvals would be great indeed, especially from people who are familiar with the annotations (@Ocramius maybe?) @SenseException I don't know how familiar you are with psalm annotations and how we use them inside Doctrine, feel free to merge.

Meanwhile, please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

git rebase -i origin/3.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
A window will show up with many lines, replace pick with fixup on every line but the first one
Close your editor, git should do its magic, and you should end up with one commit
Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

SenseException · 2020-01-27T22:38:34Z

Not familiar enough to merge it without hesitation.

greg0ire · 2020-05-17T20:35:36Z

I've become more familiar with Psalm now.. I think this could be merged, but should be merged on 2.7, since it is just code comments, and in order to keep differences between branches minimal.

orklah · 2020-05-17T21:24:07Z

@greg0ire great! I don't really know how to switch branches though, do you also have an explanation like the one for rebase? (it was extremely useful btw, I learned to rebase thanks to that explanation and it made me understand a lot about git)

greg0ire · 2020-05-17T21:47:34Z

Start by clicking the edit button and changing the target branch to 2.7. You should see a lot of commit appear. What you would want to rebase is only the commit you added on top of 2.8.x, so that would be git rebase --onto origin/2.7 2.8.x psalm-types-2.8. After you can force push and things should be back to normal.

Another way to do the same thing would be this:

Fetch all new commits: git fetch --all. ⬇️
Rebase on what you fetched interactively: ✂️
1. git rebase -i origin/2.7, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin repository.
2. A window will show up with many lines, remove every line but the last one, which should correspond to your commit.
3. If you run into a conflict: 💥
  1. fix it with git mergetool. 😷
  2. continue on your merry way with git rebase --continue. ⏩
Force push to overwrite all this : git push --force. ⬆️

orklah · 2020-05-18T16:31:40Z

Mmh, that went pretty smoothly, it's suspicious!

Fetching origin
Fetching upstream
remote: Enumerating objects: 547, done.
remote: Counting objects: 100% (547/547), done.
remote: Compressing objects: 100% (31/31), done.
remote: Total 834 (delta 511), reused 532 (delta 510), pack-reused 287
Receiving objects: 100% (834/834), 438.74 KiB | 1.17 MiB/s, done.
Resolving deltas: 100% (596/596), completed with 291 local objects.
From https://github.com/doctrine/orm
   fdad48278..6780a963f  2.7             -> upstream/2.7
   a236a83fa..850d57e79  2.8.x           -> upstream/2.8.x
   e77091399..9a8e1fb47  master          -> upstream/master
 * [new branch]          new-embeddables -> upstream/new-embeddables
 * [new tag]             v2.7.1          -> v2.7.1
 * [new tag]             v2.7.2          -> v2.7.2

C:\Users\User\PhpstormProjects\orm>git rebase -i upstream/2.7
error: cannot rebase: You have unstaged changes.
error: Please commit or stash them.

C:\Users\User\PhpstormProjects\orm>git rebase -i upstream/2.7
Successfully rebased and updated refs/heads/psalm-types-2.8.

C:\Users\User\PhpstormProjects\orm>git push --force
Enumerating objects: 34, done.
Counting objects: 100% (34/34), done.
Delta compression using up to 8 threads
Compressing objects: 100% (16/16), done.
Writing objects: 100% (18/18), 2.15 KiB | 2.15 MiB/s, done.
Total 18 (delta 15), reused 0 (delta 0)
remote: Resolving deltas: 100% (15/15), completed with 14 local objects.
To https://github.com/orklah/orm.git
 + 38eb578b1...6e6c28986 psalm-types-2.8 -> psalm-types-2.8 (forced update)

I wanted to try the first solution but I messed up and used the second one without noticing. but it finished successfully but I still see orklah wants to merge 1 commit into doctrine:2.8.x from orklah:psalm-types-2.8 on the github PR

So I'm not sure if everything went well. Can you check if it's okay please?

I noticed a new error on psalm, it is indeed an issue in code (we try to instantiate a potential null string). I'd prefer to avoid touching code unless I'm more comfortable with the repo. If it's okay with you, I'll just add it to the baseline or add an issueHandler in psalm.xml

greg0ire · 2020-05-18T16:57:15Z

but I still see orklah wants to merge 1 commit into doctrine:2.8.x from orklah:psalm-types-2.8 on the github PR

That's expected, isn't it? That's what I expected at least.

greg0ire · 2020-05-18T16:58:18Z

I'd prefer to avoid touching code unless I'm more comfortable with the repo. If it's okay with you, I'll just add it to the baseline or add an issueHandler in psalm.xml

No please don't, I will look into it for you.

orklah · 2020-05-18T17:04:15Z

That's expected, isn't it? That's what I expected at least.

Well, wasn't the whole point to switch from 2.8 to 2.7? The fact that it still displays 2.8 anywhere that isn't the name of my branche seems disturbing.

No please don't, I will look into it for you.

Great, thanks!

greg0ire · 2020-05-18T17:09:22Z

Well, wasn't the whole point to switch from 2.8 to 2.7? The fact that it still displays 2.8 anywhere that isn't the name of my branche seems disturbing.

Oh right you did not switch the head branch with the edit button

greg0ire · 2020-05-18T17:11:05Z

Can you please allow pushes from maintainers? Should be a checkbox, bottom right

orklah · 2020-05-18T17:13:10Z

Thanks for the change, I didn't know you had to change it there too. (EDIT: oh. That's actually the first thing you posted on your explanation. I have no excuse :( )

The checkbox was already checked. I unchecked it and then rechecked just to be sure.

greg0ire · 2020-05-18T17:21:14Z

Well, for some reason I have this:

gpf orklah HEAD:psalm-types --no-verify
Enumerating objects: 36, done.
Counting objects: 100% (36/36), done.
Delta compression using up to 4 threads
Compressing objects: 100% (17/17), done.
Writing objects: 100% (19/19), 2.94 KiB | 2.94 MiB/s, done.
Total 19 (delta 16), reused 0 (delta 0)
remote: Resolving deltas: 100% (16/16), completed with 15 local objects.
To github.com:orklah/orm.git
 ! [remote rejected]     HEAD -> psalm-types (permission denied)
error: failed to push some refs to 'git@github.com:orklah/orm.git'

Not sure if related to this: https://twitter.com/nicolasgrekas/status/1258709224734982146

greg0ire · 2020-05-18T17:21:40Z

Here are my changes BTW:

───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: lib/Doctrine/ORM/Query/FilterCollection.php
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ lib/Doctrine/ORM/Query/FilterCollection.php:23 @
namespace Doctrine\ORM\Query;

use Doctrine\ORM\EntityManagerInterface;
use function assert;

/**
 * Collection class for all the query filters.
@ lib/Doctrine/ORM/Query/FilterCollection.php:114 @ class FilterCollection
        if ( ! $this->isEnabled($name)) {
            $filterClass = $this->config->getFilterClassName($name);

            assert($filterClass !== null);
 
            $this->enabledFilters[$name] = new $filterClass($this->em);

            // Keep the enabled filters sorted for the hash

greg0ire · 2020-05-18T17:26:06Z

You forgot the use statement. Please squash all of your commits together, we should try not to have commits that fail the build.

orklah · 2020-05-18T17:36:30Z

I'm really bad with CS rules :D I should run them locally! It should be ok now

greg0ire · 2020-05-18T17:53:21Z

Trying to get a report from Travis, without success it seems :/

orklah · 2020-05-18T18:15:37Z

yeah, don't know what's up with that...

greg0ire · 2020-05-18T18:16:05Z

I have the same issue with this other PR: doctrine/cache#341 , but their status seems ok: https://www.traviscistatus.com/

greg0ire · 2020-05-18T19:05:16Z

Seems to work better now :)

greg0ire · 2020-05-18T19:17:58Z

lib/Doctrine/ORM/Query/SqlWalker.php

@@ -142,6 +142,8 @@ class SqlWalker implements TreeWalker
     * Map of all components/classes that appear in the DQL query.
     *
     * @var array
+     *
+     * @psalm-var array<string, array{metadata: ClassMetadata, mixed}>


I was going to merge, then saw this… Isn't that equivalent to

Suggested change

* @psalm-var array<string, array{metadata: ClassMetadata, mixed}>

* @psalm-var array<string, array{metadata: ClassMetadata}>

Or is there something I am missing?

to be honest, I don't remember that part. I didn't use psalter and I don't see why I would have added that manually. It was probably a psalm suggestion, even if that doesn't make a lot of sense. I'll revert that

greg0ire · 2020-05-18T19:18:09Z

lib/Doctrine/ORM/Query/SqlWalker.php

@@ -232,6 +234,8 @@ public function getEntityManager()
     * @param string $dqlAlias The DQL alias.
     *
     * @return array
+     *
+     * @psalm-return array{metadata: ClassMetadata, mixed}


greg0ire · 2020-05-18T19:48:38Z

Thanks a lot @orklah

orklah mentioned this pull request Jan 16, 2020

Fix issues detected by psalm #7986

Closed

greg0ire closed this Jan 16, 2020

greg0ire reopened this Jan 16, 2020

greg0ire reviewed Jan 16, 2020

View reviewed changes

lib/Doctrine/ORM/Configuration.php Outdated Show resolved Hide resolved

greg0ire reviewed Jan 17, 2020

View reviewed changes

lib/Doctrine/ORM/Configuration.php Show resolved Hide resolved

greg0ire previously approved these changes Jan 17, 2020

View reviewed changes

SenseException previously approved these changes Jan 21, 2020

View reviewed changes

orklah force-pushed the psalm-types-2.8 branch from 5de4de2 to 38eb578 Compare January 25, 2020 15:07

greg0ire requested a review from a team January 28, 2020 08:01

orklah dismissed stale reviews from SenseException and greg0ire via 6e6c289 May 18, 2020 16:20

orklah force-pushed the psalm-types-2.8 branch from 38eb578 to 6e6c289 Compare May 18, 2020 16:20

greg0ire changed the base branch from 2.8.x to 2.7 May 18, 2020 17:09

orklah added a commit to orklah/orm that referenced this pull request May 18, 2020

Fix Psalm error doctrine#7989 (comment)

c0ed66e

orklah force-pushed the psalm-types-2.8 branch 3 times, most recently from 268c90f to e9b5727 Compare May 18, 2020 17:34

greg0ire previously approved these changes May 18, 2020

View reviewed changes

greg0ire changed the title ~~Fix issues detected by psalm on 2.8~~ Fix issues detected by psalm on 2.7 May 18, 2020

greg0ire closed this May 18, 2020

greg0ire reopened this May 18, 2020

greg0ire closed this May 18, 2020

greg0ire reopened this May 18, 2020

greg0ire reviewed May 18, 2020

View reviewed changes

orklah dismissed greg0ire’s stale review via b892d8c May 18, 2020 19:29

add psalm types

d409d30

orklah force-pushed the psalm-types-2.8 branch from b892d8c to d409d30 Compare May 18, 2020 19:29

greg0ire approved these changes May 18, 2020

View reviewed changes

greg0ire merged commit de2e2a1 into doctrine:2.7 May 18, 2020

greg0ire self-assigned this May 18, 2020

greg0ire added this to the 2.7.3 milestone May 18, 2020

orklah mentioned this pull request Jul 5, 2020

add/fix more types (checked by psalm) #8199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues detected by psalm on 2.7 #7989

Fix issues detected by psalm on 2.7 #7989

orklah commented Jan 16, 2020

greg0ire commented Jan 16, 2020

orklah commented Jan 17, 2020

greg0ire commented Jan 17, 2020

SenseException left a comment

orklah commented Jan 25, 2020

greg0ire commented Jan 25, 2020

SenseException commented Jan 27, 2020

greg0ire commented May 17, 2020 •

edited

orklah commented May 17, 2020

greg0ire commented May 17, 2020

orklah commented May 18, 2020 •

edited

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020 •

edited

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020

greg0ire commented May 18, 2020 •

edited

greg0ire commented May 18, 2020

greg0ire May 18, 2020

orklah May 18, 2020

greg0ire May 18, 2020

greg0ire commented May 18, 2020

	* @psalm-var array<string, array{metadata: ClassMetadata, mixed}>
	* @psalm-var array<string, array{metadata: ClassMetadata}>

Fix issues detected by psalm on 2.7 #7989

Fix issues detected by psalm on 2.7 #7989

Conversation

orklah commented Jan 16, 2020

greg0ire commented Jan 16, 2020

orklah commented Jan 17, 2020

greg0ire commented Jan 17, 2020

SenseException left a comment

Choose a reason for hiding this comment

orklah commented Jan 25, 2020

greg0ire commented Jan 25, 2020

SenseException commented Jan 27, 2020

greg0ire commented May 17, 2020 • edited

orklah commented May 17, 2020

greg0ire commented May 17, 2020

orklah commented May 18, 2020 • edited

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020 • edited

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020

greg0ire commented May 18, 2020

orklah commented May 18, 2020

greg0ire commented May 18, 2020 • edited

greg0ire commented May 18, 2020

greg0ire May 18, 2020

Choose a reason for hiding this comment

orklah May 18, 2020

Choose a reason for hiding this comment

greg0ire May 18, 2020

Choose a reason for hiding this comment

greg0ire commented May 18, 2020

greg0ire commented May 17, 2020 •

edited

orklah commented May 18, 2020 •

edited

orklah commented May 18, 2020 •

edited

greg0ire commented May 18, 2020 •

edited