Fix/invalid calc parsing #169

raxbg · 2019-09-18T15:50:34Z

This PR adds better handling of invalid calc definitions.

sabberworm · 2019-11-12T15:58:07Z

tests/files/calc-invalid.css

+}
+
+div{
+  height: calc


I don’t think we should disallow calc used as a non-function keyword (I realize your change didn’t break this and simply improved the error reporting) but to my knowledge the following is valid:

#outer { grid-template-areas: "top calc" "bottom calc"; } #inner { grid-area: calc; }

Or maybe I misunderstood… is the error caused by the missing semicolon at the end?

It appears that

#inner { grid-area: url; }

is as valid. I pushed an update that handles this and as a side effect it also fixes #170. If this gets merged PR #174 can be closed.

…em as non-function keywords

sabberworm · 2019-12-02T15:21:01Z

lib/Sabberworm/CSS/Parsing/ParserState.php

+	public function setAnchor() {
+		$this->iAnchor = $this->iCurrentPosition;
+	}
+
+	public function backtrackToAnchor() {
+		if ($this->iAnchor !== null) {
+			$this->iCurrentPosition = $this->iAnchor;
+		}
+	}
+


I kinda like the idea with the anchor but I’d prefer it to be implemented as a separate class:

class Anchor { private $iPosition; private $oParserState; […] function backtrack() { $this->oParserState->iCurrentPosition = $this->iPosition; // Or using setter } } class ParserState { […] function anchor() { return new Anchor($this->iCurrentPosition, $this); } […] }

sabberworm · 2019-12-02T15:27:00Z

lib/Sabberworm/CSS/Value/Value.php

-			$oParserState->consume(')');
+			if ($oParserState->streql('url', $mResult)) {
+				$oParserState->backtrackToAnchor();
+				$mResult = URL::parse($oParserState);


Do we use URL::parse and CalcFunction::parse also in other places? Otherwise we could just change them to not parse the function name (or pass it as argument in case of CalcFunction).

Also maybe we should just add a CSSFunction::parse method so that all three cases have the same structure.

sabberworm · 2019-12-02T15:31:34Z

lib/Sabberworm/CSS/Value/URL.php

@@ -14,11 +14,21 @@ public function __construct(CSSString $oURL, $iLineNo = 0) {
 	}

 	public static function parse(ParserState $oParserState) {
-		$bUseUrl = $oParserState->comes('url', true);


I’m not sure why comes isn’t good… Can you explain?

sabberworm · 2020-03-18T15:34:20Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2
- Added 7
           

Complexity increasing per file
==============================
- lib/Sabberworm/CSS/Value/URL.php  2
- lib/Sabberworm/CSS/Value/CalcFunction.php  4

See the complete overview on Codacy

sabberworm · 2020-03-18T15:34:22Z

tests/files/calc-invalid.css

+
+div{
+  height: calc
+  (25% - 1em);


Issue found: Expected RBRACE at line 7, col 3.

sabberworm · 2020-03-18T15:34:23Z

lib/Sabberworm/CSS/Value/URL.php

 			$oParserState->consumeWhiteSpace();
 			$oParserState->consume('(');
+		} else {


Issue found: The method parse uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them.

sabberworm · 2020-03-18T15:34:23Z

tests/files/calc-invalid.css

+}
+
+div{
+  height: calc


Issue found: Missed semicolon (CssSyntaxError)

sabberworm · 2020-03-18T15:34:25Z

lib/Sabberworm/CSS/Value/Value.php

-			$oParserState->consume(')');
+			if ($oParserState->streql('url', $mResult)) {
+				$oParserState->backtrackToAnchor();
+				$mResult = URL::parse($oParserState);


Issue found: Avoid using static access to class 'Sabberworm\CSS\Value\URL' in method 'parseIdentifierOrFunction'.

sabberworm · 2020-03-18T15:34:26Z

tests/files/calc-invalid.css

+
+div{
+  height: calc
+  width: 100px


Issue found: Expected RBRACE at line 12, col 8.

sabberworm · 2020-03-18T15:34:27Z

tests/files/calc-invalid.css

@@ -0,0 +1,22 @@
+div{
+  height: calc (25% - 1em);


Issue found: Expected RBRACE at line 2, col 16.

sabberworm · 2020-03-18T15:34:28Z

lib/Sabberworm/CSS/Value/Value.php

+				$mResult = URL::parse($oParserState);
+			} else if ($oParserState->streql('calc', $mResult) || $oParserState->streql('-webkit-calc', $mResult) || $oParserState->streql('-moz-calc', $mResult)) {
+				$oParserState->backtrackToAnchor();
+				$mResult = CalcFunction::parse($oParserState);


Issue found: Avoid using static access to class 'Sabberworm\CSS\Value\CalcFunction' in method 'parseIdentifierOrFunction'.

sabberworm

Thank you very much, looks fantastic!

raxbg added 3 commits September 12, 2019 18:51

Fix parsing of missing calc functions

823bf14

Detect invalid calc definitions like 'calc ('

81143bd

Add unit tests with invalid calc definitions

5ed4383

sabberworm reviewed Nov 12, 2019

View reviewed changes

raxbg added 4 commits November 13, 2019 18:51

Merge branch 'master' into fix/invalid_calc_parsing

6ac546d

Handle EOF when parsing calc functions

c7a03a3

Improve parsing of calc and url by handling escapes and preserving th…

64f397c

…em as non-function keywords

Import the tests from PR MyIntervals#174

ee1f8fe

sabberworm reviewed Dec 2, 2019

View reviewed changes

sabberworm reviewed Mar 18, 2020

View reviewed changes

tests/files/calc-invalid.css Outdated

div{

height: calc

(25% - 1em);

Copy link

Contributor

sabberworm Mar 18, 2020

Choose a reason for hiding this comment

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

Issue found: Expected RBRACE at line 7, col 3.

sabberworm reviewed Mar 18, 2020

View reviewed changes

tests/files/calc-invalid.css Outdated

}

div{

height: calc

Copy link

Contributor

sabberworm Mar 18, 2020

Choose a reason for hiding this comment

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

Issue found: Missed semicolon (CssSyntaxError)

sabberworm reviewed Mar 18, 2020

View reviewed changes

tests/files/calc-invalid.css Outdated

div{

height: calc

width: 100px

Copy link

Contributor

sabberworm Mar 18, 2020

Choose a reason for hiding this comment

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

Issue found: Expected RBRACE at line 12, col 8.

sabberworm reviewed Mar 18, 2020

View reviewed changes

raxbg added 8 commits September 17, 2022 14:36

Merge branch 'fix/invalid_calc_parsing' into mergable_master

49168f3

Merge branch 'upstream_master' into mergable_master

221974b

Fix indents

c3c707c

Add back code that got dropped while resolving conflicts

2cef8a0

Re-implement ParserState anchors with dedicated Anchor class

8ac156d

Streamline parsing functions

f7019f8

Code refactor for CI

c99ccf4

Fix unit tests

0678cff

raxbg mentioned this pull request Sep 19, 2022

Fix/arithmetic in functions #390

Closed

sabberworm approved these changes Sep 20, 2022

View reviewed changes

sabberworm merged commit bbece12 into MyIntervals:master Sep 20, 2022

JakeQZ mentioned this pull request Jan 23, 2024

[TASK] Prepare the 8.5.0 release #419

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/invalid calc parsing #169

Fix/invalid calc parsing #169

raxbg commented Sep 18, 2019

sabberworm Nov 12, 2019

sabberworm Nov 12, 2019

raxbg Nov 13, 2019 •

edited

Loading

sabberworm Dec 2, 2019

sabberworm Dec 2, 2019

sabberworm Dec 2, 2019

sabberworm commented Mar 18, 2020

sabberworm Mar 18, 2020

sabberworm Mar 18, 2020

sabberworm Mar 18, 2020

sabberworm Mar 18, 2020

sabberworm Mar 18, 2020

sabberworm Mar 18, 2020

sabberworm Mar 18, 2020

sabberworm left a comment

Fix/invalid calc parsing #169

Fix/invalid calc parsing #169

Conversation

raxbg commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raxbg Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabberworm commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabberworm left a comment

Choose a reason for hiding this comment

raxbg Nov 13, 2019 •

edited

Loading