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

fix: cant auto complete method chaining #1446

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SocolaDaiCa
Copy link

Summary

Type of change

  • 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

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

cant auto complete method chaining

I find the description a bit lacking TBH. Thanks for the screenshots, at least helps a bit but…

The foreach block you added is more or less the same as the above it? The difference being the static?
image

I've a feeling this will make one case work and break another or generate misleading autocomplete in other cases.

Can someone else chime in here maybe?

Maybe the PR should also get a rebase against current master and I bet many test with snapshots will fail, when I'm reading the change right -> this should be adapte too.

@SocolaDaiCa
Copy link
Author

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

@mfn
Copy link
Collaborator

mfn commented Feb 18, 2024

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?

@SocolaDaiCa
Copy link
Author

@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

@mfn
Copy link
Collaborator

mfn commented Feb 20, 2024

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.

@SocolaDaiCa
Copy link
Author

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

@mfn
Copy link
Collaborator

mfn commented Feb 20, 2024

reasonable and safe

Please re-read #1446 (comment) , in the current form the PR creates issues with existing projects.

@SocolaDaiCa SocolaDaiCa force-pushed the fix/cant-auto-complete-method-chaining branch 4 times, most recently from 5fdd7dd to d2f1941 Compare February 20, 2024 13:03
@SocolaDaiCa SocolaDaiCa force-pushed the fix/cant-auto-complete-method-chaining branch from d2f1941 to 46b4c71 Compare February 20, 2024 13:10
@SocolaDaiCa
Copy link
Author

@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

@mfn
Copy link
Collaborator

mfn commented Feb 20, 2024

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

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.

3 participants