Skip to content

Loading…

DCOM-10: The annotation parser isn't EBNF compliant #396

Closed
doctrinebot opened this Issue · 6 comments

2 participants

@doctrinebot

Jira issue originally created by user bschussek:

The EBNF allows passing multiple comma-separated annotations to an annotation:

Annotation ::= "@" AnnotationName ["(" [Values] ")"]
Values ::= Array | Value {"," Value}*
Value ::= PlainValue | FieldAssignment
PlainValue ::= integer | string | float | boolean | Array | Annotation

Therefore the following should be possible.

/*** @Name(@Foo, @Bar) **/

This results in an error though.

IMO,

/*** @Name(@Foo, @Bar) **/

should be equivalent to

/*** @Name({@Foo, @Bar}) **/

just like

/*** @Name(foo = "foo", bar = "bar") **/

is equivalent to

/*** @Name({foo = "foo", bar = "bar"}) **/
@doctrinebot

Comment created by bschussek:

As I've just noticed, the statement that

/*** @Name(foo = "foo", bar = "bar") **/

equals

/*** @Name({foo = "foo", bar = "bar"}) **/

is wrong. The first does field assignments, the second stores the array in the "value" field.

Nevertheless, either Doctrine's implementation (as per my commit) or the EBNF have to be updated.

@doctrinebot

Comment created by @guilhermeblanco:

Your branch added another vulnerability, so I cannot merge.

The problem appears when you do this:

@Name(@Foo, {bar="bar"})

What would you expect on this situation? I'd imagine this:

array(
    0 => object<Foo>,
    1 => array(
        'bar' => 'bar'
    )
)

But with your patch, the actual result is:

array(
    0 => object<Foo>,
    'bar' => 'bar'
)

There's a way to fix it by changing how FieldAssignment returns. Instead of returning an array, it should return return an stdClass.
Then we could only update the Values grammar accordingly.

I'll assign this issue to me, so I can work on it to be EBNF compatible. Too bad it took so many time for us to look at it. =(

@doctrinebot

Comment created by bschussek:

Yes, I'd expect the first result. Thanks for looking into this, I completely forgot about this issue.

@doctrinebot

Comment created by @guilhermeblanco:

This issue was fixed in: 4210fbd

Thanks a lot for the bug report.
EBNF and Code are now compliant with each other. Took some time to be fixed, but here it is. =)

@doctrinebot

Issue was closed with resolution "Fixed"

@doctrinebot doctrinebot added the Bug label
@doctrinebot doctrinebot closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.