Update Psalm and fix a few issues

[muglug]

This should also help PHPStan understand the collections class a little better, as it doesn't support array-key yet.

[greg0ire]

Can you please squash both of your commits into one and provide a little more detail about what specifically was fixed in your commit message ? 🙏

[muglug]

@greg0ire sure - without these changes, latest Psalm emits these issues on the codebase:

ERROR: InvalidArgument - lib/Doctrine/Common/Collections/AbstractLazyCollection.php:297:48 - Argument 1 of Doctrine\Common\Collections\Collection::offsetExists expects TKey:Doctrine\Common\Collections\AbstractLazyCollection as array-key|null, int|string provided
        return $this->collection->offsetExists($offset);


ERROR: InvalidArgument - lib/Doctrine/Common/Collections/AbstractLazyCollection.php:311:45 - Argument 1 of Doctrine\Common\Collections\Collection::offsetGet expects TKey:Doctrine\Common\Collections\AbstractLazyCollection as array-key|null, int|string provided
        return $this->collection->offsetGet($offset);


ERROR: InvalidArgument - lib/Doctrine/Common/Collections/AbstractLazyCollection.php:323:38 - Argument 1 of Doctrine\Common\Collections\Collection::offsetSet expects TKey:Doctrine\Common\Collections\AbstractLazyCollection as array-key|null, int|string provided
        $this->collection->offsetSet($offset, $value);


ERROR: InvalidArgument - lib/Doctrine/Common/Collections/AbstractLazyCollection.php:334:40 - Argument 1 of Doctrine\Common\Collections\Collection::offsetUnset expects TKey:Doctrine\Common\Collections\AbstractLazyCollection as array-key|null, int|string provided
        $this->collection->offsetUnset($offset);


ERROR: InvalidPropertyAssignmentValue - lib/Doctrine/Common/Collections/ArrayCollection.php:297:9 - $this->elements with declared type 'array<TKey:Doctrine\Common\Collections\ArrayCollection as array-key, T:Doctrine\Common\Collections\ArrayCollection as mixed>' cannot be assigned type 'non-empty-array<TKey:Doctrine\Common\Collections\ArrayCollection as array-key|int, T:Doctrine\Common\Collections\ArrayCollection as mixed>'
        $this->elements[] = $element;

I don't quite understand why int|string is not considered compatible with array-key|null, can somebody please explain? It implies some ints or strings can be invalid keys, but I don't know of such values.

[ostrolucky]

How about merging this into 1.6? These look like bugfixes and since these are not runtime changes, are safe to do as patch IMHO.

[greg0ire]

@ostrolucky done. Weirdly, I had to add more annotations to ArrayCollection for Psalm to pass…

[greg0ire]

Trying to get a report from Travis…

[ostrolucky]

Why this @greg0ire ?

[greg0ire]

Oh sorry I meant to use 7.1. The reason for this is being able to update vimeo/psalm without ending up with 7.4 dependencies, and a red build: https://travis-ci.org/github/doctrine/collections/builds/690553813

[greg0ire]

I will use 7.1.3 here, since that's what Psalm requires and it still is ok with what we use in Travis: (7.1.11)

[greg0ire]

To be more clear: my goal is that people needing to update our deps like in this PR can do so safely from machines that use php 9 or whatever.

[ostrolucky]

I thought config.platform is in effect only when it's root project, not as dependency. If you want to allow doctrine/collections to install on php 8, we should change php constraint

[greg0ire]

If you want to allow doctrine/collections to install on php 8, we should change php constraint

I don't want that. All I want is for a contributor (so like @muglug here) that uses another version of php than 7.1 (like me, for instance, who uses 7.4.6) to be able to generate a composer.lock that is guaranteed to work on 7.1, because that is the lowest php version we use on Travis. See failing build above for an example of failure that might happen.

[ostrolucky]

Oh my god, again this lockfile bs and working around it. We should seriously drop it.

[ostrolucky]

Actually, let's drop this config.platform change. That isn't required change for this PR, this change should be separated in different PR. There are more concerns about that from my side, for example we run all builds now with PHP 7.1 dependencies only.

[greg0ire]

There are more concerns about that from my side, for example we run all builds now with PHP 7.1 dependencies only.

Before this PR, we run all builds with whatever-version-of-php-the-last-persons-who-generated-the-composer.lock uses. How is that better?

Besides, I cannot drop the change and have a passing build, unless I find a way to use something else than 7.4. If you or @muglug are not using 7.4, you are welcome to give it a try, but IMO that's just waiting for the next contributor to stumble on the same issue.

[ostrolucky]

Before this PR, we run all builds with whatever-version-of-php-the-last-persons-who-generated-the-composer.lock uses. How is that better?

It isn't, but this is still separate concern from this PR. Goal of this PR is fix Psalm annotations, not improve our CI. I have no problem with merging unrelated changes, but only if we are both on board with them, without discrepancies. Here, we have different opinions about this change, so all discussion about it and the way how to fix it should happen in different PR. I think in some other doctrine projects we just delete composer.lock as part of CI job so lockfile is regenerated. @beberlei you are familiar with this.

I cannot drop the change and have a passing build.

How come? I though that's what's composer.lock for, so it downloads same dependencies in CI as in lockfile (7.1 ones). I thought your original issue with failing build is because your computer generated lock file for PHP 7.4 version, while CI runs with PHP 7.1.

[greg0ire]

Ah maybe you mean I can run composer update nothing? Should work, let me try.

[greg0ire]

Ok so the workflow is:

1. do change in composer.json, and add config block
2. composer update whatever-you-need
3. remove config block
4. composer update nothing

Not great but indeed, let's discuss this separately.

[ostrolucky]

Thanks for your patience, I know I can be annoying and pedantic.