Fix/arithmetic in functions #390
Conversation
There was a problem hiding this comment.
Functional changes are all good. A few minor side issues though.
@oliverklee, would you like to see if there is anything else?
I'm aware of two things that I think are beyond the scope of this PR, and part of broader changes we want to make moving forwards:
- We want to move the test data from files to be inlined with the relevant test code.
- We want to have class-specific tests as well as general parsing tests (but as yet there is no
ValueTestTestCase).
| /** | ||
| * @param ParserState $oParserState | ||
| * @param bool $bIgnoreCase | ||
| * | ||
| * @return string | ||
| * | ||
| * @throws SourceException | ||
| * @throws UnexpectedEOFException | ||
| * @throws UnexpectedTokenException | ||
| */ | ||
| public static function parseName(ParserState $oParserState, $bIgnoreCase = false) | ||
| { | ||
| return $oParserState->parseIdentifier($bIgnoreCase); | ||
| } | ||
|
|
||
| /** | ||
| * @param ParserState $oParserState | ||
| * | ||
| * @return array | ||
| * | ||
| * @throws SourceException | ||
| * @throws UnexpectedEOFException | ||
| * @throws UnexpectedTokenException | ||
| */ | ||
| public static function parseArgs(ParserState $oParserState) | ||
| { | ||
| return Value::parseValue($oParserState, ['=', ' ', ',']); | ||
| } | ||
|
|
There was a problem hiding this comment.
I like the refactoring. But can the new methods be made private - I presume they are (currently) only intended to be used internally by the class?
Also, since we now have a minimum PHP version of 7.2, the return types can be included in the function declaration (and removed from the DocBlock, since they would then become redundant there). (I know we didn't have PHP>=7.2 at the time this PR was created.)
| $sExpected = 'div {height: max(300,vh + 10);} | ||
| div {height: max(300,vh - 10);} | ||
| div {height: max(300,vh * 10);} | ||
| div {height: max(300,vh / 10);}'; |
There was a problem hiding this comment.
For readability, can this be a series of concatenations with "\n", so that indentation is retained?
| div { | ||
| height: max(300, vh + 10); | ||
| } | ||
| div { | ||
| height: max(300, vh - 10); | ||
| } | ||
| div { | ||
| height: max(300, vh * 10); | ||
| } | ||
| div { | ||
| height: max(300, vh / 10); | ||
| } |
There was a problem hiding this comment.
I'd prefer valid examples here. vh is a unit not a value. 100vh is the viewport height. Though an extra test where the number is omitted wouldn't go amiss.
| * @throws UnexpectedEOFException | ||
| * @throws UnexpectedTokenException | ||
| */ | ||
| public static function parseArgs(ParserState $oParserState) |
There was a problem hiding this comment.
Perhaps parseArguments is a better name (though I know 'args' is a very common shorthand).
| @@ -0,0 +1,3 @@ | |||
| div { | |||
| height: ///!; | |||
|
do we have an ETA on this one? not having the calc and clamp functions work is a huge issue in 2024 |
|
@nickfmc How fast we progress with this PR depends on whether someone (TM) would be willing to invest time into it. Would you be willing to create a PR (or multiple PRs) for the refactoring contained in this PR here so we get them out of the way, and then take over this PR? |
|
I looked at rebasing this. I don't think it's possible. There have been too many changes since. I'd like to be able to see the full diff on GitHub without the review comments, in order to re-apply the changes. But I can't find how to do that. |
I found the '[Don't] Show Comments' option, it's on the '•••` menu, on a per-file basis. I should be able to take this on as fresh PRs, unless @nickfmc you would like to?! Thanks for the nudge :) |
Also - Update the `parse` function signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclass to avoid contravariance. This is the refactoring change from #390.
Also - Update the `parse` function signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclasses to avoid contravariance. This is the refactoring change from #390.
Also - Update the `parse` function signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclasses to avoid contravariance. Unfortunately, there seems to be a bug in PHP 7.3 (and 7.2) whereby it doesn't recognize a specific class name as a more-specific type than `object`, even though it does recognize it as more specific than an untyped return value. Thus some more-specific type annotations have had to be downgraded. This is the refactoring change from #390.
This is the enhancement from #390.
Also - Update the `parse` function signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclasses to avoid contravariance. Unfortunately, there seems to be a bug in PHP 7.3 (and 7.2) whereby it doesn't recognize a specific class name as a more-specific type than `object`, even though it does recognize it as more specific than an untyped return value. Thus some more-specific type annotations have had to be downgraded. This is the refactoring change from #390.
Also - Update the `parse` function signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclasses to avoid contravariance. Unfortunately, there seems to be a bug in PHP 7.3 (and 7.2) whereby it doesn't recognize a specific class name as a more-specific type than `object`, even though it does recognize it as more specific than an untyped return value. Thus some more-specific type annotations have had to be downgraded. This is the refactoring change from #390.
Also - Update the `parse` function signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclasses to avoid contravariance. This is the refactoring change from #390.
Also - Update the `parse` method signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclasses to avoid contravariance. This is the refactoring change from #390.
Upon checking, I find that |
) Also - Update the `parse` method signature to use PHP7 type hints; - Remove subsequently redundant DocBlock entries; - Ditto for same-named method in subclasses to avoid contravariance. This is the refactoring change from #390. Co-authored-by: Jake Hotson <[email protected]>
|
This PR now needs a rebase. |
I already looked at rebasing, but it seemed a bit of a nightmare with the number of changes since, particularly given I'd be working on someone else's fork, and that in any case we wanted to split this up into three more atomic changes. So I am reimplementing the changes from here as fresh PRs: #599, #607, and the infinite loop fix TBD. |
|
Reopening, as there's still the infinite loop bugfix from this to be applied. |
|
Does this ticket address the lack of CSS feature clamp working correctly? Or do we need a new ticket for that? |
I'll add a test for that and get back to you... |
Good news. Yes, it does. Or at least I'm 99.99% sure - I extended the test previously added to include |
PS. What may still not work is expressions involving brackets, like @nickfmc, would you be able to try the 9.0.x-dev branch to confirm this fixes the |
Upon investigation, I find the infinite loop bug was subsequently fixed by #413. Since we want to move away from having test data in separate files, I am not going to include the additional test for this as it is. I looked at adding a more targeted test, and found a curious behaviour, which I logged as #622. Thus, I am re-closing this in favour of that. |
This is the enhancement from #390. `calc` was already supported.
This is the enhancement from #390. `calc` was already supported.
This is the enhancement from #390. `calc` was already supported. Co-authored-by: Jake Hotson <[email protected]>
Builds on top of #169