barryvdh / laravel-ide-helper

IDE Helper for Laravel
MIT License
14.16k stars 1.16k forks source link

ISSUE :: Model Static Classes ... continuing issue #541

Closed Artistan closed 4 years ago

Artistan commented 7 years ago

There is an update to Laravel 5.4 that makes the relations more clear to all (should be in v5.4.29) https://github.com/laravel/framework/commit/77d8c23fed764e6a38cc8608c28f0ad7c39deee4

The only problem is that the helper @mixin \Eloquent needs to move to the Model class in order to work with the magic static methods properly https://github.com/laravel/framework/pull/20044/files

Note ... you should no longer have to add that to each instance of a Model. It works with just the static definitions on the Model class.

codeheart09 commented 7 years ago

I'm getting this warning...

"Non-static method 'where' should not be called statically less... (Ctrl+F1) Dynamic class method called as static."

for an eloquent call like that: $entry = SomeModel::where(...)->get();

... in PHPStorm on my updated laravel project.

I notice that when I CTRL+click the 'where' method on the eloquent call above it takes me to the builder.php, where the 'where' method is really not-static.

But, when I CTRL+click the 'where' method on some project that I didn't ran the composer update, it takes me to the _ide_helper.php.

Is this issue mentioned above?

buglinjo commented 7 years ago

AnkurPrem same here... I'm having problem with findOrFail();

Non-static method 'findOrFail' should not be called statically

and when I CTRL+click on it, it goes to Builder.php, not in _ide_helper.php.

KKSzymanowski commented 7 years ago

@buglinjo It goes to the Builder because now Builder is hinted as a mixin in Model.

https://github.com/laravel/framework/blob/5.4/src/Illuminate/Database/Eloquent/Model.php#L19

jonphipps commented 7 years ago

The pull request that @Artistan submitted works, but I think it works because of a bug in PhpStorm. Here's my research on the problem in a comment on the PhpStorm support site: https://intellij-support.jetbrains.com/hc/en-us/community/posts/115000473830/comments/115000386750

Basically it looks like PhpStorm overwrites class instance mixins with the parent class mixins rather than merging them. So removing the new mixin introduced in 5.4.29 from the Laravel Model class fixes it, and adding "@mixin \Eloquent" to the Laravel Model class also fixes it. But just adding "@mixin \Eloquent" to the Model instance, as ide_helper does, doesn't work.

Artistan commented 7 years ago

I did a pull request to Laravel for adding "@mixin \Eloquent" to the Laravel Model. This, of course, was rejected because it is not a laravel class, it is a helper mixin.

As I stated in my original message here... "The only problem is that the helper @mixin \Eloquent needs to move to the Model class in order to work with the magic static methods properly"

Maybe @jonphipps stated it clearer, If the @mixin \Eloquent is added to the Laravel Model class, it fixes it. This should be able to be automatically done by the ide helper.

!!!Advantage! No need for "@mixin \Eloquent" on every Model instance.

The main point we both are trying to make...

ADD THE "@mixin \Eloquent" to the Laravel Model instance via the helper!

Artistan commented 7 years ago

...Model... image I will be looking into the ide helper can automatically do this. Everything forks fine for me when this is added.

Artistan commented 7 years ago

oops

Artistan commented 7 years ago

adding the class reference and example for a quick fix here...

In the laravel database eloquent model class. ./vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php

add the Eloquent mixin used by the ide helper.

/**
 * @mixin \Illuminate\Database\Eloquent\Builder
 * @mixin \Eloquent
 */

And, you no longer need the mixin on the individual Model classes.

jonphipps commented 7 years ago

@Artistan Curious how you can fix this in ide_helper, since PhpStorm stomps on @mixin in any extension of Eloquent\Model. Maybe there's some other magic for getting PhpStorm to recognize the \Eloquent class as an Eloquent\Model mixin?

Without a fix in PhpStorm, it seems like the best fix might be to revert https://github.com/laravel/framework/commit/77d8c23fed764e6a38cc8608c28f0ad7c39deee4 and add the Eloquent\Builder mixin to ide_helper. That would have the same effect.

