Problem/Motivation

The math expression engine has several faults:

1) max() and min() don't work because functions can have only one argument.
2) There is no support for strings.
3) There is no support for if() or equality operators.

This significant retool improves this engine quite a bit, adding a simple if(), basic equality operators, fixing max() and min(), and adding support for strings as long as they're quoted. It also improves code style a little bit, though maybe not vastly, because I just used PHPStorm's re-format.

Proposed resolution

Create patch.

Create tests (Added as issue tag): see #2830393: Improve test coverage and function docs: math_expression.

Remaining tasks

#67
I noticed another bug, too: strtolower() is inappropriate. Added a TODO to the growing list. I would appreciate feedback on these:

- TODO: Is this supposed to remove all constants? we should remove all
- TODO: Because the expr can contain string operands, using strtolower here is a bug.
- TODO: A really bad idea: It might be good for those using the 12,345.67

User interface changes

None.

API changes

None.

Data model changes

None.

Files: 
CommentFileSizeAuthor
#72 ctools-n1958538-72.patch57.69 KBrivimey
#72 1958538-72-interdiff.txt1.31 KBrivimey
#64 ctools-n1958538-64.patch58.42 KBrivimey
#64 1958538-63-64-interdiff.txt3.83 KBrivimey
#64 1958538-61-63-interdiff.txt19.01 KBrivimey
#26 interdiff.txt1.75 KBrobertwb
#23 interdiff.txt1.55 KBrobertwb
#21 drupal-1958538-7.patch46.52 KBrobertwb
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es). View
#19 drupal-1958538-6.patch46.52 KBrobertwb
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 1 fail(s), and 0 exception(s). View
#17 drupal-1958538-5.patch46.43 KBrobertwb
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 1 fail(s), and 0 exception(s). View
#15 drupal-1958538-4.patch35.79 KBrobertwb
FAILED: [[SimpleTest]]: [MySQL] 120 pass(es), 1 fail(s), and 0 exception(s). View
#14 drupal-1958538-4.patch35.79 KBrobertwb
FAILED: [[SimpleTest]]: [MySQL] 120 pass(es), 1 fail(s), and 0 exception(s). View
#10 drupal-1958538-3.patch35.72 KBrobertwb
FAILED: [[SimpleTest]]: [MySQL] 86 pass(es), 1 fail(s), and 0 exception(s). View
#4 drupal-1958538-2.patch46.33 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 1 fail(s), and 0 exception(s). View
#4 interdiff.txt10.7 KBdawehner
#2 drupal-1938062-66.patch24.16 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1938062-66_1.patch. Unable to apply patch. See the log in the details link for more information. View
#2 interdiff.txt428 bytesdawehner
#1 1958538-math-expr-update.patch35.63 KBmerlinofchaos
PASSED: [[SimpleTest]]: [MySQL] 68 pass(es). View

Comments

merlinofchaos’s picture

Status: Active » Needs review
FileSize
35.63 KB
PASSED: [[SimpleTest]]: [MySQL] 68 pass(es). View

Notes:

This needs to be documented. I removed the docblock and old license because the rewrite is significant enough as to no longer require the old license, I believe.

Tests are relatively trivial to write and probably should be written. The tests could also help serve to demonstrate some of the things that can be done here.

Being able to have a very simple if() is quite valuable, particularly with token parsing.

dawehner’s picture

FileSize
428 bytes
24.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1938062-66_1.patch. Unable to apply patch. See the log in the details link for more information. View

Backported the test coverage from drupal 8 to drupal 7: http://drupalcode.org/project/ctools.git/commit/4dc2d93c34ae0a5759dace09...

Just wondering: Does "if(3 > 2, 4, 5)" actually work? The test says no., see math_expression.test at the end.

@todo: Add a test for string support.

