-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 non-facade classes will result in no autocomplete #841
Conversation
Hoops, I accidentally removed the spaces (not that they should be there). |
Seems to me this is the same as #802 but with less "noise"? |
0c4188a
to
03b214d
Compare
Sorry for the late reply, the problem with #802 is that it will remove the Arr and Str classes from the namespace{
// ...
} section so the \Arr and \Str calls will lose autocomplete. Also the import use Illuminate\Support\Facades; should be use Illuminate\Support\Facades\Facade; because currently it filters out all the facades. I think this was some newer change because originally it worked for me (I think). This version does not have the mentioned issues. Also I rebased my pull request to the latest master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should update your branch, because there are changes in this method: getAliasesByExtendsNamespace
03b214d
to
e19f0a6
Compare
Ok. I rebased it to current master. |
<?php if ($namespace == '\Illuminate\Database\Eloquent') : | ||
continue; | ||
endif; ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was originally filtered out because it's not a facade so it doesn't need to be there but all non facade classes are filtered out already.
I found the original issue: #451, it was basically the same as this but then it was only limited to one class (Model) instead of three (Model, Arr, Str).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nice, thank you 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM
There's one thing I'm not sure: AFAIK people sometimes expect non-Facades also to show up here, which would have happened before.
Also I hope at some day we can have some tests for this, but I see that this isn't easy here…
Basically you don't want the non Facade classes show up there as they would be empty and break the aliased classes. They still show up in the alias section, so |
Thank you for your clarification, makes sense to me! |
Does this PR needs any more attention from me or is it good for now? |
Good question, I think we wait for approval from @barryvdh |
@barryvdh Any blocking reason here I should resolve? |
Still has my 👍🏼 ! |
ping |
* Filter out non-facade base classes * Remove Model as it is already skipped because it's not a facade
Currently non-facade classes are resulting non functioning autocomplete. The reason of this that an empty class will be defined in the ide helper class, which will be used for the auto-completion. This pull request prevents this behavior by only handling facades in the namespaced part of the helper file.
Unlike #802 this pull request will keep the alias registration.
Fixes #784