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

Don't get repository at construction time, it could be not loaded yet! #574

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

fullbl
Copy link

@fullbl fullbl commented Apr 26, 2018

I ran into an issue when trying to load some repositories through compiler pass.
At Doctrine\Bundle\DoctrineBundle\Repository\ContainerRepositoryFactory->getRepository(), it tried to load repository of my User Entity.

I changed the TokenManager, so it doesn't try to get it too soon!

@fullbl
Copy link
Author

fullbl commented Apr 26, 2018

After some more researches, I discovered that could be my installation fault. I'll leave this PR open, since it could be good to remove that call from constructor!

* @param $name
* @return mixed
*/
public function __get($name)
Copy link
Member

Choose a reason for hiding this comment

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

should not be public to provide BC with a protected property.

and it should trigger a deprecation warning

Copy link
Author

Choose a reason for hiding this comment

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

__get can only be public... I can't think a method to provide retrocompatibility for private property.

return $this->getRepository();
}

return $this->$name;
Copy link
Member

Choose a reason for hiding this comment

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

should not allow access. This gives public access to private properties

Copy link
Author

Choose a reason for hiding this comment

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

Removed!

@GuilhemN
Copy link
Member

👍 for me, phpstan is broken however.

@alcaeus alcaeus added this to 1.6 in Roadmap Aug 7, 2019
@alcaeus alcaeus moved this from 1.6 to 2.0 in Roadmap Aug 7, 2019
@alcaeus
Copy link

alcaeus commented Aug 26, 2019

Note: this also applies to other repositories, not just EntityRepositories.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants