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

readonly flag should be set before setter, because the setter depends on that being set #154

Merged
merged 3 commits into from
Sep 20, 2012

Conversation

mvrhov
Copy link
Contributor

@mvrhov mvrhov commented Jul 30, 2012

No description provided.

@@ -166,6 +166,11 @@ protected function loadMetadataFromFile(\ReflectionClass $class, $path)
$pMetadata->xmlKeyValuePairs = 'true' === (string) $pElem->attributes()->{'xml-key-value-pairs'};
}

//we need read-only before setter and getter set, because that method depends on flag being set
if (null !== $readOnly = $pElem->attributes()->{'read-only'}) {
//$pMetadata->readOnly = 'true' === strtolower($readOnly);
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this line commented? There seems to be something wrong here. Could you add a test which covers this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aw. i was testing.... and it seems that I left it commented

@mvrhov
Copy link
Contributor Author

mvrhov commented Jul 31, 2012

@schmittjoh I have no idea on how to test, that the above is executed before setter and getter methods are called.
The only thing to test that comes to mind is to change the getSerializer function in BaseSerializationTest to use a different driver and then serialize object and there should be no exception.

@schmittjoh
Copy link
Owner

Looking at it, the AnnotationDriver seems fine as we always call setAccessor after setting readOnly. So, only the XML and YAML driver are affected.

I'd suggest to add a simple config which has a read-only property and only a getter configured to the BaseDriver test. Currently, this should throw an exception. After your patch, it should not throw one.

@mvrhov
Copy link
Contributor Author

mvrhov commented Aug 1, 2012

done

@travisbot
Copy link

This pull request passes (merged c9f0407 into 6bd1105).

@@ -159,14 +159,15 @@ public function loadMetadataForClass(\ReflectionClass $class)
$propertyMetadata->xmlValue = true;
} else if ($annot instanceof AccessType) {
$AccessType = $annot->type;
//we need ReadOnly before setter and getter set, because that method depends on flag being set
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment should be removed. Moving it here does not ensure anything about the order in which the annotations are found

@schmittjoh schmittjoh merged commit c9f0407 into schmittjoh:master Sep 20, 2012
jorns pushed a commit to jorns/JMSSerializerBundle that referenced this pull request Aug 11, 2014
…erialization

Fix XML null DateTime deserialization
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.

4 participants