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

[5.2] Fixed issue introduced on reverting previous aggregate functions #15150

Closed
wants to merge 1 commit into from

Conversation

paulfarrow
Copy link

A change to the query builder sum function introduced on revert #14994 presents an issues where the sum function would return values other than of numeric type. It is my understanding that the sum() function should always return a numeric type, as before it used to perform a simple return 0 if result was false.

A change to the query builder sum function introduced issues where the sum function would return values other than of numeric type.
@GrahamCampbell
Copy link
Member

We intentionally reverted this to match the old behaviour. This breaking change will be moved to 5.3 or 5.4 if at all.

@GrahamCampbell GrahamCampbell changed the title Fixed issue introduced on reverting previous aggregate functions [5.2] Fixed issue introduced on reverting previous aggregate functions Aug 30, 2016
@paulfarrow
Copy link
Author

@GrahamCampbell But from what I can tell, the old behaviour for the sum method had a "return $result ?? 0;" before commit 73ee91b on the 12th August, ensuring it always returned a numeric value. This was also stated in the original PHP Doc defined for the sum method, which should return a float|int. This has now gone which breaks functionality with existing code?

@GrahamCampbell
Copy link
Member

Yeh, that change in behaviour was a breaking change that was reverted since it broke stuff.

@paulfarrow
Copy link
Author

Ok, thanks for the info. But just to confirm, the "return $result ?? 0;" was omitted on purpose from the reverted changes, as all the other changes for that commit were reverted back from what I can see with the rest of the code except for that line? Sorry to double check, it's just I want to make sure before I go through and change our project to support non-numeric return values on the sum function. Thanks for you help.

@GrahamCampbell
Copy link
Member

Yes, we intentionally omitted this because the result can be non-numeric, so in those cases summing nothing cannot just default to the additive identity.

@paulfarrow
Copy link
Author

Ok, thanks for confirming that for me.

@paulfarrow
Copy link
Author

@GrahamCampbell Request #15345 seems to have applied the change I pushed in this request, (although in an alternate way). I have already made the changes to my code to work around the changed return type which you said would be staying. Could you please confirm why this functionality has now been put back as I had originally attempted and if it is going to stay this way?

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