Conversation
|
@cseufert I would love your feedback on this PR. |
cseufert
left a comment
cseufert
left a comment
There was a problem hiding this comment.
This looks very close, thanks for getting this working.
I noticed a couple of things, the major one being making the hooks an empty list rather than null, and having the constructor param last, rather than in-between existing params.
| { | ||
| parser: { | ||
| version: "8.1", | ||
| suppressErrors: true, |
There was a problem hiding this comment.
Yes, because the array of error added to the AST is just gibberish. Only the first error is relevant: once the parser get confused/lost then it just create noise. It is very distracting/confusing when working on thigs and trying to understand what test fails and why.
There was a problem hiding this comment.
The array of errors thing is to allow parsing for tools like prettier where they can still format some of the code, even though the complete file is not valid AST (in theory at least, not sure that it works in practice)
| "kind": "program", | ||
| } | ||
| `; | ||
| exports[`Test classes Test that readonly method parameters are throwing errors 1`] = `"readonly properties can be used only on class constructor on line 3"`; |
There was a problem hiding this comment.
This looks like a regression.
There was a problem hiding this comment.
This is related to the change you commented in the class.test file https://github.com/glayzzle/php-parser/pull/1143/files#r1865229710
Instead of letting the parser getting lost and generate gibberish errors, the exception is fired and serialized, using toThrowErrorMatchingSnapshot.
There was a problem hiding this comment.
@czosel Whats your opinion on the change in the error handling?
| readonly, | ||
| nullable, | ||
| type, | ||
| hooks, |
There was a problem hiding this comment.
It would probably make sense to put hooks last as this would be less breaking of a change for existing code. Also making the default [] would make the type signature simpler, and reduce ambiguity between an empty list and null, as an empty property list is not valid syntax.
There was a problem hiding this comment.
Adding the hooks parameter at the end of the list would be inconsistent with how things have been done so far.
The PR introducing support for typed class constant added type "in the middle". https://github.com/glayzzle/php-parser/pull/1136/files#diff-33d9ef64a624f7abad7378a94559475184322ac87b7c864326beea672657adbcR34
There was a problem hiding this comment.
@cseufert I do not think it is possible to put hooks as the last parameters: when we create a node, the parameter docs and location are passed in by some other layer.
There was a problem hiding this comment.
Yer not last last, every node has docs and location last, this is automatically populated.
However if 3rd party code is replacing/creating new AST nodes, then this will cause some undefined behavior.
2 participants
Fixes #1142
RFC - https://wiki.php.net/rfc/property-hooks
Official documentation - https://www.php.net/manual/en/language.oop5.property-hooks.php