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

feat(driver): TypedPropertiesDriver support virtual property getter #1487

Merged

Conversation

mpoiriert
Copy link
Contributor

@mpoiriert mpoiriert commented May 14, 2023

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

This allow to extract return type of virtual property when they are a getter on the class.
Same logic as a type properties but also supporting virtual property getter.

Also support type from php doc for virtual property getter.

@mpoiriert mpoiriert force-pushed the support-typed-virtual-property-getter branch from e0454c5 to e18df85 Compare May 14, 2023 15:12
@mpoiriert mpoiriert force-pushed the support-typed-virtual-property-getter branch 4 times, most recently from 33c3b40 to b19521c Compare May 14, 2023 19:04
@mpoiriert mpoiriert force-pushed the support-typed-virtual-property-getter branch from a43bf4f to c35a532 Compare May 14, 2023 19:15
Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contribution!
Code looks great, I left one small comment. :)

* @return string[]
*/
#[Serializer\VirtualProperty]
public function getArrayOfIntegers(): array
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name seems to be a bit misleading. :) Can we rename it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I changed the type of value while doing it, forgot to change the method name.

Done !

@mpoiriert mpoiriert force-pushed the support-typed-virtual-property-getter branch from c35a532 to 7ff5b64 Compare May 14, 2023 21:21
@mpoiriert
Copy link
Contributor Author

I am using it on a project, I have a issue while removing all Type definition.
I'll check if it's a mistake from my side or an issue with my patch.

I'll put this back to ready to review after.

@mpoiriert mpoiriert marked this pull request as draft May 14, 2023 21:35
@mpoiriert
Copy link
Contributor Author

@scyzoryck It seems that the issue I have is not related to my change.

I am in symfony context, I have use the jms_serializer.metadata.infer_types_from_doc_block = true which is new (didn't use it before).

I get an error from the DocBlockTypeResolver that is trying to parse Doctrine\\Common\\Collections\\AbstractLazyCollection:collection

    "code": 500,
    "message": "Can't use non-array generic type Collection for collection in Doctrine\\Common\\Collections\\AbstractLazyCollection:collection",
    "detail": {
        "class": "InvalidArgumentException",
        "message": "Can't use non-array generic type Collection for collection in Doctrine\\Common\\Collections\\AbstractLazyCollection:collection",
        "code": 0,
        "file": "/home/wwwroot/vendor/jms/serializer/src/Metadata/Driver/DocBlockDriver/DocBlockTypeResolver.php",
        "line": 108,
        "stack": [
            "#0 /home/wwwroot/vendor/jms/serializer/src/Metadata/Driver/DocBlockDriver.php(64): JMS\\Serializer\\Metadata\\Driver\\DocBlockDriver\\DocBlockTypeResolver->getPropertyDocblockTypeHint(Object(ReflectionProperty))",
            "#1 /home/wwwroot/vendor/jms/metadata/src/Driver/LazyLoadingDriver.php(38): JMS\\Serializer\\Metadata\\Driver\\DocBlockDriver->loadMetadataForClass(Object(ReflectionClass))",
            "#2 /home/wwwroot/vendor/jms/metadata/src/MetadataFactory.php(111): Metadata\\Driver\\LazyLoadingDriver->loadMetadataForClass(Object(ReflectionClass))",
            "#3 /home/wwwroot/vendor/jms/serializer-bundle/Debug/TraceableMetadataFactory.php(37): Metadata\\MetadataFactory->getMetadataForClass('Doctrine\\\\ORM\\\\Pe...')",
            "#4 /home/wwwroot/vendor/jms/serializer/src/Handler/ArrayCollectionHandler.php(89): JMS\\SerializerBundle\\Debug\\TraceableMetadataFactory->getMetadataForClass('Doctrine\\\\ORM\\\\Pe...')",
            ```
            
            Called from the ArrayCollecitonHandler exclusion strategy. 
        if (false === $this->initializeExcluded) {
        $exclusionStrategy = $context->getExclusionStrategy();
        if (null !== $exclusionStrategy && $exclusionStrategy->shouldSkipClass($context->getMetadataFactory()->getMetadataForClass(\get_class($collection)), $context)) {
            $context->startVisiting($collection);

            return $visitor->visitArray([], $type);
        }
    }

I don't see exactly when we would check 'should skip class' on the collection itself ? Since we are not returning the collection but rather the object inside of it as an array.

I have check all the related code and everything is old, I have the feeling that not a lot (AKA no one) is using the DocBlockDriver with Doctrine. If I put the 'initialize_excluded' = true (which default to false) everything works.

Anyway, this is not related to the my pr so everything is fine.
 
            
            

@mpoiriert mpoiriert marked this pull request as ready for review May 14, 2023 22:17
@goetas goetas merged commit d5cc467 into schmittjoh:master May 18, 2023
@goetas
Copy link
Collaborator

goetas commented May 18, 2023

Thank you

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

Successfully merging this pull request may close these issues.

3 participants