Add support for Laravel 9 #213

Nielsvanpach · 2022-01-19T10:07:58Z

I don't think it' possible to add Laravel 9 support without dropping 7.3 and 7.4 support.

Current issues

Codeception v4 does not support symfony/finder v6 but they are working on a v5 release with support.
symfony/finder v6 is used in Laravel 9.
Codeception v5 has a conflicting package constraint for sebastian/diff: https://github.com/Codeception/Codeception/blob/5.0/composer.json#L37
https://github.com/vimeo/psalm/blob/4.18.x/composer.json#L36
weirdan/codeception-psalm-module does not support Codeception v5 yet. We could try to open a PR to that repo.

Hopefully if this is fixed, all the dependency issues are solved and we can work on proper Laravel 9 compatibility.

composer.json

caugner

Good initiative, but I would suggest only adding 9.x to the illuminate_version matrix for now.

Previously, the tests were testing the lowest and highest version of each Laravel major version, but I don't think the status quo of this PR still does that.

.github/workflows/test.yml

composer.json

.github/workflows/test.yml

caugner · 2022-02-08T19:32:58Z

.github/workflows/test.yml

-        illuminate_version: [6.*, 8.*]
-        composer_flags:  ['', '--prefer-lowest']
+        illuminate_version: [6.*, 7.*, 8.*, 9.*]
+        stability: [prefer-lowest, prefer-stable]


--prefer-stable is not the opposite of --prefer-lowest, it also makes sense in combination.

Actually, I think that --prefer-stable is even the default, but I'm not totally sure.

