laravel-ide-helper
laravel-ide-helper copied to clipboard
Add support for protected Attribute accessors
Summary
The documentation for defining accessors states that “To define an accessor, create a protected method on your model to represent the accessible attribute.” (emphasis mine).
This is currently not supported by IDE Helper as noted in #1293.
The problem is that get_class_methods
doesn't return protected or private functions. The solution is to use Reflection instead and filter out any unwanted methods:
- Methods marked as
private
since marking an accessor asprivate
breaks the accessor and the two methods defined in - Two methods defined in each model through the
\Illuminate\Database\Eloquent\Concerns\HasAttributes
trait.
Any accessors marked as private will now not show up in the IDE Helper files. I chose to not mark this as a breaking change since those accessors would not have worked anyway.
This PR conflicts with #1338.
Type of change
- [x] New feature (non-breaking change which adds functionality)
Checklist
- [x] Existing tests have been adapted and/or new tests have been added
- [x] Add a CHANGELOG.md entry
- [x] Code style has been fixed via
composer fix-style
Can this PR be looked at? Using protected
rather than public is the documented way of using this functionality and prevents accidentally calling the method instead of the magic property.
Will soon check it out; this competes with https://github.com/barryvdh/laravel-ide-helper/pull/1325 right?
Will soon check it out; this competes with #1325 right?
Correct.
I would like to ask if someone could look into this again. The summary states the case/necessity for this PR.
There are some fairly major changes to ModelsCommand.php
, but all tests are green. They also result in that flow being more consistent: Everything now uses ReflectionClass
/ReflectionMethod
instead of only the parts that explicitly require that.
I would like to update this PR, but it relies on #1338. I would like to note that this PR is required to support the Attribute accessors that are new since Laravel 9. Without this PR, they are not recognised when following Laravel's documentation in implementing those.
I am currently using Attribute
in some places and it is making me very mad. I truly don't want to make the methods public
, because that is not how the framework works and just changing them to public
because we still don't have this fix merged (reported a long time ago) makes me really mad, as if anyone runs the command, we will lose those properties unless they are aware of this "bug" and they change the properties to public
, run the command, and move them back to protected
...
That would be unmaintainable... so please, @mfn @barryvdh or any collaborator, member or admin, can this be looked at and merged ASAP?
@pindab0ter I do see there are conflicts (but I can't see them), could you fix them so this can be taken care of immediately?
Would appreciate having this merged and release ASAP because it is a must (required-dev
) for any Laravel project I work on, so this is truly a gem package for devs, so having this issue not fixed breaks my heart 😥
I rebased this PR onto #1338 which is in turn rebased on the most recent commit on main
.
Please review #1338 as this PR relies on it. This is not a massive change and all changes are covered by tests. It would mean a lot to a lot of people if this was accepted.
@mfn With the 2.13.0 release, do you think it's possible to get this PR done?
I would like to reiterate that this package as of now still does not support a basic Laravel feature introduced in Laravel 9; Attribute accessors.
- I changed the visibility of the test methods (I assume this is what you're referring to) because this is what the Laravel Docs on Attributes states; "To define an accessor, create a protected method on your model […]." What reason is there for not wanting to change the visibility to what it should be from now on?
- I changed variable usage to reduce clutter as much as possible. I would suggest renaming
$method
to$methodName
to prevent possible confusion about the type of the variable once you have OKed this PR, if that's okay with you. - I made a few style changes because I felt the code style wasn't in line with the style of the rest of the code in those places. If you disagree, I can revert those changes.
Approving, the error with psalm is unrelated and needs its own fix.
Btw, reported this over there -> https://github.com/vimeo/psalm/issues/9345
Awesome, thanks!
What are your thoughts on this?
I would suggest renaming
$method
to$methodName
to prevent possible confusion about the type of the variable […].
Can you do #1338 next? It adds better support for (missing) type hinting of Attributes.