Doctrine Common

Make annotation parser a bit cleverer

Details

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

Ascending order - Click to sort in descending order
Hide
Permalink
Johannes Schmitt added a comment -

The pull request is here:
https://github.com/doctrine/common/pull/9

Show
Johannes Schmitt added a comment - The pull request is here: https://github.com/doctrine/common/pull/9
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Benjamin Eberlei added a comment -

Fixed formatting of code block

Show
Benjamin Eberlei added a comment - Fixed formatting of code block
Benjamin Eberlei made changes -
Field Original Value New Value
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? 		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:

{code}
    /**
     * 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.
     */
{code}

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?
Hide
Permalink
Benjamin Eberlei added a comment -

Fixed

Show
Benjamin Eberlei added a comment - Fixed
Benjamin Eberlei made changes -
Status Open [ 1 ] Resolved [ 5 ]
Fix Version/s 2.0.2 [ 10121 ]
Resolution Fixed [ 1 ]
Benjamin Eberlei made changes -
Workflow jira [ 12463 ] jira-feedback2 [ 17888 ]
Benjamin Eberlei made changes -
Workflow jira-feedback2 [ 17888 ] jira-feedback3 [ 19851 ]
This list may be incomplete, as errors occurred whilst retrieving source from linked applications:
  • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DCOM-41, expand=changesets[-21:-1].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

People

  • Assignee:
    Benjamin Eberlei
    Reporter:
    Johannes Schmitt
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: