As described in the hijack of #1054168-2: EntityFieldQuery fails for entities that have no bundle if you have something like

SELECT column + 1 AS calculated_column
HAVING calculated_column > 1

convert this into DBTNG and make it a countQuery then it removes the column + 1 AS calculated_column expression and replaces with COUNT(*) which fails the HAVING.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review
FileSize
847 bytes

Test, fails.

bfroehle’s picture

FileSize
2.3 KB

Is this sort of approach what we are looking for?

chx’s picture

Status: Needs review » Needs work

Thanks for jumping on the issue so quick! HAVING foo = 1 AND ( bar = 1 OR bar > 3) will break (Edit: for bar). I think you need to refactor the having handling to collect fields. Edit: and even that might not be enough because if you have several levels of DatabaseConditions then havingCondition will only see the top ones. :/

chx’s picture

Status: Needs work » Needs review
FileSize
4.91 KB

I needed to merge #1196292: Allow Pager count queries to utilise GROUP BY into this one to make the test pass. We dont remove any expression if there is HAVING.

Damien Tournoud’s picture

I am not sure the original query is valid in the first place. In strict SQL, WHERE and GROUP BY can only contain references to columns, not to aliases. It is likely the case for HAVING too.

chx’s picture

Status: Needs review » Needs work

Sure. The having methods fixes are still valid even if the countQuery isn't.

chx’s picture

Status: Needs work » Needs review
FileSize
4.79 KB

I studied http://www.postgresql.org/docs/8.1/static/tutorial-agg.html and http://www.postgresql.org/docs/9.0/static/sql-select.html#SQL-HAVING and I think that you can run HAVING on an expression provided the column (or the expression itself?? MySQL and PostgreSQL seems to disagree on that) is selected.

xjm’s picture

Tracking.

catch’s picture

Issue tags: +Needs backport to D7

Tagging for backport.

xjm’s picture

Rerolled for core/.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Looking at the patch itself, it's a major "duh". :-) (chx talked to me about this a while back; it just fell off my radar.)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks great, don't see anything to complain about. Committed to 8.x, moving back to 7.x for which this will need a re-roll.

xjm’s picture

Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
4.79 KB

Nothing particular to D8 so it's a straight reroll.

Dries’s picture

Committed to 7.x. Thanks!

aspilicious’s picture

Status: Reviewed & tested by the community » Fixed

:)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

changed to #1054168-2 instead of #1054168:2