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

Issues after Laravel 11.15 #1572

Closed
Sergiobop opened this issue Jul 17, 2024 · 17 comments · Fixed by #1591 or #1631
Closed

Issues after Laravel 11.15 #1572

Sergiobop opened this issue Jul 17, 2024 · 17 comments · Fixed by #1591 or #1631
Labels

Comments

@Sergiobop
Copy link

Versions:

  • ide-helper Version: 3.1
  • Laravel Version: 11.15
  • PHP Version: 8.3

Description:

Additionally to issues like #1571, Laravel 11.15 also ruined some other _ide_helper.php methods (find, findOrFail, etc)

I think is related to laravel/framework#51851 and laravel/framework#52037

Before 11.15:

/**
             * Find a model by its primary key or throw an exception.
             *
             * @param mixed $id
             * @param array|string $columns
             * @return \Illuminate\Database\Eloquent\Model|\Illuminate\Database\Eloquent\Collection|static|static[] 
             * @throws \Illuminate\Database\Eloquent\ModelNotFoundException<\Illuminate\Database\Eloquent\Model>
             * @static 
             */            public static function findOrFail($id, $columns = [])
            {
                                /** @var \Illuminate\Database\Eloquent\Builder $instance */
                                return $instance->findOrFail($id, $columns);
            }

After 11.15:

/**
             * Find a model by its primary key or throw an exception.
             *
             * @param mixed $id
             * @param array|string $columns
             * @return \Illuminate\Database\Eloquent\($id is (\Illuminate\Contracts\Support\Arrayable<array-key, mixed>|array<mixed>) ? \Illuminate\Database\Eloquent\Collection<int, TModel> : TModel)
             * @throws \Illuminate\Database\Eloquent\ModelNotFoundException<TModel>
             * @static 
             */            public static function findOrFail($id, $columns = [])
            {
                                /** @var \Illuminate\Database\Eloquent\Builder $instance */
                                return $instance->findOrFail($id, $columns);
            }

Now for example this is wrong:
$pack = Pack::query()->findOrFail($packId);

Previous to 11.15: $pack: Pack|Pack[]|Builder|Builder[]|Collection|Model|null
After 11.15: $pack: |Collection|Model|null

I don't know if we have to do something to make it work again in our end.

Thanks

@Sergiobop Sergiobop added the bug label Jul 17, 2024
@joelstein
Copy link

Same here.

For example, User::first() returns a User object. However, after Laravel 11.15, I get an intelephense error if I try to typehint it.

Expected type 'App\Models\User'. Found 'Illuminate\Database\Eloquent\TValue|null'.

From _ide_helper.php:

/**
 * Execute the query and get the first result.
 *
 * @param array|string $columns
 * @return \Illuminate\Database\Eloquent\TValue|null 
 * @static 
 */
public static function first($columns = [])
{
    /** @var \Illuminate\Database\Eloquent\Builder $instance */
    return $instance->first($columns);
}

@max13fr
Copy link

max13fr commented Jul 25, 2024

Hello,

Same for me, all models are viewed as Illuminate\Database\Eloquent\Builder instead of an instance of App\Models\MODELNAME.

Thanks in advance,
Max

@mengidd
Copy link

mengidd commented Jul 30, 2024

Same issue here. It seems that Laravel changed the DocBlocks for the Eloquent Builder class in Laravel 11.

It now has a template tag on the class itself @template TModel of \Illuminate\Database\Eloquent\Model, and it references this template on some of the methods @return TModel.

@tlevi101
Copy link

tlevi101 commented Sep 16, 2024

What worked for me is that i disabled inteliphense and i only had this php intelisense enabled. Then i added this phpdoc to MyModel: @mixin \Illuminate\Database\Eloquent\Model<\App\Models\MyModel> then regenerate _ide_helper_models.php.

@wattnpapa
Copy link
Contributor

I have the same problem

@alexandre-tobia
Copy link

Same problem here

@KentarouTakeda
Copy link
Contributor

I created a draft pull request #1591 to fix this issue. The tests don't pass, but the fix is ​​done.

A simple fix would require Laravel to only support versions 11.15 and later, which is a breaking change.

For compatibility, we could change the behavior depending on the running Laravel version, but because the Laravel Framework allows type changes even in minor versions, we might need to continue implementing similar version detection in the future. In my opinion, I don't recommend it.

