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

EntityRepositoryGenerator default repository #1089

Merged

Conversation

encoder32
Copy link
Contributor

  1. I set default repository class in configuration to MyApp\Doctrine\DefaultRepository
  2. I created entity class MyApp\Entity\TestEntity and generated repository class for it
  3. Actually the repository class "MyApp\Entity\TestEntityRepository" turned out to be a descendant of "Doctrine\ORM\EntityRepository" when I expected "MyApp\Doctrine\DefaultRepository" as default.

Related pull doctrine/DoctrineBundle#315

@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-3231

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

* @param string $repositoryName
* @return \Doctrine\ORM\Tools\EntityRepositoryGenerator
*/
public function setDefaultRepositoryName($repositoryName)
Copy link
Member

Choose a reason for hiding this comment

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

This API is not needed, as the default repository name is already specified in configuration

Copy link
Member

Choose a reason for hiding this comment

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

Nvm, was confusing this one with a different generator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I set default repository class in configuration to MyApp\Doctrine\DefaultRepository
  2. I created entity class MyApp\Entity\TestEntity and generated repository class for it
  3. Actually the repository class "MyApp\Entity\TestEntityRepository" turned out to be a descendant of "Doctrine\ORM\EntityRepository" when I expected "MyApp\Doctrine\DefaultRepository" as default.

{
$repositoryName = $this->_repositoryName;

if (substr($repositoryName, 0, 1) != '\\') {
Copy link
Contributor

Choose a reason for hiding this comment

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

!== and you can use $repositoryName[0] instead od substr.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also this should be done only when namespace is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll check it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@encoder32 encoder32 changed the title Entity repository generator default repository EntityRepositoryGenerator default repository Jul 27, 2014
@@ -32,20 +32,20 @@
*/
class EntityRepositoryGenerator
{
protected $_repositoryName = 'Doctrine\ORM\EntityRepository';
Copy link
Member

Choose a reason for hiding this comment

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

please use a private property and remove the leading underscore (we cannot change other properties because of BC)

Copy link

Choose a reason for hiding this comment

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

Why private? What's wrong with protected?

Copy link
Member

Choose a reason for hiding this comment

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

@v3labs we are very defensive about inheritance, and even disallow it in many cases.

Unless there is an explicit (already existing) use-case for the field to be protected, all should be declared as private. Same goes for new methods.

Copy link

Choose a reason for hiding this comment

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

Good to know. I'll keep that in mind for future reference.

@Ocramius
Copy link
Member

Would still need a couple of tests here in order to merge this feature.

*
* @return string
*/
protected function generateClassName($fullClassName)
Copy link
Member

Choose a reason for hiding this comment

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

I would make these private rather than protected (so that we don't need to ensure BC on them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@encoder32
Copy link
Contributor Author

Writed unit tests

@encoder32
Copy link
Contributor Author

Writed tests (orm:generate-repositories command)

*
* @return string $repositoryName
*/
protected function generateEntityRepositoryName($fullClassName)
Copy link
Member

Choose a reason for hiding this comment

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

iMO, it should be private

@encoder32
Copy link
Contributor Author

@Ocramius all done

@Ocramius Ocramius self-assigned this Nov 11, 2014
Ocramius added a commit that referenced this pull request Nov 11, 2014
…ultRepository

EntityRepositoryGenerator default repository
@Ocramius Ocramius merged commit ab62914 into doctrine:master Nov 11, 2014
@Ocramius
Copy link
Member

@encoder32 thanks, merged!

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.

6 participants