Doctrine Common
  1. Doctrine Common
  2. DCOM-10

The annotation parser isn't EBNF compliant

    Details

    • Type: Bug Bug
    • Status: 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

        Show
        Bernhard Schussek added a comment - Fixed in http://github.com/bschussek/doctrine-common/tree/DCOM-10
        Hide
        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
        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
        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
        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
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: