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

Remove incorrect (outdated) validation for public fields in SchemaValidator #935

Merged
merged 1 commit into from
Feb 19, 2014

Conversation

da-eto
Copy link
Contributor

@da-eto da-eto commented Feb 6, 2014

Doctrine 2.4 now supports proxy objects for entites with public properties. But SchemaValidator still checks that mapped properties in entities should be private or protected. Can we omit this validation?

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

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

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

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2014

@Da-eto-Ya I didn't try it, but I'm not really sure if this still affects HHVM

@da-eto
Copy link
Contributor Author

da-eto commented Feb 6, 2014

Are you mean this: Ocramius/ProxyManager#127 ?

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2014

@Da-eto-Ya yes, so far I've seen HHVM hang on that hack. I wouldn't remove the notice until we improved that.
The PR can stay open till then

@stof
Copy link
Member

stof commented Feb 6, 2014

maybe we should improve the wording of the message to mention that the issue only affects HHVM now ?

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2014

I will try a run of HHVM with the latest PHPUnit first

@Ocramius
Copy link
Member

Ocramius commented Feb 6, 2014

Created #936 - let's see what travis says...

Ocramius added a commit that referenced this pull request Feb 19, 2014
Remove incorrect (outdated) validation for public fields in SchemaValidator
@Ocramius Ocramius merged commit 5a2497d into doctrine:master Feb 19, 2014
@Ocramius
Copy link
Member

Went through this and it is actually ok.

@Ocramius
Copy link
Member

Note: we still depend on facebook/hhvm#1930 for this

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