Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatible with Generics Eloquent Builder in Laravel 11.15 #1591

Merged
merged 5 commits into from
Oct 17, 2024

Conversation

KentarouTakeda
Copy link
Contributor

@KentarouTakeda KentarouTakeda commented Oct 9, 2024

Summary

Resolves #1572

When using Laravel IDE Helper with Laravel 11.15 or higher, the return type of model retrieval methods such as $builder->get() and $builder->find() becomes Model, which is more inconvenient than not using IDE Helper.

This pull request will fix that.

Laravel 11.15 introduced breaking changes to generic types in DocBlocks. Therefore, in order to fix this, we need to pin the Laravel version that IDE Helper supports to 11.15 or higher. (There are ways to avoid this, but I don't recommend it because there are non-negligible tradeoffs.)

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
    • Drop support for Laravel versions earlier than 11.15.
  • This change requires a documentation update

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md

Detail

The reported issue was the lack of type information for model retrieval methods such as find($id), but the cause was the type of the Eloquent Builder that returned it.

Laravel 11.15 has the following improvements:

  • Eloquent Builder accepts generic type parameters
  • Model return methods define their generic type as the return type

However, our tool defines a plain Builder with no type parameters as the return type for Model::query() and magic where methods. Intellephense and PHPStan read this information.

As a result, most model retrieval methods were missing return type information. This can be resolved by explicitly specifying a concrete model as the type parameter, which is always static.

So the fix boils down to the following code:

- * @method static Builder query()
+ * @method static Builder<static> query()

@KentarouTakeda KentarouTakeda changed the title Builder generics support Compatible with Generics Eloquent Builder in Laravel 11.15 Oct 9, 2024
@pataar
Copy link
Contributor

pataar commented Oct 10, 2024

What about adding the self return type in the ->where* cases and adding a @mixin Builder<self> to the docs? This also makes the static::query statement obsolete

@KentarouTakeda
Copy link
Contributor Author

That approach has the advantage of being shorter, but is likely to confuse static analyzers and cause other problems.

The return type of where() and where*() is Builder, so it would be safer to comply with that.

return-type-of-builder

@KentarouTakeda KentarouTakeda force-pushed the builder-generics-support branch from 0a0bd35 to 2bbdfae Compare October 10, 2024 10:30
@KentarouTakeda KentarouTakeda marked this pull request as ready for review October 10, 2024 10:59
@alexandre-tobia
Copy link

Good job @KentarouTakeda ! Would be awesome if this get merged.
@barryvdh

@barryvdh
Copy link
Owner

I think this is probably good. But probably we can just keep the 3.x version, as composer will make sure you can't install the older version.

There is a failing test:
image

@KentarouTakeda KentarouTakeda force-pushed the builder-generics-support branch from 2bbdfae to bea576f Compare October 15, 2024 06:56
@KentarouTakeda
Copy link
Contributor Author

KentarouTakeda commented Oct 15, 2024

Thank you for your opinion.

I deleted the 4.0 entry from the README and restored it to its original state.

Test failure is a separate issue. Fixed in #1593.

@KentarouTakeda
Copy link
Contributor Author

#1593 was closed because there was a mistake in the way it was modified.

@KentarouTakeda
Copy link
Contributor Author

@barryvdh
The test failure you mentioned seems to be caused by barryvdh/ReflectionDocBlock, not this repository.

It can't parse complex generics or closures. The cause is probably this:
https://github.com/barryvdh/ReflectionDocBlock/blob/e6811e927f0ecc37cc4deaa6627033150343e597/src/Barryvdh/Reflection/DocBlock/Tag/ParamTag.php#L49-L62

I'm not sure how to fix it, so could you please look at the fix in this pull request first?

@KentarouTakeda
Copy link
Contributor Author

To properly fix the failing tests, We needed to fix not only this repository but also barryvdh/ReflectionDocBlock.I've created a fix for:

Could you please review this as well?

@barryvdh barryvdh merged commit 085ca3e into barryvdh:master Oct 17, 2024
9 of 11 checks passed
@KentarouTakeda KentarouTakeda deleted the builder-generics-support branch October 17, 2024 22:58
@KentarouTakeda
Copy link
Contributor Author

Thank you very much for merging and publishing the RC version.

For testing:

$ composer require --dev -w barryvdh/laravel-ide-helper:v3.2.0-rc1

I used the RC version in two real world projects and confirmed that:

  • SomeModel::query()->find() returns SomeModel
  • There is no change in the results of Larastan, which was previously error-free.

It seems to work fine for me.

@bbprojectnet
Copy link

Plausible error/conflict with "smart-reset" option (php artisan ide-helper:models --write --smart-reset):
generics eloquent builder annotations are added over and over.

@KentarouTakeda
Copy link
Contributor Author

Thank you for the report. I reproduced it in my environment as well. I'll look into it.

@tvbeek
Copy link

tvbeek commented Oct 28, 2024

@KentarouTakeda @barryvdh I have a small question, was the drop of PHP 8.1 intended? The question is because I didn't read about it in the changelog 😄

@KentarouTakeda
Copy link
Contributor Author

@tvbeek
Yes, due to this feature, php-8.1 is no longer supported.

Since Eloquent Builder's generics support was added in Laravel 11.15, the IDE Helper also requires that version of Laravel be at least that version. Laravel 11 requires php-8.2 or higher.

@barryvdh
Copy link
Owner

Yes, the current version will keep working for older versions though

@barryvdh
Copy link
Owner

barryvdh commented Oct 28, 2024

Hmm I always thought that Composer would handle this for me, but it seems it doesn't

@barryvdh
Copy link
Owner

Oh wait it does:

~/Sites/laravel10/ composer require --dev barryvdh/laravel-ide-helper
Cannot use barryvdh/laravel-ide-helper's latest version v3.2.0 as it requires php ^8.2 which is not satisfied by your platform.
./composer.json has been updated
Running composer update barryvdh/laravel-ide-helper
Loading composer repositories with package information
Updating dependencies
Lock file operations: 8 installs, 0 updates, 0 removals
  - Locking barryvdh/laravel-ide-helper (v3.1.0)
  - Locking barryvdh/reflection-docblock (v2.1.3)

Which seems good enought, right? Laravel 11 is PHP8.2+ anyways, but 3.1.x keeps working on Laravel 10 and below and is possible to receive bugfixes if really required.

@tvbeek
Copy link

tvbeek commented Oct 28, 2024

@tvbeek Yes, due to this feature, php-8.1 is no longer supported.

I understand that there is a moment to drop PHP 8.1 (I have a project that is running on 8.1 that is why I found it) But I did suspect to see it in the changelog, that did me question if it was intended or not. 😃

@barryvdh
Copy link
Owner

Yeah I've updated the Release tab nog https://github.com/barryvdh/laravel-ide-helper/releases/tag/v3.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues after Laravel 11.15
6 participants