Added ability to specify INDEX BY when creating a QueryBuilder from a Repository #592

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@cmenning

This is a handy shortcut when using indexed results.

@doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DDC-2322

@beberlei
Member

I think this is enough to be available through $entityManager->createQueryBuilder()->from()

@beberlei beberlei closed this Mar 12, 2013
@cmenning

In DoctrineBundle, however,

getDoctrine()->getEntityManager()

has been deprecated, which leaves you to use

getDoctrine()->getRepository($class)->createQueryBuilder();

Adding ->from() at that point is redundant since it's already been specified inside the createQueryBuilder() call. It seems wasteful to not allow a passthru of the $indexBy argument during the initial call when it's already supported by the from() called inside createQueryBuilder().

@jrobeson

i agree with @cmenning : any way you can reconsider @beberlei?

@jrobeson

i was going to submit the exact same feature request btw. but @cmenning actually did a PR.

@Ocramius
Member

What's the problem in doing following?

$qb = $em->createQueryBuilder()->select('e')->from($repo->getClassName(), 'e', 'e.stuff')?

The repository already does more than needed (imo)

@cmenning

I don't think it's out of scope for the Repository, which by definition in the Doctrine docs "provides many ways to retrieve entities of the specified type". It makes sense to allow it to specify the index for the results.

In a Symfony controller, for example, these are functionally equivalent:

$this->getDoctrine()
    ->getManager()
    ->createQueryBuilder()
    ->select('e')
    ->from($repo->getClassName(), 'e', 'e.stuff');

and

$this->getDoctrine()
    ->getRepository($entityClass)
    ->createQueryBuilder('e', 'e.stuff');

This has the following benefits:

  • More concise
  • Doesn't require as much manual query building
  • In line with very common ways of getting objects from the database ($repo->find($criteria))
  • Is more logical from an object-oriented point of view: you start with a collection of objects (the Repository) and not a database connection (the EntityManager)
@intel352

Agreed with @cmenning, this PR ought to be reconsidered @beberlei

@laytoneverson

@beberlei: This really would be a helpful change and I don't believe it's out of scope at all, in fact I wonder how it was overlooked in the first place, you would think a shortcut function would except the extra, simple parameters?

@Ocramius
Member

by definition in the Doctrine docs "provides many ways to retrieve entities of the specified type".

No, that's not what a repository IS. A repository is just a queriable set of entities.

Cluttering it with more public API methods is just going to spawn more bugs and making changes harder, as well as breaking inheritance.

Simply use the method suggested above: it works just fine.

@laytoneverson

We have been forced to.

@cmenning
cmenning commented Jul 1, 2014

@Ocramius

"Cluttering it with more public API methods"

This isn't adding more public API methods, it's adding an optional argument to an existing API method that's passed unchanged to a function call that already exists and already accepts the extra argument.

@Ocramius
Member
Ocramius commented Jul 1, 2014

Adding an additional optional method IS cluttering the public API (yes, my previous wording was wrong).

Adding constants, protected properties, protected methods, public methods, public/protected method parameters, those are all BC breaks and sources of future BC breaks (and it's not an exhaustive list).

Use the already existing API or a custom repository method, which is much simpler and cleaner.

@Ocramius Ocramius self-assigned this Jul 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment