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

Add support for protected Attribute accessors

Open pindab0ter opened this issue 2 years ago • 6 comments

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 as private 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

pindab0ter avatar Apr 06 '22 10:04 pindab0ter

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.

pindab0ter avatar Apr 16 '22 20:04 pindab0ter

Will soon check it out; this competes with https://github.com/barryvdh/laravel-ide-helper/pull/1325 right?

mfn avatar May 04 '22 08:05 mfn

Will soon check it out; this competes with #1325 right?

Correct.

pindab0ter avatar May 04 '22 08:05 pindab0ter

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.

pindab0ter avatar Jul 18 '22 07:07 pindab0ter

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.

pindab0ter avatar Aug 16 '22 11:08 pindab0ter

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 😥

matiaslauriti avatar Aug 19 '22 21:08 matiaslauriti

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.

pindab0ter avatar Jan 05 '23 13:01 pindab0ter

@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.

pindab0ter avatar Feb 06 '23 11:02 pindab0ter

  • 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.

pindab0ter avatar Feb 18 '23 22:02 pindab0ter

Approving, the error with psalm is unrelated and needs its own fix.

Btw, reported this over there -> https://github.com/vimeo/psalm/issues/9345

mfn avatar Feb 19 '23 19:02 mfn

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.

pindab0ter avatar Feb 19 '23 21:02 pindab0ter