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

fixed file property accessibility #1836

Closed
wants to merge 1 commit into from
Closed

fixed file property accessibility #1836

wants to merge 1 commit into from

Conversation

philipphoffmann
Copy link
Contributor

The file property should be private and there should be a setter for the property (so the form can save it).
Otherwise doctrine complains about the public property: Field 'file' in class 'Acme\DemoBundle\Entity\Document' must be private or protected. Public fields may break lazy-loading.

The file property should be private and there should be a setter for the property (so the form can save it).
Otherwise doctrine complains about the public property: Field 'file' in class 'Acme\DemoBundle\Entity\Document' must be private or protected. Public fields may break lazy-loading.
@stof
Copy link
Member

stof commented Oct 20, 2012

This is not needed here as this property is not mapped by Doctrine.

@philipphoffmann
Copy link
Contributor Author

Yeah I know, but Doctrine is complaining about the fact that the property is public. So should this be fixed in Doctrine or in the example?

@stof
Copy link
Member

stof commented Oct 20, 2012

I think this is a bug in the Doctrine tool validating this.

@yahuarkuntur
Copy link

Any updates on this issue?

@weaverryan
Copy link
Member

I'm going to close this PR for now for 2 reasons:

  1. This entire document is going to be heavily updated (see Refactor the Uploading Document #2346).
  2. When we do that, I think we should use protected/private properties anyways since it's a better practice (even if it makes the docs a bit longer). So, that should fix this one way or another.

Even if the bug is in Doctrine, we should code to make sure things work. But the rewrite of this section will take care of this - there are other things (see the above issue) that I dislike about this entry :).

Thanks!

@weaverryan weaverryan closed this Mar 30, 2013
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