The diff looks horrible because ->e( support was dropped.

merlinofchaos’s picture

Hm. ->e() support can be added back in. I just thought it was kind of silly, so removed it. But that's actually a bad idea; clearly people might be using it.

In working with this today I've found quite a few issues with the string support, so I'll have to post a new patch when I get a chance. I also added support for substr() because that's really useful when doing some string manipulation.

dawehner’s picture

FileSize
10.7 KB
46.33 KB
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 1 fail(s), and 0 exception(s). View

Here are the files from the right directory.

Status: Needs review » Needs work

The last submitted patch, drupal-1958538-2.patch, failed testing.

zterry95’s picture

tested patch #4, and it can solve the problem.

Tested only the express "MAX" and "MIN".

robertwb’s picture

Component: Code » Documentation

Where should documentation for this function go? In the code, or somewhere here on the Drupal site?

I am currently doing some trial and error and would like to share some of my observations.

Thanks,
r.b.

dawehner’s picture

It seems to be that for ctools advanced help is the place to go?

robertwb’s picture

So... some of this is not ready for the docs page since it is developmental. In answer to @dawehner #2 above, I believe that the answer is "no", the IF code does not yet work. I tested this as follows:

For my test expression I desired a basic switch, keyed on a 0 or a 1 - so if my switch is 1 I return calculation "s" and if it is not (zero is the other setting currently) it returns calculation "c". I have two cases of the IF expression here, the 1st does not work and the 2nd does:

a = if(([field_rate_type] != 1), s, c);
a

This does not work, since the function ctools_math_expr_if is passed a blank character as its first argument a la: ctools_math_expr_if(, 3.97828, 4.3182714126005 ).

However, The IF expression DOES work if it is passed a numeric binary argument, so the following code DOES work:

a = if( [field_rate_type] , s, c);
a

Returning "s" if my field-rate_type = 1 and "c" otherwise. So, what I think is happening is that the TRUE/FALSE result of the comparator "if (($op1 == $op2)) " is being somehow stripped because it is non-numeric?

The following code works in both cases:

         case '==':
            //original code 
            //$stack->push($op1 == $op2);
            if (($op1 == $op2)) {
               $stack->push(1);
            } else {
               $stack->push(0);
            }
            break;

I have not formulated this into a patch, as 1) I don't know if returning numerical instead of logical results is the desired result of this IF function, and 2) I am just starting to learn git and don't know how to do a proper patch yet.

Regards,
r.b.

robertwb’s picture

Status: Needs work » Needs review
FileSize
35.72 KB
FAILED: [[SimpleTest]]: [MySQL] 86 pass(es), 1 fail(s), and 0 exception(s). View

The switch from TRUE/FALSE to 0/1 has been applied to the == operator only in this patch. Still dunno if this is correct. Should the module return "pure" logical operators?

Status: Needs review » Needs work

The last submitted patch, drupal-1958538-3.patch, failed testing.

robertwb’s picture

Status: Needs work » Needs review

#10: drupal-1958538-3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1958538-3.patch, failed testing.

robertwb’s picture

FileSize
35.79 KB
FAILED: [[SimpleTest]]: [MySQL] 120 pass(es), 1 fail(s), and 0 exception(s). View

Added the e() function as an alias of the evaluate() function so that the tests could execute.

r.b.

robertwb’s picture

Status: Needs work » Needs review
FileSize
35.79 KB
FAILED: [[SimpleTest]]: [MySQL] 120 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, drupal-1958538-4.patch, failed testing.

robertwb’s picture

Status: Needs work » Needs review
FileSize
46.43 KB
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 1 fail(s), and 0 exception(s). View

Repatched from #4...

Status: Needs review » Needs work

The last submitted patch, drupal-1958538-5.patch, failed testing.

robertwb’s picture

Status: Needs work » Needs review
FileSize
46.52 KB
FAILED: [[SimpleTest]]: [MySQL] 127 pass(es), 1 fail(s), and 0 exception(s). View

Added the fix mentioned in #8 to the ">" operator in order to pass the test.

Status: Needs review » Needs work

The last submitted patch, drupal-1958538-6.patch, failed testing.

robertwb’s picture

Status: Needs work » Needs review
FileSize
46.52 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es). View

Modified test file, testif in line 141 was doomed to fail I think - since it test if a small # was > a bigger number, but asserted equal to return the larger number instead of the smaller:

$this->assertEqual($math_expression->evaluate("if($random_number_a > $random_number_b, $random_number_a, $random_number_b)"), $random_number_a);
TO
$this->assertEqual($math_expression->evaluate("if($random_number_a > $random_number_b, $random_number_a, $random_number_b)"), $random_number_b);

