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

Always expose virtual properties by default #145

Merged

Conversation

Lumbendil
Copy link
Contributor

Added support for virtual properties when the exclusion policy is set to "all" by making them allways exposed by default.

… to "all" by making them allways exposed by default.
@schmittjoh
Copy link
Owner

There was another PR submitted for this (#144), but this one looks simpler.

Could you add a small test to the base driver test?

@Lumbendil
Copy link
Contributor Author

I'll do it right now (after I understand fully how it's tested.)

Btw, I noticed that after doing

composer install
phpunit

There are two tests that fail. Is this already known?

@schmittjoh
Copy link
Owner

Can you try "composer install --dev", maybe we also need to adjust the
minimum stability after latest composer changes.

On Fri, Jul 6, 2012 at 11:10 AM, Roger Llopart Pla <
[email protected]

wrote:

I'll do it right now (after I understand fully how it's tested.)

Btw, I noticed that after doing

composer install
phpunit

There are two tests that fail. Is this already known?


Reply to this email directly or view it on GitHub:

#145 (comment)

@Lumbendil
Copy link
Contributor Author

Already done that, the output of PHPUnit is the following:

There were 2 errors:

1) JMS\SerializerBundle\Tests\Serializer\JsonSerializationTest::testNestedFormErrors
Argument 2 passed to Symfony\Component\Form\Form::__construct() must implement interface Symfony\Component\EventDispatcher\EventDispatcherInterface, none given, called in /home/lumbendil/projects/fashionweb.local/vendor/bundles/JMS/SerializerBundle/Tests/Serializer/BaseSerializationTest.php on line 355 and defined

/home/lumbendil/projects/fashionweb.local/vendor/bundles/JMS/SerializerBundle/vendor/symfony/form/Symfony/Component/Form/Form.php:189
/home/lumbendil/projects/fashionweb.local/vendor/bundles/JMS/SerializerBundle/Tests/Serializer/BaseSerializationTest.php:355

2) JMS\SerializerBundle\Tests\Serializer\XmlSerializationTest::testNestedFormErrors
Argument 2 passed to Symfony\Component\Form\Form::__construct() must implement interface Symfony\Component\EventDispatcher\EventDispatcherInterface, none given, called in /home/lumbendil/projects/fashionweb.local/vendor/bundles/JMS/SerializerBundle/Tests/Serializer/BaseSerializationTest.php on line 355 and defined

/home/lumbendil/projects/fashionweb.local/vendor/bundles/JMS/SerializerBundle/vendor/symfony/form/Symfony/Component/Form/Form.php:189
/home/lumbendil/projects/fashionweb.local/vendor/bundles/JMS/SerializerBundle/Tests/Serializer/BaseSerializationTest.php:355

I'm working on the tests, I've understood how the fixtures are done, so I'll be finished in a moment.

@Lumbendil
Copy link
Contributor Author

I've managed to implement tests for Annotations YAML and XML, but I'm having issues with the PhpDriver, since it ain't docummented how it works.

I'll commit what i have up to now, It'd be great if you could give me some hints on how to move on from here.

@Lumbendil
Copy link
Contributor Author

Disgread what I said, I've found it out.

@@ -0,0 +1,3 @@
JMS\SerializerBundle\Tests\Fixtures\ObjectWithVirtualPropertiesAndExcludeAll:
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 exclusion_policy: ALL is missing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, fixing it now.

@Lumbendil
Copy link
Contributor Author

Anything else needed, or can this PR be merged?

schmittjoh added a commit that referenced this pull request Jul 6, 2012
…port

Always expose virtual properties by default
@schmittjoh schmittjoh merged commit 6df1922 into schmittjoh:master Jul 6, 2012
@schmittjoh
Copy link
Owner

No, looks good :)

@bamarni bamarni mentioned this pull request Jul 6, 2012
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.

2 participants