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.
Comment | File | Size | Author |
---|---|---|---|
#92 | ctools-1958538-92.patch | 69.47 KB | MustangGB |
#72 | ctools-n1958538-72.patch | 57.69 KB | rivimey |
#64 | 1958538-63-64-interdiff.txt | 3.83 KB | rivimey |
#64 | 1958538-61-63-interdiff.txt | 19.01 KB | rivimey |
#26 | interdiff.txt | 1.75 KB | robertwb |
Comments
Comment #1
merlinofchaos CreditAttribution: merlinofchaos commentedNotes:
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.
Comment #2
dawehnerBackported 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.
Comment #3
merlinofchaos CreditAttribution: merlinofchaos commentedHm. ->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.
Comment #4
dawehnerHere are the files from the right directory.
Comment #6
zterry95 CreditAttribution: zterry95 commentedtested patch #4, and it can solve the problem.
Tested only the express "MAX" and "MIN".
Comment #7
robertwb CreditAttribution: robertwb commentedWhere 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.
Comment #8
dawehnerIt seems to be that for ctools advanced help is the place to go?
Comment #9
robertwb CreditAttribution: robertwb commentedSo... 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:
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:
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:
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.
Comment #10
robertwb CreditAttribution: robertwb commentedThe 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?
Comment #12
robertwb CreditAttribution: robertwb commented#10: drupal-1958538-3.patch queued for re-testing.
Comment #14
robertwb CreditAttribution: robertwb commentedAdded the e() function as an alias of the evaluate() function so that the tests could execute.
r.b.
Comment #15
robertwb CreditAttribution: robertwb commentedComment #17
robertwb CreditAttribution: robertwb commentedRepatched from #4...
Comment #19
robertwb CreditAttribution: robertwb commentedAdded the fix mentioned in #8 to the ">" operator in order to pass the test.
Comment #21
robertwb CreditAttribution: robertwb commentedModified 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.
Comment #22
dawehnerIt would be cool if you could provide an interdiff, as it is way easier to review.
Comment #23
robertwb CreditAttribution: robertwb commentedThanks for the heads up - interdiff attached.
Comment #24
dawehnerThank 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?
Comment #25
robertwb CreditAttribution: robertwb commentedThat'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.
Comment #26
robertwb CreditAttribution: robertwb commentedAttached sets all logical operators to return integer 1/0 using an intval cast.
r.b.
Comment #27
izmeez CreditAttribution: izmeez commentedI 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).
Comment #28
robertwb CreditAttribution: robertwb commentedCan 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.
Comment #29
izmeez CreditAttribution: izmeez commented@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.
Comment #30
izmeez CreditAttribution: izmeez commentedThank you. The issue this relates to in AbleOrganizer has been resolved.
Comment #34
Joanna_Kisaakye CreditAttribution: Joanna_Kisaakye commentedI 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.
Comment #35
robertwb CreditAttribution: robertwb commentedI suppose that means we lack test coverage for the pow() function?
Comment #36
draenen CreditAttribution: draenen at Monarch Digital commentedPatch from #34 tested and works with the Math Field module.
Comment #37
mgiffordComment #38
japerryWow this is pretty large to just push into ctools . I'd like to see some more people review this first.
Comment #39
robertwb CreditAttribution: robertwb commentedHey @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?
Comment #40
DamienMcKennaMoving this to the v7.x-1.10 release plan.
Comment #41
howto CreditAttribution: howto commentedHi,
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:
Comment #42
damiankloip CreditAttribution: damiankloip commentedNeeds a reroll, was quite confusing, then realised it was just due to #2512058: spelling errors in D7... *sigh* :)
Comment #43
robertwb CreditAttribution: robertwb commentedThe re-roll in #42 applies just fine. I have not yet tested functionality in Views.
Comment #44
stephaniemoore CreditAttribution: stephaniemoore commentedApplied patch #42 to ctools 7.x-1.9, tested with min() and max() in Math Field module, worked great.
Comment #45
RenrhafHello, what about implementing the modulo operator (%) directly in the operator's list ?
Comment #46
DamienMcKennaThis didn't get added to 7.x-1.10.
Comment #47
veroniqueg CreditAttribution: veroniqueg at Mouvement ATD Quart Monde commentedI can't apply the patch on version 7.x-1.10 nor 7.x-1.12
Comment #48
DamienMcKennaThis needs a reroll.
Comment #49
DamienMcKennaRerolled.
Comment #50
veroniqueg CreditAttribution: veroniqueg at Mouvement ATD Quart Monde commentedpatch #49 works for me for max() on version 7.x-1.12, thanks
Comment #51
rivimeyJust 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.
Comment #52
rivimeymerlinofchaos wrote:
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.
Comment #53
DamienMcKennaMy rerolled patch includes the copyright comment again.
Comment #54
DamienMcKennaThis adds an e() wrapper for the method for backwards compatible, which completely makes sense.
Comment #55
rivimeyNew patch, because there was a "$this->" missing.
Took opportunity to add 'public'/'protected' markers to the exported function names.
Comment #56
mautumn CreditAttribution: mautumn as a volunteer commentedPatch #49 works for me for max() on version 7.x-1.12. Thanks very much for this excellent module.
Comment #57
rivimeyReroll patch.
Comment #58
darrenwh CreditAttribution: darrenwh as a volunteer and at Investis Digital commentedThe var keyword must not be used to declare a property. (should be public, protected or private)
There should be no white space after an opening "{"
Inline comments must start with a capital letter, needs a full stop at end, Comments may not appear after statements.
Inline comments must start with a capital letter, needs a full stop at end, Comments may not appear after statements.
There should be no white space after an opening "{"
Description for the @return value is missing
Missing ending period(.) should use /** style comments
Needs space before =>
I've started doing some DCS checks on this but it needs a good tidy up.
Comment #59
rivimey@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?
Comment #60
darrenwh CreditAttribution: darrenwh commentedYes sure, will take a look tomorrow
Comment #61
rivimeyDarren, 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 :-)
Comment #62
rivimeyComment #63
darrenwh CreditAttribution: darrenwh commentedI'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
Comment #64
rivimeyI 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
Comment #65
zenimagine CreditAttribution: zenimagine commentedAggregate to display the sum of a mathematical expression does not work
Comment #66
robertwb CreditAttribution: robertwb commented@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.
Comment #67
zenimagine CreditAttribution: zenimagine commented@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...
Comment #68
robertwb CreditAttribution: robertwb commented@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.
Comment #69
darrenwh CreditAttribution: darrenwh commented@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
Comment #70
darrenwh CreditAttribution: darrenwh commentedComment #71
darrenwh CreditAttribution: darrenwh commentedComment #72
rivimeydarrenwh: 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?
Comment #73
rivimeyComment #74
japerryI think my only concern here is with adding the contexts parameter to hook_ctools_math_expression_functions_alter, this would be an API change. Not sure how better to approach this, but I have concerns that this change would break things...
Comment #75
rivimey@japerry While I agree that this is an API change - and perhaps we should call it out in release notes or whatever - I believe it is completely backward compatible in this case: old hook implementations will continue to interact with $functions as before and that is fine. To make it safer, the api.php code should have a default value, so that should a new hook implementation exist and be used with an old ctools you don't get a php warning:
Comment #76
darrenwh CreditAttribution: darrenwh commented@rivimey Can I assume the tests cover all new code? If this is the case RTBC
Comment #77
yugasa CreditAttribution: yugasa commentedComment #79
DamienMcKennaThis didn't get into 1.13, maybe it will get into 1.14.
We need to try getting this to RTBC.
Comment #80
kebne CreditAttribution: kebne commentedCant patch #72 cleanly against 7.x-1.14
Comment #81
DamienMcKennaComment #82
darrenwh CreditAttribution: darrenwh commentedLooks like a few files being changed to 755, only folders should have this permission
Comment #83
izmeez CreditAttribution: izmeez commentedWhat about ctools/tests/ctools.drush.sh does this need to be 755 or only when on a testing server?
Also patch in #72 fails to apply to current 1.x-dev (2018-02-26) probably needs a re-roll. Unable to quickly review and test.
Can anyone confirm that is the patch that needs to be RTBC or does the concern raised in comment #66, #1958538-66: Improve math expression engine mean there has been a divergence?
Comment #84
MustangGB CreditAttribution: MustangGB commentedBased on #72, added support for no arguments so time() works, which is needed for PHP 7.3 compatibility.
Would have added an interdiff, but due to re-roll it was unintelligible.
Comment #85
rivimeyRe comment #66: This was about aggregation, and I think the commenter believed that database row-level aggregation could be done in math expresion, which is not true. Consequently I believe it is ok to ignore that.
MustangGB: good to know 0-arg funcs work now, but did anyone ever sort out single-argument function calls? ISTR that this was a pain point when I was doing things.
Comment #86
MustangGB CreditAttribution: MustangGB commentedAnyone know why page manager tests (HeadLinksTestCase) are failing, is this a problem unrelated to this issue?
@rivimey Could you expand on the single argument calls, or point to the reply mentioning it, they seem to work fine in my testing.
Comment #87
rivimeyMustangGB, sorry my mistake. It is the double arg calls -- min(), max(), pow() and so on that at one point failed. The cause (then) was that the expression parser didn't understand that two arg functions could exist, despite someone putting in code to declare them possible. However in #50 above it is stated that these work.
I did a lot of other work in issue #2830393: Improve test coverage and function docs: math_expression and it would be good to get that merged so we don't have to worry about testing such things.
Comment #88
MustangGB CreditAttribution: MustangGB commentedOkay thanks for clarifying, in my simpletest and manual testing everything appears to be working fine, granted I didn't test with #2830393: Improve test coverage and function docs: math_expression.
I mean ideally would be good to get both issues committed, however they seem to be blockers of each other...
Any maintainers care to add comment?
Comment #89
DamienMcKennaWould it be worth merging in the test coverage changes, given this one was reverted and improved?
Comment #90
MustangGB CreditAttribution: MustangGB commentedComment #91
MustangGB CreditAttribution: MustangGB commentedRemoved stranded debug statements to fix test failure.
Comment #92
MustangGB CreditAttribution: MustangGB commentedAnd here is a version merged with #2830393: Improve test coverage and function docs: math_expression as per #1958538-89: Improve math expression engine.
I noticed some tests weren't behaving nicely with negative numbers, so have added a couple of todo's in there.
Comment #93
joelpittetComment #94
joelpittetThought not a big fan of the @todo and lost of unrelated code standard fixes in the api file, but it does have much better test coverage and PHP 7.4 support so I'll include it in the next release, thanks for all you work on this everybody!
I did a small whitespace cleanup introduced by the patch on commit, BTW
Comment #96
MustangGB CreditAttribution: MustangGB commentedThanks Joel, it's definitely a "multiple steps in the right direction" rather than "everything is perfect" patch.
At least it gives future adventurers something to build on.