Sorry if this is too much detail, i am monkeying about and being a newby I want to make sure that it is crystal clear what I am goofing up.

r.b.

dawehner’s picture

It would be cool if you could provide an interdiff, as it is way easier to review.

robertwb’s picture

FileSize
1.55 KB

Thanks for the heads up - interdiff attached.

dawehner’s picture

Thank you! Theoretically we could cast the bool to an int here, but I am fine with that.

We have quite some test coverage to it feels okay to get that one in and maybe even iterate on top of it later?

robertwb’s picture

That's just fine, though it would take me about 15 minutes to duplicate that approach for the remaining comparison operators, making it a more functional patch. I will put that version of the patch up here and then you can decide which you desired to put in.

r.b.

robertwb’s picture

FileSize
1.75 KB
46.38 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es). View

Attached sets all logical operators to return integer 1/0 using an intval cast.

r.b.

izmeez’s picture

Issue summary: View changes

I have applied the patch in #26 because of warnings in ableorganizer #2167695: Divide by zero on dashboard if no sample data installed

Patch applies without problems. However, after that I have two warnings:

User warning: division by zero in ctools_math_expr->trigger() (line 504 of /home/ableorg/public_html/profiles/ableorganizer/modules/contrib/ctools/includes/math-expr.inc).

Warning: number_format() expects parameter 1 to be double, string given in views_handler_field_math->render() (line 58 of /home/ableorg/public_html/profiles/ableorganizer/modules/contrib/views/handlers/views_handler_field_math.inc).

robertwb’s picture

Can you supply some details about your use case? The warning given suggests that you are passing it a string instead of a number?

One thing I have seen is that by default Drupal numeric fields include a thousands separator ",", which the Math Expression engine does not like. If your sample data has a thousands separator in the view field definition, it will give an error as well as failing to evaluate / resulting in zero.

Warning: number_format() expects parameter 1 to be double, string given
izmeez’s picture

@robertwb Thanks for the response. I hope Dane Powell is following this thread along with #2167695: Divide by zero on dashboard if no sample data installed.

izmeez’s picture

Thank you. The issue this relates to in AbleOrganizer has been resolved.

  • dawehner committed 4dc2d93 on 8.x-2.x
    Issue #1958538: Backported math expression test coverage from d8
    

  • dawehner committed 4dc2d93 on 8.x-3.x
    Issue #1958538: Backported math expression test coverage from d8
    
Joanna_Kisaakye’s picture

FileSize
46.45 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es). View
600 bytes

I applied the drupal-1958538-8 patch and still got a "too many arguments" error whenever I tried to use the power function which takes 2 arguments. So I'm rerolling drupal-1958538-8.patch with the correction to this included.

robertwb’s picture

I suppose that means we lack test coverage for the pow() function?

draenen’s picture

Component: Documentation » Code
Status: Needs review » Reviewed & tested by the community

Patch from #34 tested and works with the Math Field module.

mgifford’s picture

japerry’s picture

Status: Reviewed & tested by the community » Needs review

Wow this is pretty large to just push into ctools . I'd like to see some more people review this first.

robertwb’s picture

Hey @japerry - the patch is big, but I would estimate that 25% of it is due to reformatting of spaces, and another 50% is due to renaming the test method from e() to evaluate(). Would it be more easy to review if the test method name changes were split into a separate patch/issue?

DamienMcKenna’s picture

Moving this to the v7.x-1.10 release plan.

howto’s picture

Hi,

I can not apply patch https://www.drupal.org/files/issues/drupal-1958538-9.patch to Ctools latest version 7.x-1.x. Here is the error:

error: patch failed: includes/math-expr.inc:1
error: includes/math-expr.inc: patch does not apply
Checking patch tests/math_expression.test...

damiankloip’s picture

FileSize
46.43 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es). View

Needs a reroll, was quite confusing, then realised it was just due to #2512058: spelling errors in D7... *sigh* :)

robertwb’s picture

The re-roll in #42 applies just fine. I have not yet tested functionality in Views.

stephaniemoore’s picture

Applied patch #42 to ctools 7.x-1.9, tested with min() and max() in Math Field module, worked great.

Renrhaf’s picture

Hello, what about implementing the modulo operator (%) directly in the operator's list ?

