Resolve collections from DocBlock

[dgafka]

Q	A
Bug fix?	no
New feature?	yes
Doc updated	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1208
License	MIT

There are few things worth discussion:

1. What should happen, when incorrect DocBlock type is given?

   /**
   * @var \stdClass
   */
   public array $productIds;
   

Current implementation will throw exception, as above has no sense

2. What should happen, when union type is given?

    /**
    * @var ProductName[]|ProductDescription[]
    */
   public array $productIds;
   

Current implementation will throw exception, as we will not know, which class should be used for deserialization.

3. What should happen, when class does not exists?

    /**
    * @var NotExistingClass[]
    */
   public array $productIds;
   

Current implementation will throw exception as, this class will not be deserializable.

4. If exception is thrown in any case of 1) and 2) 3) what exception type it should be?

Current implementation will throw \InvalidArgumentException

[dragosprotung]

Any chance to support also this format:

/**
 * @var array<ProductName>
 */
public array $productIds;

[dgafka]

@dragosprotung yes, I can add it :)

@goetas can you deliver feedback about current implementation and answer above questions?
So I can know, if current behaviour is expected and continue with possible fixes. :)

[goetas]

I really like how this feature is progressing.

I did not review possible edge cases in the comment parsing, that would be done when we agree on the architecture of this feature.

In particular im interested in the feedback about implementing this feature as a new driver and implementing DriverInterface

[dgafka]

@goetas Proposal of @dragosprotung is implemented.
This will be possible:

 /**
 * @var array<Product>
 */
private array $products;

To keep this compatible with JMS Serializer documentation this will also be possible:

 /**
 * @var array<string, Product>
 */
private array $products;

[dgafka]

@goetas I would like to ask for review now.
There are 4 failing tests left, I am not able to fix them, as I don't understand why they fail. Can you check them out?

[goetas]

This looks great so far! I will try it in the next days/weeks and see if I can help with the failing tests

[dgafka]

@goetas will you find time to check the tests? :)

[dgafka]

@goetas can you tell, if you will find time for verifying tests?

I understand that you may have not enough time for it.
So please tell me, if you will not find the time, then I will try to fix it on my own :)

[goetas]

I have tested this and looks great, but is seems that needs to be targeting a 4.0 release...

the issue is the following code that works as expected now, and is broken if we were going to merge this PR.

class User {
     /**
     * @var Country
     */
     private $country;

     public function __construct($country) 
     {
         $this->country = $country;
     }
}

$user = new User(new Car()); // yes we are passing a "car", not a country, it is not a typo

Even if the following code is "wrong", it works. A user will be serialized and in the "country" there will be serialized the car object.

If this PR gets merged, the serializer will try to traverse the "car" by using the props defined for "country".

This is not an uncommon scenario... immagine just how many docblocks might be not up to date in a project.... and now suddenly applications will start crashing.

With typed props this was not an issue is since PHP will do the check for us and tell the user that the code was wrong... but in our case there is no such check.

[dgafka]

This is understandable, you would like to avoid breaking change for current major release.

Should I leave the MR to you now and wait for 4.0 release? :)

[goetas]

Should I leave the MR to you now and wait for 4.0 release? :)

@dgafka Sadly I have to say yes. The 4.0 is not yet planned but there are a couple of things I would like to place into it (but have no time to work on it....).

The only option I can see to have it in 3.x to make it opt-in, so who is interested can enable it and deal with potential bc breaks? What do you think? (i think it could be possible just by removing the instantiation of it from https://github.com/schmittjoh/serializer/pull/1214/files#diff-31d5d899bd07e7ac0794ccc1b0facdeaR64 and move it to the serializer builder).

Thanks a lot again for your great work!

[dgafka]

@goetas I have added DocBlock Type Resolver as opt in feature, which by default is disabled. :)

You can enable it by using method setDocBlockTypeResolver on SerializerBuilder

builder->setDocBlockTypeResolver(true)

[dgafka]

@goetas will we make it to 3.x after recent change? :)

[goetas]

hi! sorry if it took that long, but I think that this is ready!

I was trying to use https://github.com/phpstan/phpdoc-parser to parse the docblocks, but I did not make it, will try again!

Thanks a lot for your work!

[dgafka]

If I understand you were trying to replace the part which resolves the docblock types for an external library.
Plus of the current solution is less dependencies on the JMS\Serializer :)

Thank you for merging it :)

I am looking forward for next MR, in order to optimize deserializing process. I feel like version 3, had big impact on that as the times increased.
However that's a plan for a future, as I will need to understand the library deeper :)

[goetas]

If I understand you were trying to replace the part which resolves the docblock types for an external library.
Plus of the current solution is less dependencies on the JMS\Serializer :)

yeah. the advantage of the other lib is that is fully tested and support very exotic syntaxes to express types (see PHPStan)...

I am looking forward for next MR, in order to optimize deserializing process.

Looking forward to it.

I feel like version 3, had big impact on that as the times increased.

did not know that... IMO should be faster than 1.x... but I might be wrong.

[goetas]

However that's a plan for a future, as I will need to understand the library deeper :)

thanks again for this contribution

[simPod]

@dgafka how to enable DocBlockDriver?

[dgafka]

@simPod you can see example usage in here:
901f072#diff-9739e997b0a9ef5ef5a7b3304d94d26ba594544fd4d8604c9f0f43262fd40d55R269

[simPod]

I see. I'll have to do it differently if I use the sf bundle. Thanks.