Skip to content

[2.4] Skip public property validation if no reflection class instance present#1237

Closed
deeky666 wants to merge 1 commit intodoctrine:2.4from
deeky666:fix-public-property-schema-validation
Closed

[2.4] Skip public property validation if no reflection class instance present#1237
deeky666 wants to merge 1 commit intodoctrine:2.4from
deeky666:fix-public-property-schema-validation

Conversation

@deeky666
Copy link
Member

@deeky666 deeky666 commented Jan 2, 2015

The schema validator triggers a fatal error if you try to validate a class metadata that does not have the ClassMetadataInfo::$reflClass set.
This currently breaks the DoctrineBundle test suite. Therefore it's best to skip that validation part if the property is not set.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. However did not open it on the "master"
branch. Our Git workflow requires all pull requests to go through "master" branch
and the release masters then merge them back into stable branches, if they are
bug fixes.

Please open the pull request again for the "master" branch and close
this one.

Nevertheless I have opened a Jira ticket for this Pull Request to track this
issue:

http://www.doctrine-project.org/jira/browse/DDC-3474

We use Jira to track the state of pull requests and the versions they got
included in.

@deeky666
Copy link
Member Author

deeky666 commented Jan 2, 2015

See the discussion here: doctrine/DoctrineBundle#352

@Ocramius
Copy link
Member

Ocramius commented Jan 2, 2015

This looks invalid to me. Metadata should always be processed by the reflection service before being used. See https://github.com/doctrine/doctrine2/blob/52376929795d7537f725409c1150d1b76ecc32a0/lib/Doctrine/ORM/Mapping/ClassMetadataFactory.php#L643-L659

@deeky666
Copy link
Member Author

deeky666 commented Jan 2, 2015

The tests that cause the error in DoctrineBundle are indeed invalid concerning that. But does it hurt to prevent the fatals if you pass an invalid ClassMetadataInfo instance?

@deeky666
Copy link
Member Author

deeky666 commented Jan 2, 2015

@Ocramius
Copy link
Member

Ocramius commented Jan 2, 2015

But does it hurt to prevent the fatals if you pass an invalid ClassMetadataInfo instance?

ClassMetadataInfo behaves like that because it is highly optimized for serialization/unserialization. I realize that there is some lack of encapsulation, but that's a tradeoff.

The solution is to pass class metadata info instance through the static reflection service before using it.

@deeky666
Copy link
Member Author

deeky666 commented Jan 2, 2015

I totally get that. It's just hard to mock that stuff then I suppose. If this one is not acceptable then I'm fine with it. Might loose some mocking capabilities in DoctrineBundle then I suppose.

@Ocramius
Copy link
Member

Ocramius commented Jan 2, 2015

It's all in doctrine/common anyway as far as I know.

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