DamienMcKenna’s picture

This didn't get added to 7.x-1.10.

veroniqueg’s picture

I can't apply the patch on version 7.x-1.10 nor 7.x-1.12

DamienMcKenna’s picture

Status: Needs review » Needs work

This needs a reroll.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
43.26 KB

Rerolled.

veroniqueg’s picture

patch #49 works for me for max() on version 7.x-1.12, thanks

rivimey’s picture

Related issues: +#2830393: Improve test coverage and function docs: math_expression

Just a note to say that I have created a test suite for math expression in #2830393: Improve test coverage and function docs: math_expression, linked.

rivimey’s picture

merlinofchaos wrote:

This needs to be documented. I removed the docblock and old license because the rewrite is significant enough as to no longer require the old license, I believe.

Doing this makes me feel uneasy because this seems to be a derived work and although I have no problem removing parts of the doc block if it is now irrelevant, the author details should at least be left as they were.

I also worry about the proposed change of e() to evaluate(); it seems far too risky a thing to do at this point.
If it must be included, can I suggest including a 'thunk' function: public function e($expr) { return $this->evaluate($expr); } for compatability.

DamienMcKenna’s picture

My rerolled patch includes the copyright comment again.

DamienMcKenna’s picture

FileSize
58.11 KB
431 bytes

This adds an e() wrapper for the method for backwards compatible, which completely makes sense.

rivimey’s picture

New patch, because there was a "$this->" missing.

Took opportunity to add 'public'/'protected' markers to the exported function names.

GlossyIbis’s picture

Patch #49 works for me for max() on version 7.x-1.12. Thanks very much for this excellent module.

rivimey’s picture

darrenwh’s picture

