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

QueryBuilder::addCriteria improvements #584

Closed

Conversation

chEbba
Copy link
Contributor

@chEbba chEbba commented Feb 19, 2013

  1. Fix problem with different comparisons on the same field in QueryExpressonVisitor (now index value is added).
  2. Add criteria field aliasing. Usually oject criteria has "filed = value" notation while DQL has "alias.field = value".
    First level fields are added with alias, second+ level fields (object.field, parent.object.field) are truncated to the second level (object.field) without alias. Alias map can be implemented in future.

@doctrinebot
Copy link

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-2305

@beberlei
Copy link
Member

I don't like that the visitor gets all the additional code, this is not really his responsibility.

@chEbba
Copy link
Contributor Author

chEbba commented Feb 20, 2013

I don't see a good way to implement this features outside. Parameter and alias handling require visit all expressions.

@alreece45
Copy link

The visitor seems to be at least partially responsible here. All of the expressions need to be visited to add the alias in the WHERE expression. The visitor was already doing this.

However, with this implementation, the visitor gains the now:

  • Tracks aliases
  • Uses/generates the aliases in ordering (it didn't do anything with the orderings before)
  • Uses a more sophisticated method of tracking parameter names.

Those three things can be can be corrected. What would be the acceptable solution here?

  • Shift ordering code outside the visitor? (back to the QueryBuilder)
  • Simplify parameter name generation? (Keep it in the vistor or move it to the QueryBuilder or another Object?)
  • Shift aliasing outside the visitor? (to the the QueryBuilder or some other object?)
  • Something else?

@chEbba
Copy link
Contributor Author

chEbba commented May 4, 2013

This PR can be splitted into 2 parts like described in annotation:

  1. Parameter name generation to fix an issue with several conditaional field expressions. This part should be done inside visitor otherwise it will require a complicated logic to handle parameter placeholders outside.
  2. Aliasing. Aliases are applied to where expression fields and ordering fields. It can be moved outside but QB where expression will have to be revisited.
  3. If just ordering logic is moved to QB than alias name generation will be shared between visitor and QB (as order fields should have same names)

The only way to break the logic i see is to create independent FieldAliasVisitor for Criteria -> Criteria converting
This Visitor can be added to the doctrine-common.

@beberlei What do you think?

@chEbba
Copy link
Contributor Author

chEbba commented Jul 19, 2013

@beberlei Any news? I can remove field alias from this PR and just save the parameter bugfix, which can be applied for 2.4

@mnapoli
Copy link
Contributor

mnapoli commented Sep 17, 2013

ping @beberlei it would be great to either merge this, or find an alternative

Criteria still can't be used on the QueryBuilder in 2.4.

We use it to implement Domain Driven Design, in which a Repository behaves like an in-memory collection. Criterias are supposed to be an abstraction for filtering collections, but right now it's only usable with ArrayCollections, so it kind of defeat the purpose of the abstraction.

@beberlei
Copy link
Member

beberlei commented Jan 3, 2014

I merged a fix for this by into master.

@beberlei beberlei closed this Jan 3, 2014
@beberlei
Copy link
Member

beberlei commented Jan 3, 2014

See 35a62e9

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.

5 participants