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

Improve replacement of return type for methods from Query\Builder #1575

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

pjio
Copy link
Contributor

@pjio pjio commented Jul 27, 2024

Summary

See issue for expected improvement: #1574

The class \Eloquent in _ide_helper.php contains methods from Illuminate\Database\Eloquent\Builder and Illuminate\Database\Query\Builder. The return type for $this is replaced with the real class (in case of the Eloquent\Builder already with an additional |static). This merge requests replaces $this in methods from Query\Builder with the identical return type Illuminate\Database\Eloquent\Builder|static. In the context of an \Eloquent instance this should be the real behavior and improves the derived types.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Code style has been fixed via composer fix-style

@pjio
Copy link
Contributor Author

pjio commented Jul 27, 2024

Here the old and new file for easier comparison:
_ide_helper.php.old.txt
_ide_helper.php.new.txt

@eldair
Copy link

eldair commented Aug 16, 2024

@pjio will this fix #1572 as well?

@pjio
Copy link
Contributor Author

pjio commented Aug 26, 2024

@pjio will this fix #1572 as well?

No, for #1572 it doesn't make a difference. The cause is a different one. This PR only affects the return type $this.

pjio added a commit to pjio/laravel-ide-helper that referenced this pull request Sep 3, 2024
pjio added a commit to pjio/laravel-ide-helper that referenced this pull request Sep 3, 2024
@pjio
Copy link
Contributor Author

pjio commented Sep 3, 2024

Changes 03.09.24

  • normalizeReturn() does set the return type only with $tag->setContent() (before it was overwritten by $tag->setType() which had the side effect to reset Tag::$content)
  • It is now clearer how $this could be normalized depending on the alias.
  • Generation of _ide_helper.php tested with Laravel 10 and PHP 8.1, 8.2 and 8.3
  • Set "Type of change" to "New feature" (the generated file changes but it shouldn't break anything)

Edit: Intelephense up to version 1.10.4 and again since version 1.12.6 works with the |static return type.

@CongAn
Copy link

CongAn commented Sep 5, 2024

Thank you very much for your valuable contributions!
I would greatly appreciate it if the project maintainers could merge this pull request and release a new version as soon as possible.

@barryvdh
Copy link
Owner

Is this fixed with 3.2 now adding the type?

@pjio
Copy link
Contributor Author

pjio commented Oct 29, 2024

Is this fixed with 3.2 now adding the type?

Not yet. This pull request additionally adds the type for various methods of \Eloquent in _ide_helper.php (I tested with a fresh Laravel 11 project and 3.2 to see if it already would work without the change).

Examples:

User::query()->where('name', 'joe')->first(); // `query()` from the model already works as of 3.2
User::where('name', 'joe')->first();          // `where` from Eloquent\Builder will work with this pull request
User::whereNull('name')->first();             // `whereNull` from Query\Builder will work with this pull request

@barryvdh barryvdh merged commit e3ec773 into barryvdh:master Oct 31, 2024
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.

4 participants