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

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

Closed
wants to merge 0 commits into from

Conversation

pindab0ter
Copy link
Contributor

@pindab0ter pindab0ter commented Apr 6, 2022

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

  • 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 pindab0ter marked this pull request as ready for review April 6, 2022 01:02
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@barryvdh WDYT?

@MrJmpl3
Copy link

MrJmpl3 commented Apr 7, 2022

A little question, the PR works with "untyped get attributes", but what happen with "untyped set attributes"?

@pindab0ter
Copy link
Contributor Author

pindab0ter commented Apr 8, 2022

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?

@pindab0ter pindab0ter requested a review from mfn April 8, 2022 12:42
@sawirricardo
Copy link

any updates on this? would love to see this in action

@pindab0ter
Copy link
Contributor Author

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

@barryvdh
Copy link
Owner

Looks good to me. @mfn good to merge?

@mfn
Copy link
Collaborator

mfn commented Jul 18, 2022

I'm still on vacation and will take me a couple days to check it out again.

@pindab0ter
Copy link
Contributor Author

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.

@EmanueleCoppola
Copy link

Is there any update on this? This feature would be highly appreciated.

@pataar
Copy link
Contributor

pataar commented Oct 25, 2022

@mfn Do you have any update about this? (Hope you had a nice vacation btw)

@mfn
Copy link
Collaborator

mfn commented Oct 31, 2022

No, not yet. Though I'm back from vacation 😅 it's been a super busy year so far.

@KentarouTakeda
Copy link
Contributor

@mfn Any update?

@bretto36
Copy link

@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

@pindab0ter
Copy link
Contributor Author

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.

@pataar
Copy link
Contributor

pataar commented Jan 5, 2023

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)

@pindab0ter
Copy link
Contributor Author

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.

@pataar
Copy link
Contributor

pataar commented Jan 5, 2023

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.

image

image

Also, Github does close the PR automatically:
https://stackoverflow.com/a/46053849

@pindab0ter
Copy link
Contributor Author

Then it seems I have my facts mixed up. Thanks for pointing it out! I'll be sure to remember that for next time.

@pataar
Copy link
Contributor

pataar commented Jan 5, 2023

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)

@pindab0ter
Copy link
Contributor Author

pindab0ter commented Jan 5, 2023

The branches were a bit of a mess. I rebased both this and #1339 on the most recent develop and force pushed to both make sure they're up-to-date and untangle the branch mess.

With that in mind, do you think we're good?

Edit: By the way. I force pushed #1339 as well and that didn't close.

@pindab0ter
Copy link
Contributor Author

@mfn could you please re-open this PR and give it a look? I closed it by mistake and can't reopen it myself.

@mfn
Copy link
Collaborator

mfn commented Jan 24, 2023

I can't, the button stays grayed out:
image

@pindab0ter
Copy link
Contributor Author

I recreated the PR. At least we're sure that works!

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.

9 participants