Status: Needs review » Needs work
  1. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +  var $suppress_errors = FALSE;
    +  var $last_error = NULL;
    +  var $errors = array();
    +
    +  var $v = array('e' => 2.71, 'pi' => 3.14); // variables (and constants)
    +  var $f = array(); // user-defined functions
    +  var $vb = array('e', 'pi'); // constants
    +  var $fsimple = array( // built-in functions
    +    'sin', 'sinh', 'asin', 'asinh',
    +    'cos', 'cosh', 'acos', 'acosh',
    +    'tan', 'tanh', 'atan', 'atanh',
    +    'exp',
    +    'sqrt', 'abs', 'log',
    +    'time', 'ceil', 'floor', 'round'
    +  );
    +  var $fb = array(
    +    'ln' => array(
    +      'function' => 'log',
    +      'arguments' => 1,
    +    ),
    +    'arcsin' => array(
    +      'function' => 'asin',
    +      'arguments' => 1,
    +    ),
    +    'arcsinh' => array(
    +      'function' => 'asinh',
    +      'arguments' => 1,
    +    ),
    +    'arccos' => array(
    +      'function' => 'acos',
    +      'arguments' => 1,
    +    ),
    +    'arccosh' => array(
    +      'function' => 'acosh',
    +      'arguments' => 1,
    +    ),
    +    'arctan' => array(
    +      'function' => 'atan',
    +      'arguments' => 1,
    +    ),
    +    'arctanh' => array(
    +      'function' => 'atanh',
    +      'arguments' => 1,
    +    ),
    +    'min' => array(
    +      'function' => 'min',
    +      'arguments' => 2,
    +      'max arguments' => 99,
    +    ),
    +    'max' => array(
    +      'function' => 'max',
    +      'arguments' => 2,
    +      'max arguments' => 99,
    +    ),
    +    'pow' => array(
    +      'function' => 'pow',
    +      'arguments' => 2,
    +    ),
    +    'if' => array(
    +      'function' => 'ctools_math_expr_if',
    +      'arguments' => 2,
    +      'max arguments' => 3,
    +    ),
    +    'number' => array(
    +      'function' => 'ctools_math_expr_number',
    +      'arguments' => 1,
    +    ),
    +  );
    +
    

    The var keyword must not be used to declare a property. (should be public, protected or private)

  2. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +    if (preg_match('/^\s*([a-z]\w*)\s*=\s*(.+)$/', $expr, $matches)) {
    

    There should be no white space after an opening "{"

  3. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +      if (in_array($matches[1], $this->vb)) { // make sure we're not assigning to a constant
    

    Inline comments must start with a capital letter, needs a full stop at end, Comments may not appear after statements.

  4. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +      } // get the result and make sure it's good
    

    Inline comments must start with a capital letter, needs a full stop at end, Comments may not appear after statements.

  5. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +    elseif (preg_match('/^\s*([a-z]\w*)\s*\(\s*([a-z]\w*(?:\s*,\s*[a-z]\w*)*)\s*\)\s*=\s*(.+)$/', $expr, $matches)) {
    

    There should be no white space after an opening "{"

  6. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +   * @return array
    

    Description for the @return value is missing

  7. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +  // Convert infix to postfix notation
    

    Missing ending period(.) should use /** style comments

  8. +++ b/includes/math-expr.inc
    @@ -81,308 +81,562 @@ LICENSE
    +      '*'=> array(
    

    Needs space before =>

I've started doing some DCS checks on this but it needs a good tidy up.

rivimey’s picture

@darrenwh, thanks for your checks. The code you pointed out was inherited from a long time ago. There is a lot that could be done to improve it, I agree. Are you willing to help?

darrenwh’s picture

Yes sure, will take a look tomorrow

rivimey’s picture

Darren, I have reworked some things and I think it is better now. I have commented out part of ctools_math_expr_number as I feel it is dangerous to do this; feedback on that point would be great. I have been working on the math test suite in #2830393, so more tests should be added there rather than here. I feel more tests are probably warranted, though.

I'll leave it for now and hope for feedback :-)

rivimey’s picture

Status: Needs work » Needs review
darrenwh’s picture

FileSize
58.62 KB

I've done further changes to address other DCS issues on the math-expr.inc file, though there are still a few issues regarding naming conventions on the class names and properties that need to be in camel case.

Removed patch #57 from file list

rivimey’s picture

I have added here an interdiff for your change in #63 and for this change (#64), just for the record.

I don't think we can reasonably change the class or function names at this point; they will have to count as legacy. I have made a few more changes, notably the removal of an unintentional change to the uuid.inc file.

DId you intentionally undo the change I made to the file header comment? I felt that including function and variable documentation there as well as in docblock comments should not be done (DRY) so removed them, although I do feel that the copyright, license info should remain, (and that the comment as it stood would not be seen in the normal html api docs).

I noticed another bug, too: strtolower() is inappropriate. Added a TODO to the growing list. I would appreciate feedback on these:

- TODO: Is this supposed to remove all constants? we should remove all
- TODO: Because the expr can contain string operands, using strtolower here is a bug.
- TODO: A really bad idea: It might be good for those using the 12,345.67

zenimagine’s picture

Aggregate to display the sum of a mathematical expression does not work

robertwb’s picture

@zenimagine - I am not sure if the problem you note exists unless it just suyrfaced in one of the most recent patches. If it did not surface in a patch, then it would be a separate bug report.

In order to determine if this is a problem with the patch (I do aggregates all the time with math expression with one of the earlier version of the patch in this issue) -- make sure you expect the correct behavior from the aggregate -- it needs to take place at the level of whatever operands are included in the match expression, the math expression is evaluated after the query is run and thus no aggregation could occur -- unless you were adding it to the "Group by" in the display settings, in which case this would definitely be a different request.

zenimagine’s picture

@robertwb Here is my question with more details (sorry for my english, I use a translator) :

http://drupal.stackexchange.com/questions/225621/can-not-aggregate-a-glo...

robertwb’s picture

@zenimagine - this would be a support request for a separate issue. The solution involves knowing that SQL level aggregation occurs first, then the math expression is evaluated during the rendering of the view, so your hierarchy of mathematical evaluation will need to be able to work around that.

darrenwh’s picture

@rivimey I don't think I intentionally changed your header comments feel free to reinstate, agree that function comments should not be there; should be before functions.

I'll add to does to description

darrenwh’s picture

Issue summary: View changes
darrenwh’s picture

Issue summary: View changes
rivimey’s picture

darrenwh: I wrote the tests in another issue, which originally could be applied without this issue and which is linked to this one. I have adapted the summary appropriately.

I attach an updated patch that removes the methods from the file header comment. Do you think we can RTBC this yet?

rivimey’s picture