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.

CommentFileSizeAuthor
#92 ctools-1958538-92.patch69.47 KBMustangGB
#91 ctools-1958538-91.patch57.73 KBMustangGB
#90 ctools-1958538-89.patch57.73 KBMustangGB
#84 ctools-1958538-84.patch57.79 KBMustangGB
#77 sigmaxim-MinMaxIssue.patch14.55 KByugasa
#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
#63 ctools-n1958538-63.patch58.62 KBdarrenwh
#61 ctools-n1958538-61.patch52.8 KBrivimey
#57 ctools-n1958538-57.patch43.39 KBrivimey
#55 ctools-n1958538-55.patch43.5 KBrivimey
#55 ctools-n1958538-55-interdiff.txt1.15 KBrivimey
#54 ctools-n1958538-54.interdiff.txt431 bytesDamienMcKenna
#54 ctools-n1958538-54.patch58.11 KBDamienMcKenna
#49 ctools-n1958538-49.patch43.26 KBDamienMcKenna
#42 ctools-1958538-42.patch46.43 KBdamiankloip
#34 interdiff.txt600 bytesJoanna_Kisaakye
#34 drupal-1958538-9.patch46.45 KBJoanna_Kisaakye
#26 drupal-1958538-8.patch46.38 KBrobertwb
#26 interdiff.txt1.75 KBrobertwb
#23 interdiff.txt1.55 KBrobertwb
#21 drupal-1958538-7.patch46.52 KBrobertwb
#19 drupal-1958538-6.patch46.52 KBrobertwb
#17 drupal-1958538-5.patch46.43 KBrobertwb
#15 drupal-1958538-4.patch35.79 KBrobertwb
#14 drupal-1958538-4.patch35.79 KBrobertwb
#10 drupal-1958538-3.patch35.72 KBrobertwb
#4 drupal-1958538-2.patch46.33 KBdawehner
#4 interdiff.txt10.7 KBdawehner
#2 drupal-1938062-66.patch24.16 KBdawehner
#2 interdiff.txt428 bytesdawehner
#1 1958538-math-expr-update.patch35.63 KBmerlinofchaos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

merlinofchaos’s picture

Status: Active » Needs review
FileSize
35.63 KB

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

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

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

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

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

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

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

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

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

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

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.

mautumn’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

japerry’s picture

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

rivimey’s picture

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

function hook_ctools_math_expression_functions_alter(&$functions, $context = array())
darrenwh’s picture

@rivimey Can I assume the tests cover all new code? If this is the case RTBC

yugasa’s picture

Issue tags: -Needs tests +Already tested
FileSize
14.55 KB

Status: Needs review » Needs work

The last submitted patch, 77: sigmaxim-MinMaxIssue.patch, failed testing.

DamienMcKenna’s picture

This didn't get into 1.13, maybe it will get into 1.14.

We need to try getting this to RTBC.

kebne’s picture

Cant patch #72 cleanly against 7.x-1.14

DamienMcKenna’s picture

darrenwh’s picture

 mode change 100644 => 100755 mathfield.test

Looks like a few files being changed to 755, only folders should have this permission

izmeez’s picture

What 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?

MustangGB’s picture

Status: Needs work » Needs review
FileSize
57.79 KB

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

rivimey’s picture

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

MustangGB’s picture

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

rivimey’s picture

MustangGB, 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.

MustangGB’s picture

Okay 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?

DamienMcKenna’s picture

I mean ideally would be good to get both issues committed, however they seem to be blockers of each other...

Would it be worth merging in the test coverage changes, given this one was reverted and improved?

MustangGB’s picture

Issue tags: -Already tested
FileSize
57.73 KB
MustangGB’s picture

FileSize
57.73 KB

Removed stranded debug statements to fix test failure.

MustangGB’s picture

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

joelpittet’s picture

joelpittet’s picture

Status: Needs review » Fixed

Thought 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

  • joelpittet committed 7da4709 on 7.x-1.x authored by MustangGB
    Issue #1958538 by robertwb, rivimey, MustangGB, dawehner, DamienMcKenna...
MustangGB’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.