Doctrine Common

The annotation parser isn't EBNF compliant

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.0.0-BETA4
  • Fix Version/s: None
  • Component/s: Annotations
  • Labels:
    None
  • Environment:
    PHP 5.3.2, Linux Ubuntu 10.04

Description

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"}) */

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Bernhard Schussek added a comment -

Fixed in http://github.com/bschussek/doctrine-common/tree/DCOM-10

Show
Bernhard Schussek added a comment - Fixed in http://github.com/bschussek/doctrine-common/tree/DCOM-10
Hide
Permalink
Bernhard Schussek added a comment -

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.

Show
Bernhard Schussek added a comment - 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.
Hide
Permalink
Guilherme Blanco added a comment -

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. =(

Show
Guilherme Blanco added a comment - 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. =(
Hide
Permalink
Bernhard Schussek added a comment -

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

Show
Bernhard Schussek added a comment - Yes, I'd expect the first result. Thanks for looking into this, I completely forgot about this issue.
Hide
Permalink
Guilherme Blanco added a comment -

This issue was fixed in: https://github.com/doctrine/common/commit/4210fbd8b261c000c793461c4f815b4d43bcc362

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. =)

Show
Guilherme Blanco added a comment - This issue was fixed in: https://github.com/doctrine/common/commit/4210fbd8b261c000c793461c4f815b4d43bcc362 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. =)

People

  • Assignee:
    Guilherme Blanco
    Reporter:
    Bernhard Schussek
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: