Doctrine Common
  1. Doctrine Common
  2. DCOM-41

Make annotation parser a bit cleverer

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.0.2
    • Component/s: None
    • Labels:
      None

      Description

      From an initial look that I had at the annotation parser, it simply search for anything starting with an "@" somewhere in the doc comment and assumes that it is an annotation. Now, if someone uses the @ somewhere in his doc comment but not as an annotation, the parser breaks.

      An example for this can be found in the Doctrine code base, the following comment will result in a parse error:

          /**
           * Annotation     ::= "@" AnnotationName ["(" [Values] ")"]
           * AnnotationName ::= QualifiedName | SimpleName | AliasedName
           * QualifiedName  ::= NameSpacePart "\" {NameSpacePart "\"}* SimpleName
           * AliasedName    ::= Alias ":" SimpleName
           * NameSpacePart  ::= identifier
           * SimpleName     ::= identifier
           * Alias          ::= identifier
           *
           * @return mixed False if it is not a valid annotation.
           */
      

      Obviously the first @ is not used to refer to an annotation here. I think it makes sense to allow new annotations only to start at a new line, what do you think?

        Activity

        Hide
        Johannes Schmitt added a comment -
        Show
        Johannes Schmitt added a comment - The pull request is here: https://github.com/doctrine/common/pull/9
        Hide
        Benjamin Eberlei added a comment -

        Does this still work with nested annotations?

        /**
         * @annot({@annot2})
         */
        
        Show
        Benjamin Eberlei added a comment - Does this still work with nested annotations? /** * @annot({@annot2}) */
        Hide
        Johannes Schmitt added a comment -

        Sure, my patch only changes the way how the first @ is found.

        Show
        Johannes Schmitt added a comment - Sure, my patch only changes the way how the first @ is found.
        Hide
        Roman S. Borschel added a comment - - edited

        First of all, thanks for the patch. I'm just not sure whether the added complexity (nontrivial regexp vs substr/strpos) and performance penalty (to be tested) can justify fixing these such edge-cases where an @ appears in the comment block that is not one of the already stripped inline docblocks. Just my two cents, I think this needs further investigation and more extensive testing.

        Edit: Since the regex is "only" applied once per parsed docblock the perf. difference might be negligible but should be tested anyway. Remains the verification of the correctness of the regex, given more extensive testing and test-cases.

        Show
        Roman S. Borschel added a comment - - edited First of all, thanks for the patch. I'm just not sure whether the added complexity (nontrivial regexp vs substr/strpos) and performance penalty (to be tested) can justify fixing these such edge-cases where an @ appears in the comment block that is not one of the already stripped inline docblocks. Just my two cents, I think this needs further investigation and more extensive testing. Edit: Since the regex is "only" applied once per parsed docblock the perf. difference might be negligible but should be tested anyway. Remains the verification of the correctness of the regex, given more extensive testing and test-cases.
        Hide
        Johannes Schmitt added a comment - - edited

        I think performance should not be an issue since this data is cached anyway.

        As for the necessity of this, it's a pain if you parse third party files and they use @ somewhere which then breaks the parser. Since Symfony2 is heavily relying on the annotation parser not only for Doctrine metadata, but for annotation metadata in general, imo this needs to be fixed. I'm not 100% happy with my solution since it covers not all, but only the most likely cases. Ideally, the lexer would have a better way to detect if an @ is used to mark the beginning of an annotation, and simply ignore the @ if it does not. I'm not familar with the lexer code, but you probably have a better understand of how it works and whether this would make sense.

        EDIT: I've made an alternative implementation which you can find here: https://github.com/schmittjoh/common/commit/123315e21aff6d7bce7cb77ab798660d0e68b139

        Show
        Johannes Schmitt added a comment - - edited I think performance should not be an issue since this data is cached anyway. As for the necessity of this, it's a pain if you parse third party files and they use @ somewhere which then breaks the parser. Since Symfony2 is heavily relying on the annotation parser not only for Doctrine metadata, but for annotation metadata in general, imo this needs to be fixed. I'm not 100% happy with my solution since it covers not all, but only the most likely cases. Ideally, the lexer would have a better way to detect if an @ is used to mark the beginning of an annotation, and simply ignore the @ if it does not. I'm not familar with the lexer code, but you probably have a better understand of how it works and whether this would make sense. EDIT: I've made an alternative implementation which you can find here: https://github.com/schmittjoh/common/commit/123315e21aff6d7bce7cb77ab798660d0e68b139
        Hide
        Benjamin Eberlei added a comment -

        Fixed formatting of code block

        Show
        Benjamin Eberlei added a comment - Fixed formatting of code block
        Hide
        Benjamin Eberlei added a comment -

        Fixed

        Show
        Benjamin Eberlei added a comment - Fixed

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Johannes Schmitt
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: