Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DocBlockDriver unable to parse single-line DocBlocks #1259

Closed
Namoshek opened this issue Oct 30, 2020 · 5 comments
Closed

DocBlockDriver unable to parse single-line DocBlocks #1259

Namoshek opened this issue Oct 30, 2020 · 5 comments

Comments

@Namoshek
Copy link
Contributor

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no

Steps required to reproduce the problem

  1. Use the new $builder->setDocBlockTypeResolver(true) feature from Resolve collections from DocBlock #1214.

  2. Define single-line DocBlocks to save some space:

    class Foo
    {
        /** @var Bar[] */
        public array $x;
    
        /** @var \Some\Namespace\Bar[] */
        public array $y;
     }

Expected Result

Parses successfully.

Actual Result

Throws exception(s):

Case Bar[]:

InvalidArgumentException: Can't use incorrect type Bar[] */ for collection in Some\Namespace\Foo:x

Case \Some\Namespace\Bar[]:

JMS\Serializer\Type\Exception\SyntaxError: Syntax error, unexpected "*/" (T_UNKNOWN), expected was T_COMMA

Additional Information

The same code with multi-line DocBlocks parses and de-/serializes successfully:

class Foo
{
    /**
     * @var Bar[]
     */
    public array $x;

    /**
     * @var \Some\Namespace\Bar[]
     */
    public array $y;
}

Interestingly enough, using the array<Class> syntax also works with single-line DocBlocks:

class Foo
{
    /** @var array<Bar> */
    public array $x;

    /** @var array<\Some\Namespace\Bar> */
    public array $y;
}
@Namoshek
Copy link
Contributor Author

Thank you for this cool addition by the way! Combining this with attributes and named parameters from PHP 8 will be my absolute favorite way to go in future. My vision would be something like this (some random modifier ideas thrown into the mix as well 😄):

class Foo
{
    /** @var Bar[] */
    #[Serializer\Property(
        name: 'theBars',
        nullIfEmpty: true,              // for deserialization of strings and arrays
        nullIfWhitespace: true,         // for deserialization of strings
        ignoreIfNull: true,             // for serialization
        ignoreIfEmpty: true,            // for serialization of strings and arrays
        ignoreIfWhitespace: true,       // for serialization of strings
        ignoreIfNullOrWhitespace: true, // for serialization of strings
    )]
    public ?array $bars;
}

Note: not all added parameters make sense with an array of objects... it's just to illustrate.

@Namoshek
Copy link
Contributor Author

I had a look at the regex which is used to retrieve the type from the DocBlock. The regex is currently #@var[\s]*([^\n\$]*)#. I think by changing it to #@var[\s]*([^\n\$\*\/]*)# (\*\/ added to the exclude clause), the problem should be resolved. As far as I know, neither / nor * may appear after @var, so this should be fine. If anyone can confirm this, I'm happy to provide a PR for it.

@goetas
Copy link
Collaborator

goetas commented Oct 31, 2020

IMO the best thing to do here is to use https://github.com/phpstan/phpdoc-parser to parse the docblock. Is anyone willing ro provide a PR for it?

@Namoshek
Copy link
Contributor Author

Sure, I can give it a shot. Some questions in advance though:

  1. Is it ok to add a constructor to the DocBlockTypeResolver (to instantiate the PhpDocParser)? Currently, it has none, which makes it theoretically a breaking change if someone extended the resolver and added their own constructor...

  2. Which syntaxes are supposed to be allowed for this feature anyway?
    In my opinion, the following SHOULD be supported (using single-line for brevity, of course multi-line should work as well):

    /** @var string[] */
    public array $strings;
    
    /** @var string[]|null */
    public ?array $strings;
    
    /** @var Product[] */
    public array $products;
    
    /** @var array<Product> */
    public array $products;
    
    /** @var Product[]|null */
    public ?array $products;
    
    /** @var array<Product>|null */
    public ?array $products;
    
    /** @var array<int,Product>|null */
    public ?array $products;
    
    /** @var array<string,Product>|null */
    public ?array $products;

    These variants CANNOT be supported (at least I have no idea how it would be possible):

    /**
     * @var array<Foo,Bar>
     */
    public array $bars;
    
    /**
     * @var array<Foo,Bar>|null
     */
    public ?array $bars;

    Although they might be handled correctly by the resolver, just not by the de-/serializer itself.

@Namoshek
Copy link
Contributor Author

Cleaning up my issues and realized this never has been closed even though it has been fixed. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants