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

Supporting attributes restrictions on more targets, i.e. properties #224

Closed
francescolaffi opened this issue Dec 4, 2023 · 4 comments · Fixed by #225
Closed

Supporting attributes restrictions on more targets, i.e. properties #224

francescolaffi opened this issue Dec 4, 2023 · 4 comments · Fixed by #225

Comments

@francescolaffi
Copy link
Contributor

francescolaffi commented Dec 4, 2023

Currently disallowedAttributes support only on attributes that target classes/interfaces/... or functions/methods (by AttributeUsages::processNode)

But attributes can have other targets, I discovered this limitation while trying to restrict an attributes that is used on class properties.

I think it should be possible for AttributeUsages to support nodes for the other targets, but I'd like to know where @spaze stands on this:

  1. lets add support for properties only, and see if someone else needs other targets in the future
  2. lets support all possible attribute targets, by detecting the specific php parser nodes
  3. lets support all possible attribute targets, by duck typing on attrGroups
  4. not adding support for other attribute targets

I'd go for 2, let me know what you thing and if you'd welcome a PR (probably in a few weeks) or if you'd like to handle it directly

@spaze
Copy link
Owner

spaze commented Dec 4, 2023

4 is also fine 😂 But really, I haven't added support for other targets because it was more work at the time and the extension doesn't support properties elsewhere (which I admit is more of an excuse). But I thought I had at least documented it which turned out that I haven't, sorry!

I'm fine with adding support for just properties plus document the limitation, or add full support, and then probably no documentation changes needed. Not sure what's easier now 😅

Duck typing doesn't sound like something I'd like to see but may be needed, I don't know right now. PR is always welcome!

@spaze
Copy link
Owner

spaze commented Dec 6, 2023

Hey @francescolaffi I got somehow interested more now (= can't sleep) 😄 and created #225. It's marked as Draft because there are more nodes (in PhpParser teminology) that can be targeted with an atribute like enums, and possibly more, that are not yet supported. You can work on that PR if you want/can, otherwise I'll probably add those in the upcoming days.

spaze added a commit that referenced this issue Dec 6, 2023
This adds support for properties, class constants, params, enums

Close #224
spaze added a commit that referenced this issue Dec 6, 2023
This adds support for properties, class constants, params, enums

Close #224
@spaze
Copy link
Owner

spaze commented Dec 6, 2023

All targets are now supported, would still like to add tests for attributes for arrow function, closure, function, interface, trait.

spaze added a commit that referenced this issue Dec 7, 2023
This adds support for properties, class constants, params, enums

Close #224
spaze added a commit that referenced this issue Dec 7, 2023
This adds support for properties, class constants, params, enums

Close #224
spaze added a commit that referenced this issue Dec 9, 2023
This adds support for properties, class constants, params, enums

Close #224
@spaze spaze closed this as completed in #225 Dec 9, 2023
@francescolaffi
Copy link
Contributor Author

thanks @spaze! that was fast :)

tested it out from the main branch and it works as intended, notices something minor addressed in #229

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 a pull request may close this issue.

2 participants