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

pindab0ter commented 2 years ago

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

[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

MrJmpl3 commented 2 years ago

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

pindab0ter commented 2 years ago

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?

sawirricardo commented 2 years ago

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

pindab0ter commented 2 years ago

@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 commented 2 years ago

Looks good to me. @mfn good to merge?

mfn commented 2 years ago

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

pindab0ter commented 2 years ago

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 commented 2 years ago

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

pataar commented 1 year ago

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

mfn commented 1 year ago

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

KentarouTakeda commented 1 year ago

@mfn Any update?

bretto36 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:
https://stackoverflow.com/a/46053849

pindab0ter commented 1 year ago

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 commented 1 year ago

You can force push your branch back to https://github.com/barryvdh/laravel-ide-helper/commit/3e671ab9182b294392517696160604a94e9ce278 if you'd like to restore everything. (You might need to verify if that's the most recent changeset though)

pindab0ter commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

I can't, the button stays grayed out:

pindab0ter commented 1 year ago

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

barryvdh / laravel-ide-helper