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

handle protected methods with the attribute syntax #1325

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mridul89
Copy link

@mridul89 mridul89 commented Mar 4, 2022

Summary

Laravel introduced a new syntax to define attribute "accessors / mutators" in laravel/framework#40022
Support for this new syntax was added in #1289

However, laravel documentation asks to use protected methods, but #1289 only looked at the public methods. This PR will also include protected methods.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Add a CHANGELOG.md entry
  • Code style has been fixed via composer fix-style
  • Updated the existing test and the corresponding snapshot

the test now uses protected method, which is the correct way per laravel documentation
@sforward
Copy link
Contributor

@barryvdh I've tested this in our applications and it works perfectly. Could you look into this some time?

@keatliang2005
Copy link

@barryvdh can review this PR ?

@mfn
Copy link
Collaborator

mfn commented Feb 18, 2023

FTR: this competes with #1339 which covers additional cases and is thus preferred

@barryvdh
Copy link
Owner

I merged #1339
Does this solve your issue? @keatliang2005 @mridul89 @sforward

@vpratfr
Copy link

vpratfr commented Apr 11, 2024

I merged #1339 Does this solve your issue? @keatliang2005 @mridul89 @sforward

Hi,

#1531 is still pending, and may have been introduced by #1339. I don't know however if this one addresses that issue.

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.

None yet

6 participants