laravel-ide-helper icon indicating copy to clipboard operation
laravel-ide-helper copied to clipboard

fix: cant auto complete method chaining

Open SocolaDaiCa opened this issue 1 year ago • 8 comments

Summary

Type of change

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update
  • [ ] Misc. change (internal, infrastructure, maintenance, etc.)

Checklist

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

Before:

image

image

After:

image

image

SocolaDaiCa avatar Jun 12 '23 13:06 SocolaDaiCa

@mfn yup, The difference being the static, because :: only hint static method, So for each method I add a corresponding static method

SocolaDaiCa avatar Feb 18 '24 13:02 SocolaDaiCa

The issues I see:

  • double definition issue confuses phpstan It does not seem like a problem per se with phpstorm, it is able to handle both variants and also jump corrects to their respective lines in phpdoc.

    The problem is that in PHP, you can't define a method with two kind of access patterns like this.

    Therefore, also static analyzers will be confused and it will break them.

    For example:

    • use phpstan on a project
    • have this code YourModel::query()
    • and now it will complain :

      Static call to instance method Models\YourModel::query().

    phpstan sees both definitions and, I'm guessing here, the latter will win.

  • the magic methods newModelQuery, newQuery and query don't exist as non-static version, you will get an error like this:

    BadMethodCallException Call to undefined method Illuminate\Database\Eloquent\Builder::newModelQuery().

    I'm not sure if there may other magic methods be added due to other circumstances.

The PR description talks about the custom scopes, but the change "changes everything else too". So maybe the PR isn't well scoped code-wise and should only target your custom scope issue?

mfn avatar Feb 18 '24 18:02 mfn

@mfn I'm sorry I was a little confused in the previous explanation, I'll explain again

I define scopeActive in the User model Currently ide-helper is @anotation scope method as static when i type User::ac => it suggests me User::active but when I type User::query()->ac it won't suggest me the active method, or when I type User::query()->where(...)->ac it won't suggest the method either activate me Therefore, I added a non-static method for better suggestion and combined with method chaining

Perhaps the best way is to remove static in the ide-helper @anotation instead of adding both static and non-static methods

SocolaDaiCa avatar Feb 19 '24 23:02 SocolaDaiCa

Perhaps the best way is to remove static in the ide-helper @anotation instead of adding both static and non-static methods

But this is a double edged sword: some only work for static, other for both, some only dynamic probably.

I don't see how we can do this in the current form without causing collateral issues.

mfn avatar Feb 20 '24 07:02 mfn

I don't see how we can do this in the current form without causing collateral issues.

Because all the @anotations that ide-helper creates for the model are scope and where, they certainly work in both static and non-static forms. I can't think of a case where it only works in static form

and laravel also recommends using the form Model::query()-><method1>()-><method2>() instead of the form Model::<method1>()-><method2>().

So I think changing @anotation scope and where from static to non static is reasonable and safe

SocolaDaiCa avatar Feb 20 '24 11:02 SocolaDaiCa

reasonable and safe

Please re-read https://github.com/barryvdh/laravel-ide-helper/pull/1446#issuecomment-1951412049 , in the current form the PR creates issues with existing projects.

mfn avatar Feb 20 '24 12:02 mfn

@mfn Please help me check again

I've fixed it, non-static is now only applied to function scope and where<column>

If anything is not right, please let me know

image

SocolaDaiCa avatar Feb 20 '24 13:02 SocolaDaiCa

Can you please also run the test suite so we see the changes?

mfn avatar Feb 20 '24 18:02 mfn