Add support for protected Attribute accessors #1339

pindab0ter · 2022-04-06T10:14:07Z

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

New feature (non-breaking change which adds functionality)

Checklist

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

pindab0ter · 2022-04-07T13:54:51Z

src/Console/ModelsCommand.php

@@ -1096,6 +1096,9 @@ protected function hasCamelCaseModelProperties()

    protected function getAttributeReturnType(Model $model, \ReflectionMethod $reflectionMethod): Collection
    {
+        // Private/protected ReflectionMethods require setAccessible prior to PHP 8.1
+        $reflectionMethod->setAccessible(true);


I chose not to wrap this in an if-statement checking for PHP_VERSION_ID since according to the docs from PHP 8.1 onward this method call is a noop.

pindab0ter · 2022-04-16T20:24:27Z

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.

da-mask

Looks good to me, but I'm a nobody.

mfn · 2022-05-04T08:50:57Z

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

pindab0ter · 2022-05-04T08:52:11Z

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

Correct.

pindab0ter · 2022-07-18T07:51:16Z

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.

danieleambrosino

Looks perfectly fine to me, hope this will get reviewed and merged by a maintainer soon.
Very good job @pindab0ter!

pindab0ter · 2022-08-16T11:45:15Z

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.

matiaslauriti · 2022-08-19T21:03:01Z

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 😥

pindab0ter · 2023-01-05T13:13:03Z

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 · 2023-02-06T11:22:05Z

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

mfn

👋🏼

Approach LGTM, I'm also preferring this over #1325 , covers more cases.

My feedback:

please don't change the visibility of existing attributes -> please create a new column/attribute using protected (basically like you added the private to show
to improve readability of the PR, I suggest to add $method = $methodReflection->getName(); at the top of the loop -> this will reduce a LOT of noise in the PR (except for the cases where indeed have to pass on the reflect object)
resolve the merge conflicts

(ps: any reason for the style changes?)

thanks!

pindab0ter · 2023-02-18T22:47:54Z

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.

mfn

PR is so much more readable now, thank you!

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

mfn · 2023-02-19T19:52:35Z

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

Btw, reported this over there -> vimeo/psalm#9345

pindab0ter · 2023-02-19T21:53:45Z

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.

* Add support for protected Attribute accessors * Fix formatting * Prevent accessors methods marked as private from being added * Exclude specific accessors based on trait instead of class * Add changelog entry * Add clarifying comment * Fix accessor attributes not working on PHP < 8.1 * Reintroduce method variable to reduce PR clutter * Change variable name to reduce PR clutter * Update CHANGELOG.md --------- Co-authored-by: Barry vd. Heuvel <[email protected]> Co-authored-by: Barry vd. Heuvel <[email protected]>

pindab0ter force-pushed the feature/protected-accessor branch 2 times, most recently from f2e0c6d to 4273911 Compare April 6, 2022 10:38

pindab0ter marked this pull request as ready for review April 6, 2022 10:38

pindab0ter commented Apr 7, 2022

View reviewed changes

This comment was marked as duplicate.

Sign in to view

da-mask approved these changes Apr 29, 2022

View reviewed changes

danieleambrosino approved these changes Jul 19, 2022

View reviewed changes

pindab0ter mentioned this pull request Aug 16, 2022

Add support for non type-hinted attribute accessors with no backed property #1338

Closed

4 tasks

jackwh mentioned this pull request Nov 26, 2022

Proposal: Set IDE Helper definitions using PHP 8 Attributes #1396

Open

pindab0ter force-pushed the feature/protected-accessor branch from 0bd2e90 to 6bc3404 Compare January 5, 2023 13:07

pindab0ter force-pushed the feature/protected-accessor branch from 6bc3404 to b1f85ab Compare January 24, 2023 15:32

pindab0ter mentioned this pull request Jan 24, 2023

Add support for non type-hinted attribute accessors with no backed property #1411

Merged

4 tasks

mfn requested changes Feb 18, 2023

View reviewed changes

mfn mentioned this pull request Feb 18, 2023

handle protected methods with the attribute syntax #1325

Open

4 tasks

pindab0ter added 8 commits February 18, 2023 23:20

Add support for protected Attribute accessors

b709166

Fix formatting

3de36b9

Prevent accessors methods marked as private from being added

fab3e70

Exclude specific accessors based on trait instead of class

a27a1d3

Add changelog entry

94c05b2

Add clarifying comment

f119ba6

Fix accessor attributes not working on PHP < 8.1

80d8864

Reintroduce method variable to reduce PR clutter

3f923d4

pindab0ter force-pushed the feature/protected-accessor branch from b1f85ab to 3f923d4 Compare February 18, 2023 22:34

Change variable name to reduce PR clutter

2e996b6

mfn approved these changes Feb 19, 2023

View reviewed changes

barryvdh and others added 3 commits February 20, 2023 09:39

Merge branch 'master' into feature/protected-accessor

1b507d3

Update CHANGELOG.md

917b06e

Merge branch 'master' into feature/protected-accessor

a08cf94

barryvdh merged commit 3988564 into barryvdh:master Feb 20, 2023

pindab0ter deleted the feature/protected-accessor branch February 20, 2023 08:45

mfn mentioned this pull request Aug 23, 2023

New format computed property accessor not recognised #1315

Open

mfn mentioned this pull request Jan 28, 2024

Prepare 2.14.0 release #1503

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for protected Attribute accessors #1339

Add support for protected Attribute accessors #1339

pindab0ter commented Apr 6, 2022 •

edited

Loading

pindab0ter Apr 7, 2022

pindab0ter commented Apr 16, 2022

This comment was marked as duplicate.

da-mask left a comment

mfn commented May 4, 2022

pindab0ter commented May 4, 2022

pindab0ter commented Jul 18, 2022

danieleambrosino left a comment

pindab0ter commented Aug 16, 2022

matiaslauriti commented Aug 19, 2022 •

edited

Loading

pindab0ter commented Jan 5, 2023

pindab0ter commented Feb 6, 2023

mfn left a comment •

edited

Loading

pindab0ter commented Feb 18, 2023 •

edited

Loading

mfn left a comment

mfn commented Feb 19, 2023

pindab0ter commented Feb 19, 2023

Add support for protected Attribute accessors #1339

Add support for protected Attribute accessors #1339

Conversation

pindab0ter commented Apr 6, 2022 • edited Loading

Summary

Type of change

Checklist

pindab0ter Apr 7, 2022

Choose a reason for hiding this comment

pindab0ter commented Apr 16, 2022

This comment was marked as duplicate.

da-mask left a comment

Choose a reason for hiding this comment

mfn commented May 4, 2022

pindab0ter commented May 4, 2022

pindab0ter commented Jul 18, 2022

danieleambrosino left a comment

Choose a reason for hiding this comment

pindab0ter commented Aug 16, 2022

matiaslauriti commented Aug 19, 2022 • edited Loading

pindab0ter commented Jan 5, 2023

pindab0ter commented Feb 6, 2023

mfn left a comment • edited Loading

Choose a reason for hiding this comment

pindab0ter commented Feb 18, 2023 • edited Loading

mfn left a comment

Choose a reason for hiding this comment

mfn commented Feb 19, 2023

pindab0ter commented Feb 19, 2023

pindab0ter commented Apr 6, 2022 •

edited

Loading

matiaslauriti commented Aug 19, 2022 •

edited

Loading

mfn left a comment •

edited

Loading

pindab0ter commented Feb 18, 2023 •

edited

Loading