Artistan commented 7 years ago

that commit is a proper mixin for the Model because the Model literally mixes in the Builder methods via magic method calls. adding @mixin \Eloquent to the model just allows the model to recognize the mix..

image

With the update, this mixin is only needed on the Model itself, not any of the instances that extend it. the ide_helper could add that automatically.

Artistan commented 7 years ago

That just solved the problem for me... The commit should have also extended the Query\Builder since the Eloquent\Builder does magic calls to the Query\Builder. In essence, it is inheriting method from both classes via the magic method calls.
The ide helper did this via the \Eloquent mixin, but it really should be on the laravel class since that class does "implement" all of those methods.

I will be doing a new pull request with this in the Model, it fixes it all with no help from other things.

/**
 * @mixin \Illuminate\Database\Eloquent\Builder
 * @mixin \Illuminate\Database\Query\Builder
 */
Artistan commented 7 years ago

https://github.com/laravel/framework/pull/20224

Artistan commented 7 years ago

This can be closed as soon as that pull request is accepted.

Artistan commented 7 years ago

Actually, The Eloquent/Builder has a mixin for the Query/Builder Could it be a bug with the fact that the Model is not inheriting the mixin methods on the Eloquent/Builder for the Query/Builder?

Artistan commented 7 years ago

so, this update fixes the PhpStorm reference to valid functions, but not static ones. The quick fix for static calls is to add the * @mixin \Eloquent line to the Model class....

vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php


/**
 * @mixin \Eloquent
 * @mixin \Illuminate\Database\Eloquent\Builder
 */
