Closed (fixed)
Project:
Chaos Tool Suite (ctools)
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
6 Nov 2011 at 13:55 UTC
Updated:
14 May 2014 at 14:00 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
mrfelton commentedI think this is actually a ctools issue.
Comment #2
rbosscher commentedI'm having the same issue.
I excpected the max function to behave like max() PHP function but
it seems like the max math function only accepts 1 argument but then, how can I get the max of two values?
Comment #3
merlinofchaos commentedBah. The math library is hardcoded to believe that all built in functions should have exactly 1 argument. I think we can put it into var $f instead of var $fb though.
Comment #4
dydave commentedHi Guys, I stumbled on pretty much the same issue: on ctools 7.x-1.0
Problem:
to calculate the taxes in a commerce line item view (Drupal commerce), it needs to be on a positive gross margin only:
If a product is sold at loss, then the taxes shouldn't be paid (=0).
Therefore the mathematic expression needed to add a max() for example:
max(round((300 - 342) * 3 * 0.17),0)
max(round(([commerce_unit_price] - [field_offtrade_price]) * [quantity] * 0.17),0)
So I came up with more complex requirements in terms of the formula.
Solution:
I did an attempt to patch the module as you indicated previously and based on other related tickets and came up with the attached patch: math_expr_support_multiple_arguments.patch
The problem was indeed hard coded forced one argument.
I tried to keep the changes to the minimum and you should see the patch is pretty short.
I didn't have the time to write automated tests, so I did a few already in devel/php, below, if you want to try directly:
I would greatly appreciate your feedback on this, since I would be interested in finding a proper fix.
Thanks in advance for your answers.
Comment #6
dydave commentedAnother attempt.
Comment #7
dydave commentedComment #9
rbosscher commentedThanks DYdave for your patch.
I had to patch manually, and after patching I had to add an extra line to make it work with min and max functions:
After: if ($fnn == 'ln') $fnn = 'log';
Before: eval('$stack->push(' . $fnn . '('.$op1.'));'); // perfectly safe eval() (after patching)
Would be very helpfull to get this commited.
Comment #10
robertwb commented#4: math_expr_support_multiple_arguments.patch queued for re-testing.
Comment #11
dydave commentedHi guys,
Thanks a lot for the follow-ups on this ticket.
When I initially submitted the patch in #4, I wasn't necessarily aware of the coding standards in particular for patches.
In any case, please find attached to this comment the patch from #4 re-rolled against ctools-7.x-1.x at b519a8d:
File named: ctools-using-max-in-math-expression-1333424-11.patch.
@rbosscher:
I have tried without the change you mentioned and it seems to be working fine/as expected.
I would greatly appreciate if you could take a little bit of time to take a closer look at this updated patch and let us know if you still encounter any issues (if the patch doesn't behave as you would expect).
Perhaps the code change you suggested is related to a particular testing case that I didn't take into consideration and I would really appreciate if you could let us know about it.
Feel free to let me know if you would have any further questions, comments, issues, concerns, suggestions, recommendations or objections on this updated patch or the ticket in general, I would be glad to provide more information, explain in further details or re-roll the patch if it still needs work.
Any further comments, feedback, testing, reviewing and reporting, would be highly appreciated.
Thanks again to all in advance for your support, comments, replies and feedback.
Cheers!
Comment #12
dawehnerearl proposed a patch in #1958538: Improve math expression engine to fix that.
Comment #13
dydave commentedWow guys! That's really amazing!
We would never have hoped for a complete rewrite of the Math Expressions handling.
The patch here was just really a quick fix and I can't believe you guys rewrote the file entirely (which indeed, needed some attention).
Thank you so much for your contributions, especially coming from @dawehner or @merlinofchaos who probably know better than anyone else the Views and Ctools modules' codes (project's authors/maintainers).
We have started replacing any of the previous patches from this ticket with the ones proposed at #1958538: Improve math expression engine on our projects and testing with Min and Max appears now to be working very well as expected (see one of my colleagues' comments #1958538-6: Improve math expression engine).
Since this was initially the issue in this ticket, I would suggest to update the status and let users know where the work has move to.
In any case, we will continue testing and reporting at #1958538: Improve math expression engine.
Thanks again to everyone for the amazing work on this.
Cheers!
Comment #14
robertwb commentedMarked as closed (fixed) - duplicates https://drupal.org/node/1958538