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

Fix queries in ONLY_FULL_GROUP_BY mode #1138

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

radarhere
Copy link
Contributor

Addresses #729, building on #1129

I've searched through Jethro for any query containing 'GROUP BY' and run each of them. I found three that caused an error when ONLY_FULL_GROUP_BY mode was enabled. This addresses them, removing the requirement to disable ONLY_FULL_GROUP_BY.

I understand it has been said that this is not a priority, but thought I would offer this anyway. It is possible less queries are involved than was suspected.

Copy link
Contributor

@jefft jefft left a comment

Choose a reason for hiding this comment

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

I like this patch, as it fixes some daftness in the old code (using MySQL to calculate a date offset and then returning it for every person record). It doesn't appear to have anything to do with ONLY_FULL_GROUP_BY though?

The PHP-calculated date should be the same as MySQL-calculated, since the timezone is set the same:

if (defined('TIMEZONE') && constant('TIMEZONE')) {
date_default_timezone_set(constant('TIMEZONE'));
$res = $GLOBALS['db']->query('SET time_zone = "'.date('P').'"');

It works in practice:

$ mysql -sNBe "select curdate()  + interval 7 day;"
2025-01-27
$ php -r '$ini = ["REMINDER_OFFSET" => 7]; echo $expiryDate = date("Y-m-d", strtotime($ini["REMINDER_OFFSET"] . " day")) . PHP_EOL;'
2025-01-27

@radarhere
Copy link
Contributor Author

The error generated in master by date_reminder.php is

Syntax error or access violation: 1055 Expression #21 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'jethro.cfv.value_date' which is not functionally dependent on columns in GROUP BY clause

My change removes 'cfv.value_date' altogether.

If you like that part of this PR for non-GROUP BY reasons, I'm perfectly happy to create a separate PR with just that.

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.

2 participants