-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add support for non type-hinted attribute accessors with no backed property #1338
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
Conversation
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.
LGTM!
@barryvdh WDYT?
A little question, the PR works with "untyped get attributes", but what happen with "untyped set attributes"? |
Because of this I added a lot more tests including a few edge cases. I made it so that if there's a getter, that's the type, but otherwise it takes the parameter type of the setter (since the setter return type doesn't really matter). It also seems to seems that the PR is still approved even though I made new changes afterwards. @mfn could you please look at it again? |
any updates on this? would love to see this in action |
@mfn could you look at this PR? Its changes are well described and there's tests for both the happy path as well as a few edge cases. |
Looks good to me. @mfn good to merge? |
I'm still on vacation and will take me a couple days to check it out again. |
I would like to have this PR merged so that I can update #1339, which is required for the proper usage of the Attribute accessors that came with Laravel 9. |
Is there any update on this? This feature would be highly appreciated. |
@mfn Do you have any update about this? (Hope you had a nice vacation btw) |
No, not yet. Though I'm back from vacation 😅 it's been a super busy year so far. |
@mfn Any update? |
@mfn also looking forward to this change being out. Hopefully it's not a major one and can be merged before christmas. If not, no worries. Enjoy your christmas break and hope the following year isn't too busy |
I updated the PR to be in line with the most recent changes. I don't know what I did to close the PR. Please re-open it, review it and merge it if there are no issues. The PR contains tests that cover the changes made, so it shouldn't be that much work. |
It seems that you force pushed your branch to the HEAD commit of the upstream, so your changes have been overwritten (that's why the PR doesn't have any changes and is probably closed automatically) |
The branch was closed half an hour before I force pushed my changes. A force push won't close a PR. I don't remember clicking on any 'Close issue' buttons, either. Weird. |
Not sure if that gap is correct. Also, Github does close the PR automatically: |
Then it seems I have my facts mixed up. Thanks for pointing it out! I'll be sure to remember that for next time. |
You can force push your branch back to 3e671ab if you'd like to restore everything. (You might need to verify if that's the most recent changeset though) |
@mfn could you please re-open this PR and give it a look? I closed it by mistake and can't reopen it myself. |
I recreated the PR. At least we're sure that works! |
Summary
Even though the documentation doesn't state this explicitly at the time of writing, it is possible to use the new Attribute accessor to create a calculated property where there is no backing property.
This solves #1315.
I added a check for if the Attribute accessor function has no specified type, and then add it without type.
Type of change
Checklist
composer fix-style