After hearing your opinions on the versioning approach, I will complete the pull request.

@KentarouTakeda
Copy link
Contributor

After further trial and error, it seemed like maintaining compatibility with versions below Laravel 11.15 would be a tough road to overcome, so I completed a pull request narrowing the supported version.

@feng-yifan
Copy link

great job, thank you :)

@pataar
Copy link
Contributor

pataar commented Oct 18, 2024

While the changes are really good, one thing is still working incorrectly.

Model::create(), Model::find(), Model::make() are still returning the wrong type. This is due the \Eloquent mixin which is not working with generics. My suggestion would still be to replace the default @mixin \Eloquent with @mixin Builder<static> which would improve the autocompletion.

Eloquent mixin:
image

Builder mixin:
Screenshot 2024-10-18 at 14 26 13

@KentarouTakeda What do you think?

@KentarouTakeda
Copy link
Contributor

Yes, I think it would be useful if it were fixed. On the other hand, I prefer to explicitly call the Eloquent Builder with query().

$user = User::query()->create();
// Identified as `\App\Models\User`

$user = User::create();
// Identified as `mixed`

In my opinion, Model and Builder are different classes even in terms of their inheritance, so I think it's better not to mix them.

By the way, the Eloquent mixin example you showed did not produce the same results in my environment. I don't know if it's Laravel or IDE Helper, but it may be written in a way that is interpreted differently depending on the editor. I use VSCode and PHP Intellephense.

vscode-intelephense-result

@pataar
Copy link
Contributor

pataar commented Oct 21, 2024

Yes, I think it would be useful if it were fixed. On the other hand, I prefer to explicitly call the Eloquent Builder with query().

$user = User::query()->create();
// Identified as `\App\Models\User`

$user = User::create();
// Identified as `mixed`

In my opinion, Model and Builder are different classes even in terms of their inheritance, so I think it's better not to mix them.

By the way, the Eloquent mixin example you showed did not produce the same results in my environment. I don't know if it's Laravel or IDE Helper, but it may be written in a way that is interpreted differently depending on the editor. I use VSCode and PHP Intellephense.

vscode-intelephense-result

That might be caused due the @mixin \Eloquent entry. What happens if you remove that one? Does it still indicate 'mixed'?

I agree that a Model should be a different class than Builder. Although the docs state that ::create, ::where and ::find are valid methods.

@KentarouTakeda
Copy link
Contributor

I removed @mixin \Eloquent as you suggested, but it didn't change anything.

builder-mixin

By the way, I noticed a strange output in the DocBlock generated to _ide_helper.php in recent versions.

helpers-eloquent-create

/**
 * @return \Illuminate\Database\Eloquent\TModel
 */
public static function create($attributes = [])

As you can see, a class that doesn't actually exist is being output as the return type. This is probably a bug.

A reasonable solution would be to make ide-helper:generate generic-compatible, and modify the Eloquent class itself to accept type parameters, just like I did with ide-helper:models. I think this will get you the results you want.

@pataar
Copy link
Contributor

pataar commented Oct 21, 2024

I removed @mixin \Eloquent as you suggested, but it didn't change anything.

builder-mixin

By the way, I noticed a strange output in the DocBlock generated to _ide_helper.php in recent versions.

helpers-eloquent-create

/**
 * @return \Illuminate\Database\Eloquent\TModel
 */
public static function create($attributes = [])

As you can see, a class that doesn't actually exist is being output as the return type. This is probably a bug.

A reasonable solution would be to make ide-helper:generate generic-compatible, and modify the Eloquent class itself to accept type parameters, just like I did with ide-helper:models. I think this will get you the results you want.

Thanks, perhaps the interpretation of generics is different between PhpStorm and VSCode. I'll try to fix this in a PR soonish.

@gyulavoros
Copy link

@pataar Would it make sense to reopen this ticket unit "create returns Tmodel" problem is fixed? I've just run into the same issue.

@hauthorn
Copy link

@pataar I second @gyulavoros, and would like to add that it also affects Model::find() and similar. Calling explicitly with query() every time is in conflict with the succinctness that Eloquent offers IMO.

@chack1172
Copy link

chack1172 commented Dec 18, 2024

@pataar @hauthorn I'm not sure if this is the correct way of fix this, but adding this docbloc for TModel of Eloquent class in ide-helper fix the return type in vscode:

image

single:
image

multiple:
image

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