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

Add @Virtual annotation #109

Merged
merged 9 commits into from Jun 8, 2012
Merged

Conversation

anyx
Copy link
Contributor

@anyx anyx commented Apr 29, 2012

@virtual - method annotation, which allow to add calculated fields to serialized object.

For exampe object of class:

namespace JMS\SerializerBundle\Tests\Fixtures;

use JMS\SerializerBundle\Annotation\SerializedName;
use JMS\SerializerBundle\Annotation\Virtual;

class ObjectWithVirtualProperty
{

    protected $existField = 'value';

    /**
     *
     * @Virtual(field="foo")
     */
    public function getValue() {
        return 'bar';
    }
}

to be serialized as: '{"foo":"bar","exist_field":"value"}'

@schmittjoh
Copy link
Owner

If we add something like this, then it needs to be in the GraphNavigator instead of the different visitors.

Currently, this also bypasses all exclusion strategies that have been set. To solve this, you probably need to create a sub-class of PropertyMetadata, sth like VirtualPropertyMetadata.

But in general, I think this would be indeed useful.

@michelsalib
Copy link
Contributor

I don't like the idea of Virtual. Why not using Expose ?

@schmittjoh
Copy link
Owner

This is mostly useful for data that does not exist on a property, but for example is the combination of properties, or calculated in some other fashion.

<?php

class User
{
    private $firstname;
    private $lastname;

    /** @Virtual("fullname") */
    public function getFullname()
    {
        return $this->firstname.' '.$this->lastname;
    }
}

@michelsalib
Copy link
Contributor

@schmittjoh I already got the point of this annotation. It makes perfect sense. I am jut not found of the name.

JABX allows to use XMLElement on method, which is our equivalent of Expose (I cannot check at the moment because java website is down). My point is what prevents us to use the Expose annotation on methods ?

@anyx
Copy link
Contributor Author

anyx commented Apr 30, 2012

@schmittjoh, i removed code from visitors and created VirtualPropertyMetadata as you said. As for annotation name, i prefer Virtual to Expose. If current realization is acceptable, we can discuss annotation name.

@michelsalib
Copy link
Contributor

I won't argue too much, I will just follow @schmittjoh decision.

@schmittjoh
Copy link
Owner

I think we need to remove the "field" attribute from the @virtual annotation (and instead use some transformation of the method name). Regarding @expose, I don't think it's wise to re-use this as it used in another sense already, and would have a different behavior depending on whether it is declared on properties, or on methods. Maybe we can be more explicit, and call it @VirtualProperty?

Also, all the other annotations like @SerializedName, @groups, @expose, @exclude, @XmlAttribute, @XmlValue, etc. should be available for methods as well. @SerializedName would replace the "field" attribute that is currently on the @virtual annotation.

There are also a few CS issues in your code, mostly indentation, and whitespace, could you check them as well?

Another thought, does it make sense to provide a setter for virtual properties?

@michelsalib
Copy link
Contributor

@schmittjoh I agree with you.

Making this available for setter is IMO a bad idea. The developer will have to be very (too much) careful about its use, and the consumer will definitely be confused about which property to use (the normal or the virtual one).

@anyx
Copy link
Contributor Author

anyx commented May 1, 2012

I renamed annotation into VirtualProperty and modified AnnotationDriver: now other property annotations work.

* @Required
* @var string
*/
public $field;
Copy link
Owner

Choose a reason for hiding this comment

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

What about removing this? It's redundant with @SerializedName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but if we remove this, VirtualProperty annotation will work only together with SerializedName. If you sure, that this variant is better, i can try remove this.

Copy link
Owner

Choose a reason for hiding this comment

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

I think we could use some smart default based on the method name.

if ('get' === substr($method, 0, 3)) {
    $name = lcfirst(substr($method, 3));
} else {
    $name = $method;
}

@schmittjoh
Copy link
Owner

The implementation looks better now.

Would you mind also updating the Yaml, and XML drivers?

@anyx
Copy link
Contributor Author

anyx commented May 3, 2012

I removed $field property from annotation and implement same logic for other drivers. Now we have little copy-pasted code in metadata drivers. Maybe create abstract metadata driver?
Also "SerializedName" isn't involved in sorting properties (i mean "AccessorOrder").

@schmittjoh
Copy link
Owner

Thanks for your efforts.

I think you can inline the code which is responsible for determining the virtual property name into the constructor of VirtualPropertyMetadata, and simply pass ReflectionMethod instead of $name as argument.

Regarding serialized name, I don't think we can change this without incurring additional runtime overhead. What you can do however, is define a custom property order which respects the serialized name for your class.

@anyx
Copy link
Contributor Author

anyx commented May 4, 2012

I have changed constructor VirtualPropertyMetadata. Now it accepts method name in second argument. In the constructor we can determine $reflection property as new \ReflectionMethod, but i did not do this. Add this?

@ruudk
Copy link
Contributor

ruudk commented May 10, 2012

Would it be possible to also allow this for deserializing?

I want to give the user the possibility to send an extra variable called where he can set some extra settings when creating new resources. I don't want this to end up in the serialized state of the object. It is only available for deserialized objects.

@anyx
Copy link
Contributor Author

anyx commented May 10, 2012

@ruudk, i don't understand exactly what you mean. Why is "PostDeserialize" annotation not suitable? Can you give example of usage?

@anyx
Copy link
Contributor Author

anyx commented May 12, 2012

@schmittjoh, did you see last commits? Is current state acceptable?

@schmittjoh
Copy link
Owner

Apart from a few CS issues, it looks good.

Hope that I'll be able to test it a bit more thoroughly tomorrow.

@Spea
Copy link
Contributor

Spea commented May 31, 2012

@schmittjoh Any updates when this will/can be merged?

@schmittjoh
Copy link
Owner

Probably during sflive.

On Thu, May 31, 2012 at 8:06 AM, Martin Parsiegla <
reply@reply.github.com

wrote:

@schmittjoh Any updates when this will/can be merged?


Reply to this email directly or view it on GitHub:

#109 (comment)

@Spea
Copy link
Contributor

Spea commented May 31, 2012

Ok, thank you for the update.

@schmittjoh schmittjoh merged commit 3af5c86 into schmittjoh:master Jun 8, 2012
@schmittjoh
Copy link
Owner

Hey, thanks for your patience.

I had to make a few changes to bring it up to speed with master, and also a few minor CS fixes, but apart from that I merged it completely.

@anyx
Copy link
Contributor Author

anyx commented Jun 8, 2012

Cool, thanks

@slaur
Copy link

slaur commented Sep 12, 2012

This is awesome, thanks @anyx and @schmittjoh

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.

None yet

6 participants