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.

Patch forthcoming.

Files: 
CommentFileSizeAuthor
#34 interdiff.txt600 bytesJoanna_Kisaakye
#34 drupal-1958538-9.patch46.45 KBJoanna_Kisaakye
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
#26 drupal-1958538-8.patch46.38 KBrobertwb
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
#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
StatusFileSize
new35.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

StatusFileSize
new428 bytes
new24.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

StatusFileSize
new10.7 KB
new46.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
StatusFileSize
new35.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

StatusFileSize
new35.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
StatusFileSize
new35.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
StatusFileSize
new46.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
StatusFileSize
new46.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
StatusFileSize
new46.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

StatusFileSize
new1.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

StatusFileSize
new1.75 KB
new46.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

StatusFileSize
new46.45 KB
PASSED: [[SimpleTest]]: [MySQL] 128 pass(es).
[ View ]
new600 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?