abstract class Model implements ArrayAccess, Arrayable, Jsonable, JsonSerializable, QueueableEntity, UrlRoutable
{
    use Concerns\HasAttributes,

please go here and let people know that we want to be able to have dynamic static call autocompletion as part of PhpStorm! https://youtrack.jetbrains.com/issue/WI-21620#comment=27-2336224

There is a lot of rhetoric about this! So it would be great to get more positive requests. https://youtrack.jetbrains.com/issue/WI-28043

dev commented 7 years ago

Thanks @Artistan! 👍

RalphMRivera commented 7 years ago

Is the summary that this is not really fixable until PHPStorm updates code on their end?

Artistan commented 7 years ago

The current fix is in here... which automates adding the \Eloquent mixin to the Eloquent\Model. https://github.com/barryvdh/laravel-ide-helper/pull/544

Or you can manually do it like this... https://github.com/barryvdh/laravel-ide-helper/issues/541#issuecomment-317411918

Artistan commented 7 years ago

model should be

/**
 * @mixin \Eloquent
 * @mixin \Illuminate\Database\Eloquent\Builder
 */

it does include \Illuminate\Database\Query\Builder also. but there are issues with "call as static function" when both are on the model....

Artistan commented 7 years ago

again, this is an issue of static magic methods calling calling non-static functions on an instance of the object. There is no way for phpStorm to currently handle this. I believe @phpdoc needs to have a @mixin-static implementation.

felorhik commented 7 years ago

Another way to fix this that does not involve phpdoc.

Dynamic class method called as static.

is the inspection in php storm that handles it, it can be disabled in Settings > Editor > Inspections > PHP > General.

Maybe not the best way to go about it, but works since laravel really does not play nice with that inspection anyway, PHP storm understanding the magic is only 1 IDE of many that could run into something similar. Also keeps you from needing to change code to get the pesky inspection to go away.

sauron918 commented 7 years ago

The problem is still present: Laravel Framework 5.5.3, Laravel IDE Helper v2.4.1, PhpStorm 2017.2.3 Build #PS-172.4155.25. Any updates?

abdelAudace commented 7 years ago

so we need to add @mixin \eloquent and remove the @mixin\Illuminate\Database\Query\Builder or just add the eloquent?!

because it doesn't work for me just to add @mixin \eloquent

please help

codeheart09 commented 7 years ago

As @Artistan commented here, your model should have only these on phpdoc:

/**
 * @mixin \Eloquent
 * @mixin \Illuminate\Database\Eloquent\Builder
 */

Remove all other mixins, if not explicitly entered by you for some particular reason.

Edit: add this to your Model base class so it will affect all your extending classes/models.

emadmrz commented 7 years ago

The problem is still present laravel 5.5 phpstorm 2017.2.

mitoop commented 7 years ago

@emadmrz Realtion to laravel/framework

Artistan commented 7 years ago

https://github.com/laravel/framework/pull/21183 this update to revert the docs is just making the issue worse IMHO.

Artistan commented 7 years ago

oh well, there will be no complete solution until the IDEs will support both static and non-static method calls to the same function via magic methods.

Artistan commented 7 years ago

I want to emphasize that this is an IDE issue.

please go here and let people know that we want to be able to have dynamic static call autocompletion as part of PhpStorm! https://youtrack.jetbrains.com/issue/WI-21620#comment=27-2336224

There is a lot of rhetoric about this! So it would be great to get more positive requests. https://youtrack.jetbrains.com/issue/WI-28043

mitoop commented 7 years ago

@Artistan How IDE correctly identify the magic method? Magic method has body which we can do many things and almost always do many things in it, IDE can identify language grammar, how it identify developers's own code?

Artistan commented 6 years ago

@mitoop The issue in respect to the IDE is that the IDE will not recognize magic method calls to callStatic and to call even if we define them as such.

you can mixin the methods of a class that can be called via magic methods like the eloquent model...

/**
 * Class Model.
 *
 * @mixin \Illuminate\Database\Eloquent\Builder
 * @mixin \Illuminate\Database\Query\Builder
 *

ultimately it would be great to be able to define methods as both static and instance methods. Then the IDE could allow completion and validation for both.

/**
 * Class Model.
 *
 *  ** mix them in as instance methods
 * @mixin \Illuminate\Database\Eloquent\Builder
 * @mixin \Illuminate\Database\Query\Builder
 *   ** mix them in as static method calls
 * @mixin static \Illuminate\Database\Eloquent\Builder
 * @mixin static \Illuminate\Database\Query\Builder
 */
ricardoaugusto commented 6 years ago

PHPStorm 2017.3.4 Laravel 5.5.35

Here's how to give up, since the @mixin stuff didn't work for me: https://github.com/barryvdh/laravel-ide-helper/issues/541#issuecomment-325008705

floflock commented 6 years ago

Quick fix did not worked for me, PHP Storm is still warning...

I have disabled the inspector for that kind of method call: image

Also vote for: https://youtrack.jetbrains.com/issue/WI-28043

iget-master commented 6 years ago

Any working workaround for this that doesn't involve disabling the inspection on PHPStorm?

jonphipps commented 6 years ago

This unmerged pull request fixes it for me: #633

renanBritz commented 6 years ago

Workaround:

/** @var $User \Eloquent */
$User = \App\User::class;
$User::where('clauses')->get();

I'm not really comfortable doing that, but its an option...

PHPStorm 2018.1.2

mfn commented 4 years ago

This works nowadays correctly, PhpStorm understands that due to the @mixin, the OP even edited the issue description:

Note ... you should no longer have to add that to each instance of a Model. It works with just the static definitions on the Model class.

@barryvdh there's much feedback here but IMHO it all boils down to recent version of everything working correctly, IMHO can be closed :}

kodmanyagha commented 3 years ago

https://blog.nixarsoft.com/2021/04/05/autocomplete-the-eloquent-builder-methods-in-phpstorm/

iwasherefirst2 commented 1 year ago

This works nowadays correctly, PhpStorm understands that due to the @mixin, the OP even edited the issue description:

Note ... you should no longer have to add that to each instance of a Model. It works with just the static definitions on the Model class.

@barryvdh there's much feedback here but IMHO it all boils down to recent version of everything working correctly, IMHO can be closed :}

I used to not have this issue but suddenly it appeared again. Using PhpStorm 2022.2.1, this package, and a Laravel 8 project. Also tried the mixing


* @mixin \Illuminate\Database\Eloquent\Builder
 * @mixin \Illuminate\Database\Query\Builder

Still have the issue image

Related: https://github.com/barryvdh/laravel-ide-helper/issues/1191