Skip to content

Conversation

@scq
Copy link
Contributor

@scq scq commented Dec 1, 2025

calc() expressions that are multiline, where one line ends on an operator, currently fail to parse. E.g.

.btn {
  --foo: calc(
    var(--bar) +
      (var(--baz) + var(--qux)) * 2
  );
}

This is because the code is looking for a space character following the operator, and failing when it sees a newline instead.

@coveralls
Copy link

coveralls commented Dec 1, 2025

Coverage Status

coverage: 62.467% (+2.8%) from 59.658%
when pulling 3de58cd on totara:calc-parsing-fix
into fe29992 on MyIntervals:main.

Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @scq,

Thanks for this. I concur that the spec allows any type of whitespace around the + and - operators (requiring at least one whitespace character).

To cover all types of whitespace, we can use preg_match with \s (via the 'Safe' library).

As part of this change, it would be good to also allow all types of whitespace before the operator (as well as after) - with tests covering that too.

We're aiming to move the tests away from functional tests to dedicated unit tests. I note there is not yet a CalcFunctionTest. Adding such a TestCase for the tests related to this change would be good.

Also, could you add an entry to the CHANGELOG.md file (in the root folder of the repository)?

@JakeQZ JakeQZ added the bug label Dec 1, 2025
@scq scq force-pushed the calc-parsing-fix branch from 8502a50 to 3de58cd Compare December 2, 2025 02:13
@scq
Copy link
Contributor Author

scq commented Dec 2, 2025

Sweet, thanks for the review! Made those changes.

Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this is amazing and absolutely perfect.

I wasn't expecting such a comprehensive test suite. Appreciate the time you have taken, which has saved us some :))

Thank you ever so much <3

We are on the lookout for additional collaborators if you are interested...

@JakeQZ JakeQZ merged commit b01197d into MyIntervals:main Dec 2, 2025
23 checks passed
@JakeQZ
Copy link
Collaborator

JakeQZ commented Dec 2, 2025

Coverage Status

coverage: 62.467% (+2.8%) from 59.658% when pulling 3de58cd on totara:calc-parsing-fix into fe29992 on MyIntervals:main.

That's impressive too :)

JakeQZ

This comment was marked as outdated.

Comment on lines +195 to +196
* @param string $css
* @return CalcFunction
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These PHPDoc annotations are actually redundant, since the types are in the method signature.

Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking again, I found a few other minor nitpicks.

But this is a great set of tests, so I was keen to get it merged.

Comment on lines 21 to +23
- Use typesafe versions of PHP functions (#1379, #1380, #1382, #1383, #1384)
- Fix parsing of `calc` expressions when a newline immediately precedes or
follows a + or - operator (#1399)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention that we put the changelog entries in reverse chronological order (though I'm not sure why).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants