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

CRM-21121 - Fix crash with sql_mode is only_full_group_by #10926

Merged
merged 1 commit into from
Sep 5, 2017

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Aug 31, 2017

Overview

The Event Income Summary report crashes if your sql_mode is only_full_group_by (which is default on MySQL 5.7.5+)

Before

Crash (see JIRA for backtrace)

After

No crash.

Technical Details

I was concerned about simply grouping by the currency - but then I saw that events don't support multiple currencies (though this report DOES support multiple currencies, just not on the same event).

If that were to change in the future, this SQL statement would need to change anyway, so I believe this is safe.

Comments

A tester should ideally have MySQL 5.7.5+ (Ubuntu 16.04+ has it). Testing is relatively simple, because the SQL generated is pretty straightforward.


@eileenmcnaughton
Copy link
Contributor

@lcdservices might be one for you to check?

@lcdservices
Copy link
Contributor

Confirmed -- this fixes the MySQL group by error. @eileenmcnaughton can you merge?

@eileenmcnaughton eileenmcnaughton merged commit e5a69e4 into civicrm:master Sep 5, 2017
@eileenmcnaughton
Copy link
Contributor

Thanks for putting this up / reviewing it.

@MegaphoneJon MegaphoneJon deleted the CRM-21121 branch October 10, 2017 02:23
@JKingsnorth
Copy link
Contributor

This caused a regression in CRM-20170, and I've raised an issue about it here: https://issues.civicrm.org/jira/browse/CRM-21417

@monishdeb
Copy link
Member

@JKingsnorth I have prepared an alternate patch https://gist.github.com/monishdeb/2383e6e9c88b83d3fa96d07f03c97547 which will fix the regression and will take care of CRM-21121. Can you please test my patch?

@monishdeb
Copy link
Member

After discussion with @seamuslee001 on dev-channel, I realised that my earlier patch will cause issue with MariaDB as it won't support ANY_VALUE so here's my second patch https://gist.github.com/monishdeb/2e8c399510765da4539d9866dda88df1 which I believe will fulfill all criteria.

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

Successfully merging this pull request may close these issues.

6 participants