Indeed, both can be used in combination I think. prefer-stable will check for packages with minimum-stability set to stable. But I don't think it's the default (and if so, it's still explicit this way). It's just an extra check to see if this package is easily installable, feel free to improve if possible!

zlodes · 2022-02-09T21:32:50Z

I think we need to remove restrictions of barryvdh/laravel-ide-helper, because these versions don't require laravel/framework: ^9.0

caugner · 2022-02-10T16:37:04Z

I think we need to remove restrictions of barryvdh/laravel-ide-helper, because these versions don't require laravel/framework: ^9.0

Indeed, this seems to be the big blocker going forward.

As an alternative, #204 discusses supporting the latest laravel-ide-helper and mentions a fork which achieved that. However, that fork has not been updated since August 2021.

Due to limited resources, I will probably have to migrate at least one of my Laravel projects to larastan (maintained by Nuno who works for Laravel), because it requires Laravel 9 asap.

zlodes · 2022-02-11T11:34:01Z

@caugner Larastan now (^2.0) has several nasty bugs with generics. :|

We use phpstan and psalm together because it increases global code quality. Nowadays the functionality difference between psalm and phpstan is very huge, but it's not bad, v.v. it's cool that these tools are improving.

canvural · 2022-02-11T11:59:34Z

@caugner Larastan now (^2.0) has several nasty bugs with generics. :|

What are those nasty bugs? Maybe we can fix them if you open an issue 😊

zlodes · 2022-02-13T09:34:01Z

@canvural sorry. I meant phpstan issues, e.g.: phpstan/phpstan#6505

And if I update Larastan to ^2.0 something around generics will broken.
We use Psalm and PhpStan together and now it seems that we need to release psalm-plugin-laravel because of many changes in Laravel 9.0. And then it will be possible to use Larastan ^2.0 and Psalm.

HDVinnie · 2022-02-14T02:10:00Z

Any timeline on when Laravel 9 will be supported?

mr-feek · 2022-02-14T05:59:21Z

@HDVinnie if a PR is made with green CI, I'll happily merge! I don't have the time to figure it out myself right now

Nielsvanpach · 2022-02-14T23:09:29Z

Status update

Codeception v4 does not support symfony/finder v6 but they are working on a v5 release with support.
symfony/finder v6 is used in Laravel 9.
Codeception v5 has a conflicting package constraint for sebastian/diff: https://github.com/Codeception/Codeception/blob/5.0/composer.json#L37
https://github.com/vimeo/psalm/blob/4.18.x/composer.json#L36
weirdan/codeception-psalm-module does not support Codeception v5 yet. We could try to open a PR to that repo.

Hopefully if this is fixed, all the dependency issues are solved and we can work on proper Laravel 9 compatibility.

tm1000 · 2022-02-16T21:42:01Z

Due to limited resources, I will probably have to migrate at least one of my Laravel projects to larastan (maintained by Nuno who works for Laravel), because it requires Laravel 9 asap.

This is really unfortunate. You are a maintainer of this project and yet you are essentially abandoning it and moving to phpstan because you don't have the time to maintain it.

I'm not trying to start a fight here but it was rather shocking to read that. To not have time to maintain this is one thing but to just abandon even trying to fix it and move to another product is a whole other thing and compromises the ability for people to use Psalm in Laravel which ultimately hurts Psalm in the end.

The fork (https://github.com/brokeyourbike/psalm-plugin-laravel) itself exists because #204 is still not solved (and for some reason the fork shows up on the Psalm site as a supported plugin which is further confusing and fragmenting to the community as a whole). #204 should have been solved a while ago. Maintain two separate versions. The fork exists because the fork dropped support for Laravel 6. This project could do the same by just maintaining two separate versions. I mean heck there hasn't even been a release of this in the last 8 months so it really doesn't effect anyone to have two versions of this.

The longer this process drags out (Not supporting the new IDE helper/writing your own) and not supporting Laravel 9 the more this plugin prevents people from using Psalm with Laravel. Laravel already supports PHPStan because a core developer of Laravel wrote the plugin himself.

To me the outcome is obvious. If you want to use Laravel 9 then do not use Psalm, use phpstan. Or wait until someone does another fork that supports Laravel 9...then 10 and so forth.

@HDVinnie if a PR is made with green CI, I'll happily merge! I don't have the time to figure it out myself right now

I'm sorry but the master branch doesn't even pass these requirements! See: https://github.com/psalm/psalm-plugin-laravel/runs/5188633439?check_suite_focus=true

However, that fork has not been updated since August 2021.

This project hasn't had a release since July 2021. Why does this matter?

Open Source is hard. Especially when you aren't getting paid. But watching the outcome of #204 thus far (which was essentially "we need to write our own and not use ide-helper but we don't have time") and that going no-where worries me for when this will support Laravel 9 (if ever)

Nielsvanpach · 2022-02-17T08:32:49Z

So basically you are disappointed that someone no longer has (unpaid) time to support an open source project you rely on and you feel the need to let that person know that it shocks you? 🙃

tm1000 · 2022-02-17T14:56:48Z

No. It's shocking that one of the maintainers said he has no time to work on this so he's going to use another project instead. That should not have been said. Anyone who comes here for help will see that and think to themselves why use this"

If the maintainer no longer has time then they should say "we need more help". Instead it's "we don't have time. patches welcome if you get CI to pass!" Do you not see an issue with that? Both maintainers said they have no time to maintain the project they are maintaining.

CI literally isn't even passing on the master branch and it hasn't for a while. The fact that a maintainer said patches welcome if you get CI to be green is strange because it's not even green right now

There have been several proposed fixes to support new ide helper and all have been rejected because of the continued need to support laravel 6.0 even though one could just do a sem ver release of a new major version.

The fork that exists is also an issue because it violates packagist.org guidelines which state that one should not publish packages that are forks. Yet it exists in packagist.org and additionally it's listed on the psalm site.

The reasoning the fork exists in the first place is because this project can't find a solution for a long standing issue in regards to ide helper. That's a big big issue in itself. That guy went off and forked it because he say that a solution was unattainable in this project. This will keep happening I'm sure. Especially with laravel 9. I'm trying to bring awareness that saying "I'm moving to phpstan" will just cause more of these forks.

I work on the language server engine for psalm and also I'm the maintainer for the vscode plug-in for psalm. So I do my fair share of unpaid open source development. I'm just confused by responses like "I'm moving to phpstan. Patches welcome" from a maintainer.

I wouldn't have even commented on this if the comment about "I'm switching to phpstan" from a maintainer wasn't said.

canvural · 2022-02-17T15:00:31Z

I don't think @caugner is a maintainer of this project. He just contributes a lot. So it's totally fine if he says he will not work on this.

tm1000 · 2022-02-17T15:12:31Z

@canvural and then yes. I am totally wrong. I accept that and I apologize. The appearance from issues and PRs is that he was a maintainer not just a contributor.

However maybe this project needs more than one maintainer if the solution from a very active contributor is "I'm moving to phpstan"

I think I've stirred the pot a bit much here and I'm sorry for that. Thanks for your work here @Nielsvanpach and for the packages @spatie provides.

tm1000 · 2022-04-07T20:04:00Z

@Nielsvanpach Ok this PR (Nielsvanpach#2) fixes the tests (except for 1 or 2....maybe?) and also fixes the merge conflict with master

Laravel 9 Test fixes

tm1000 · 2022-04-07T21:29:02Z

Ok so this should be a little easier for someone to fix (if not me).

The failing tests are due to php 8.1 incpompatibilies when using prefer-lowest because the issues were made in as a non-breaking change in the same release.

Additionally in Laravel 9 more specific models are returned rather than just Model itself so the tests need to be adjusted to reflect that for Laravel 9 as opposed to 8

mzur

Is there any specific reason why the tests for firstOrCreate(), firstOrNew() and factory() were removed? These can still be used in Laravel 9.

Edit: Now I see that the tests no longer make sense because of the removed Laravel versions.

mzur · 2022-04-26T13:27:02Z

I created a PR to this branch that should fix the remaining failing tests. Does anyone have a better idea to fix the PHP 8.1 issues than to require-dev working versions of the incompatible packages?

tm1000 · 2022-04-26T17:45:25Z

Thanks @mzur !

Fix failing tests for firstOrCreate, firstOrNew and PHP 8.1

mzur · 2022-04-27T06:47:43Z

All checks are green now so this may be ready to merge?

tm1000 · 2022-04-27T06:51:05Z

@mzur thanks for getting it the final way there!

mr-feek · 2022-04-27T15:20:44Z

tests/Support/Module.php

+use RuntimeException;
+
+/**
+ * Duplicate of https://github.com/psalm/codeception-psalm-module


Hey could you help me understand why we need this duplicated? Do we need some changes merged upstream to codeception-psalm-module? @weirdan has been great at merging prs fast! Let me know

It's duplicated because of the changes to codeception. Doing the PR against https://github.com/psalm/codeception-psalm-module would require a newer version of codeception which doesnt support PHP 7.x.

See: https://www.diffchecker.com/kbix1BTd

Specifically params that were strings are now PyStringNode object and additionally, exit codes are now integers instead of strings

I just went through https://github.com/psalm/codeception-psalm-module and unfortunately even checking it out and running the tests there without doing any code modifications there are 13 unit test failures.

The issue is with the deprecated usage of totallyTyped and yes usually Weirdan is quick on PR changes but @mzur already did a PR for this back in February which is still not merged. psalm/codeception-psalm-module#39

mr-feek · 2022-04-27T15:23:01Z

.github/workflows/test.yml

@@ -8,40 +8,11 @@ jobs:
    strategy:
      fail-fast: false
      matrix:
-        php: [7.3, 7.4, 8.0, 8.1]
-        illuminate_version: [6.*, 8.*]


It would be great if we could support all versions in the same major release (1.x) -- Could you help me understand what might be a breaking change not allowing us to support these still? (I want to continue supporting laravel 6 til EOL, which is in september).

I don't think it' possible to add Laravel 9 support without dropping 7.3 and 7.4 support.

Laravel 6 won't work well on php 8. It's certainly not supported. That's why I believe laravel 6 support was dropped.

Is there a major issue with just issuing a 2.0 release for this PR? I don't understand the hesitation at doing a split release as has been already mentioned in this PR previously. See: #213 (comment)

Larastan (Laravel plugin for PHPStan) also released a new major version for Laravel 9.

Main reason we have a major version increase is we dropped support for Laravel versions older than 9. Because now all the Collection stubs Larastan had are moved to the Laravel core!

My hesitation is just more maintenance overhead -- I would think we could solve that by adjusting the test matrix to not run laravel 6 on php 8 (like we are already doing with other tests) .

It's not a blocker, would just like to understand what exactly is causing the need for multiple versions

mr-feek · 2022-04-27T16:26:01Z

Thanks for the hard work everyone! Give me a day or two to figure out how best to merge + maintain this, but expect it to be merged + released soon :)

mr-feek · 2022-04-27T16:29:21Z

composer.json

-        "slevomat/coding-standard": "^6.2"
+        "phpoption/phpoption": "^1.8.0",
+        "symfony/http-foundation": "^5.3.7 || ^6.0",
+        "ramsey/collection": "^1.2.0"


curious what we need this for? I don't see it referenced in the code

I know that if ramsey/collection is less than 1.2.0 it blows up on PHP 8.1 tests because Return Type will Change errors.

These were all the packages that didn't work with PHP 8.1 at prefer-lowest. I didn't know any better than to require the versions compatible with PHP 8.1 to fix the failing tests. The main issue is the breaking change of return type checks in the minor release of PHP and all these packages claiming to work with PHP ^8.0 although they are not.

mr-feek · 2022-04-27T16:29:49Z

composer.json

@@ -46,6 +50,7 @@
    },
    "scripts": {
        "analyze": "psalm",
+        "analyse": "psalm",


this looks accidental

Fix accidental script declaration in composer.json

mr-feek · 2022-05-02T17:40:08Z

Thanks for the PR! cutting a new major release, 2.0

Nielsvanpach changed the title ~~Laravel 9~~ Add support for Laravel 9 Jan 19, 2022

mr-feek reviewed Jan 19, 2022

View reviewed changes

composer.json Show resolved Hide resolved

caugner suggested changes Feb 8, 2022

View reviewed changes

Nielsvanpach added 3 commits February 14, 2022 22:37

run test suite with Laravel 9

b8913dc

update packages in laravel test script

538e1cb

use --with on composer

6096cf0

Nielsvanpach force-pushed the laravel-9 branch from 2f8390a to 6096cf0 Compare February 14, 2022 21:37

Nielsvanpach added 9 commits February 14, 2022 22:39

allow illuminate v8

c7a4457

require instead of update

8bf5d3f

fix malformed yaml

7ba59e0

ignore unsupported matches

33a46af

define laravel/framework version

95c9c06

require a higher version of barryvdh/laravel-ide-helper

5fb7f5a

drop support for 7.3, 7.4 and Illuminate v6

08e9b38

add typehinting according to the contract

74347de

wip: require v5 of codeception

3465c35

mzur mentioned this pull request Feb 16, 2022

Upgrade to Laravel 9 biigle/core#421

Merged

20 tasks

tm1000 and others added 3 commits April 7, 2022 20:16

bump up illuminate for 8.1

1c9e193

fix version string

48b4fd7

Merge pull request #2 from tm1000/laravel-9-test-fixes

e50f7f0

Laravel 9 Test fixes

mzur reviewed Apr 26, 2022

View reviewed changes

mzur added 5 commits April 26, 2022 13:39

Fix failing tests for firstOrCreate and firstOrNew in Laravel 9

48d1c56

Fix failing tests for Eloquent relation types in Laravel 9

fad8f22

Add require-dev for phpoption/phpoption version supporting PHP 8.1

906d80f

Add require-dev for symfony/http-foundation version supporting PHP 8.1

de5f936

Add require-dev for ramsey/collection version supporting PHP 8.1

5fba336

Merge pull request #3 from mzur/laravel-9-patch-1

07f342d

Fix failing tests for firstOrCreate, firstOrNew and PHP 8.1

Nielsvanpach marked this pull request as ready for review April 27, 2022 06:52

tm1000 approved these changes Apr 27, 2022

View reviewed changes

mr-feek reviewed Apr 27, 2022

View reviewed changes

Fix accidental script declaration in composer.json

6c3c9b5

mzur mentioned this pull request Apr 27, 2022

Fix accidental script declaration in composer.json Nielsvanpach/psalm-plugin-laravel#4

Merged

Merge pull request #4 from mzur/laravel-9-patch-2

76a53f5

Fix accidental script declaration in composer.json

mr-feek merged commit 7524304 into psalm:master May 2, 2022

mr-feek added the enhancement New feature or request label May 2, 2022

Nielsvanpach deleted the laravel-9 branch May 3, 2022 07:45

Add support for Laravel 9 #213

Add support for Laravel 9 #213

Conversation

Nielsvanpach commented Jan 19, 2022 • edited Loading

caugner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zlodes commented Feb 9, 2022

caugner commented Feb 10, 2022

zlodes commented Feb 11, 2022

canvural commented Feb 11, 2022

zlodes commented Feb 13, 2022

HDVinnie commented Feb 14, 2022

mr-feek commented Feb 14, 2022

Nielsvanpach commented Feb 14, 2022 • edited Loading

tm1000 commented Feb 16, 2022 • edited Loading

Nielsvanpach commented Feb 17, 2022 • edited Loading

tm1000 commented Feb 17, 2022 • edited Loading

canvural commented Feb 17, 2022

tm1000 commented Feb 17, 2022 • edited Loading

tm1000 commented Apr 7, 2022

tm1000 commented Apr 7, 2022

mzur left a comment • edited Loading

Choose a reason for hiding this comment

mzur commented Apr 26, 2022

tm1000 commented Apr 26, 2022

mzur commented Apr 27, 2022

tm1000 commented Apr 27, 2022

Choose a reason for hiding this comment

tm1000 Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

tm1000 Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tm1000 Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-feek Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

mr-feek commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzur Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-feek commented May 2, 2022

Nielsvanpach commented Jan 19, 2022 •

edited

Loading

Nielsvanpach commented Feb 14, 2022 •

edited

Loading

tm1000 commented Feb 16, 2022 •

edited

Loading

Nielsvanpach commented Feb 17, 2022 •

edited

Loading

tm1000 commented Feb 17, 2022 •

edited

Loading

tm1000 commented Feb 17, 2022 •

edited

Loading

mzur left a comment •

edited

Loading

tm1000 Apr 27, 2022 •

edited

Loading

tm1000 Apr 27, 2022 •

edited

Loading

tm1000 Apr 27, 2022 •

edited

Loading

mr-feek Apr 27, 2022 •

edited

Loading

mzur Apr 27, 2022 •

edited

Loading