search

psalm / psalm-plugin-laravel

A Psalm plugin for Laravel
MIT License
306 stars 71 forks source link

Add support for Laravel 9 #213

Closed Nielsvanpach closed 2 years ago

Nielsvanpach commented 2 years ago

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

Current issues

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

zlodes commented 2 years ago

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

caugner commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@canvural sorry. I meant phpstan issues, e.g.: https://github.com/phpstan/phpstan/issues/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 commented 2 years ago

Any timeline on when Laravel 9 will be supported?

mr-feek commented 2 years ago

@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 commented 2 years ago

Status update

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

tm1000 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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.

xrobau commented 2 years ago

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? 🙃

Just FYI, @tm1000 was for a VERY long time the lead developer of FreePBX - the world's most popular open source PBX (also written in mainly PHP). He is well aware of how hard it is to try to 'Open Source' and make everyone happy. It is not easy, and it's also extremely easy to slightly mis-read something and end up with the totally wrong idea of what someone was trying to say.

I have no new suggestions on how to solve this common problem of, basically, people just losing interest in their projects! I just wanted to make sure that everyone knew that @tm1000 wasn't just blowing hot air, he actually knows what he's talking about (even if he was slightly off target!)

caugner commented 2 years ago

As @canvural has rightfully pointed out, I'm a contributor to this project, and I have never considered myself a maintainer of this project.. But then again, I do regret my message above, because it may have been discouraging to other fellow contributors (and the real maintainer, @mr-feek), and it obviously mislead others.

Apologies for my part, and now let's try to focus the discussion on adding support for Laravel 9, rather than talking about myself (or Larastan).

mr-feek commented 2 years ago

Always happy to add more maintainers after a proven track record of contributions. I try to merge every PR that has passing CI. I haven't worked with laravel too much for the last year or two due to a shift in focus at work, so I haven't had the personal drive to dedicate hours / week to this project for new features.

At this point in time, I think I'm ok with dropping laravel 6 active maintenance (minus bug fixes) if that helps us to support laravel 9

tm1000 commented 2 years ago

Are you ok with upgrading ide helper as well? I know the thought process was to not use ide helper and write your own but if you both don't have time then probably upgrading ide helper now is a good solution.

mr-feek commented 2 years ago

I think we can upgrade ide-helper as well and bump a minor version release. If someone wants to lead that work, I'll merge PRs

tm1000 commented 2 years ago

I missed it but upgrading ide helper is already part of this pr

So then this pr should be a new major release with laravel 9 support. Dropping php 7.3 and 7.4 and upgrading ide helper.

So as @Nielsvanpach said the only hold is codeception. Which is needed for testing (it's in require-dev).

Codeception has a beta release out of 5.0 right now. I wonder if this package could link to that (at least temporarily). Then we wouldn't need another pr for just ide helper.

Alternatively link to the codeception beta and then release a beta of this package

HDVinnie commented 2 years ago

@tm1000 This is FOSS....you cannot expect it to be maintained forever no matter how many ppl use it or depend on it. Truthfully, you should never depend on a package your not willing to maintain. They should considers time savers but should still be reviewed like they are a PR. Pulling them from a private fork also allows you to merge from upstream and review the code for several things. Look at all the NPM drama lately....your coming off as ungrateful and as if the maintainers of this repo own you something when they dont owe you a thing.

tm1000 commented 2 years ago

@HDVinnie Thanks for your comments. I think we've all moved past this. I already apologized (twice). I encourage you to reread the thread in its entirety. The discussion on that ended a while ago, calling me out directly and dredging this all back up again isn't going to help. We've already moved back to talking about this PR specifically (which is what Im going to keep doing past this)

HDVinnie commented 2 years ago

@HDVinnie Thanks for your comments. I think we've all moved past this. I already apologized. I encourage you to reread the thread in its entirety. The discussion on that ended a while ago.

Awhile ago lmao....just 6 hours ago you apologized. It doesn't matter if the guy was a maintainer or not. Never expect your owed updates on a FOSS package, thats my point. Anyway moving right along.....

tm1000 commented 2 years ago

FYI Codeception v5 is set to require PHPUnit 10. Is that going to be an issue? This is why many things in composer.json for Codeception are marked as master-dev

https://github.com/Codeception/Codeception/commit/f909c6d8523bab3fc1e97b2f23a1b0e428121e7b

mr-feek commented 2 years ago

@tm1000 I don't think phpunit should be an issue. Codeception just uses it under the hood to run our tests (we don't have a dependency on phpunit)

mzur commented 2 years ago

I tried to see what's blocking this PR (creating #221 and #223 in the process). So the main issue is Codeception v5 which introduces the following conflicts that were partly described above:

  1. weirdan/codeception-psalm-module requires Codeception v4. I tried to update it to 5.0.0-alpha2 but I couldn't resolve the conflict caused by codeception/module-filesystem:^2.0.

  2. vimeo/psalm requires sebastian/diff: ^3.0 || ^4.0 which conflicts with Codeception v5 (requiring dev-master).

So in summary, this is blocked until Codeception v5 is properly released and its version constraints finalized. Codeception v5 waits for the release of PHPUnit 10 (which is already a year late with no new release date I could find).

After that, weirdan/codeception-psalm-module and possibly vimeo/psalm need updates to support the new versions. Then this PR can proceed. Please correct me if I'm wrong.

tm1000 commented 2 years ago

@mzur Yes that is the conclusion I got as well. This depends on PHPUnit 10. Then it will depend on Psalm itself supporting PHPUnit 10.

jrmajor commented 2 years ago

vimeo/psalm requires sebastian/diff: ^3.0 || ^4.0 which conflicts with Codeception v5.

@mzur @Nielsvanpach Psalm works fine with PHPUnit 10 for me with "sebastian/diff": "5.0.x-dev as 4.0.4". Maybe the same trick would work there?

mzur commented 2 years ago

Seems like Codeception v5 will still support PHPUnit 9 for now. This should make things easier.

Nielsvanpach commented 2 years ago

Solved the dependency issues and added the sole class of weirdan/codeception-psalm-module to this repo for now (Module.php), to bypass the composer restrictions. But it seems like the package is not yet compatible with Codeception v5, tests are failing: https://github.com/psalm/psalm-plugin-laravel/actions/runs/2014834434

tm1000 commented 2 years ago

@Nielsvanpach I issued a PR against your laravel-9 branch that fixes the issues you noticed (see: https://github.com/Nielsvanpach/psalm-plugin-laravel/pull/1)

The two main issues:

Now the tests run, some of the tests do fail but now we can go through and fix each test.

Nielsvanpach commented 2 years ago

@Nielsvanpach I issued a PR against your laravel-9 branch that fixes the issues you noticed (see: Nielsvanpach#1)

Great thanks!

tm1000 commented 2 years ago

@Nielsvanpach Ok this PR (https://github.com/Nielsvanpach/psalm-plugin-laravel/pull/2) fixes the tests (except for 1 or 2....maybe?) and also fixes the merge conflict with master

tm1000 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Thanks @mzur !

mzur commented 2 years ago

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

tm1000 commented 2 years ago

@mzur thanks for getting it the final way there!

mr-feek commented 2 years ago

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 commented 2 years ago

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