max() takes 2 arguments, yet views is expecting to have only one. This results in an exception, a the result of the math expression always equals 0.

Comments

mrfelton’s picture

Project: Views (for Drupal 7) » Chaos Tool Suite (ctools)
Version: 7.x-3.x-dev » 7.x-1.x-dev

I think this is actually a ctools issue.

rbosscher’s picture

I'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?

merlinofchaos’s picture

Bah. 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.

dydave’s picture

Status: Active » Needs review
StatusFileSize
new2.77 KB

Hi 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:

//$mathexpr = "f(x)=x+2;f(5)";
$mathexpr = "f(x,y)=pow(x,y);f(2,3)";
$mathexpr = "f(x,y,z)=(pow(x,y)*z)/3;f(2,3,10)";
//$mathexpr = "max(4,3)";
//$mathexpr = "3+4";
//$mathexpr = "sqrt(5)";
//$mathexpr = "pow(5,2)";
//$mathexpr = "min(4,3)";

$expressions = explode(';', $mathexpr);
$math = new ctools_math_expr;
foreach ($expressions as $expression) {
if ($expression !== '') {
$value = $math->evaluate($expression);
}
}
dpm("RESULT: ".$value);

I would greatly appreciate your feedback on this, since I would be interested in finding a proper fix.

Thanks in advance for your answers.

Status: Needs review » Needs work

The last submitted patch, math_expr_support_multiple_arguments.patch, failed testing.

dydave’s picture

dydave’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, math_expr_support_multiple_arguments.patch, failed testing.

rbosscher’s picture

Thanks 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';

 if ($fnn == 'max' || $fnn == 'min') $op1 = implode(", ", $stack->stack);								

Before: eval('$stack->push(' . $fnn . '('.$op1.'));'); // perfectly safe eval() (after patching)

Would be very helpfull to get this commited.

robertwb’s picture

Status: Needs work » Needs review
dydave’s picture

Hi 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!

dawehner’s picture

earl proposed a patch in #1958538: Improve math expression engine to fix that.

dydave’s picture

Wow 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!

robertwb’s picture

Issue summary: View changes
Status: Needs review » Closed (fixed)

Marked as closed (fixed) - duplicates https://drupal.org/node/1958538