[TASK] Use delegation for `DeclarationBlock` -> `RuleSet` #1194

JakeQZ · 2025-03-19T00:56:25Z

... rather than inheritance.

This will allow DeclarationBlock to instead extend CSSBlockList in order to support
CSS nesting.

This is a slightly-breaking change, since now CSSBlockList::getAllRuleSets() will include the RuleSet property of the DeclarationBlock instead of the DeclarationBlock itself.

Part of #1170.

coveralls · 2025-03-19T00:58:01Z

coverage: 58.507% (-1.2%) from 59.751%
when pulling 5448e91 on task/declarationblock-delegate-ruleset
into 8c4a77e on main.

src/CSSList/CSSList.php

src/RuleSet/DeclarationBlock.php

JakeQZ · 2025-04-15T23:50:25Z

Would be good to resolve #1247 before moving on with this.

JakeQZ · 2025-05-08T22:38:33Z

This is almost ready for review, but some tests need to be added. Before doing so, I'd appreciate a pre-review from you: @sabberworm, @oliverklee.

I'm not keen on the having the various chaining methods in DeclarationBlock, but to avoid breaking changes they are necessary. A trait can be used for testing them in the same way as their counterparts in RuleSet.

For DeclarationBlock to contain nested rules, it needs to extend CSSBlockList. RuleSet cannot be a trait; it needs to be a fully-fledged type for methods like getAllRuleSets().

This is the only realistic way forward I see for now, in an 'Agile' sense, which can deliver what is required now, without wholescale refactoring or breaking changes.

config/phpstan-baseline.neon

oliverklee

I like this approach!

src/CSSList/CSSBlockList.php

src/RuleSet/DeclarationBlock.php

oliverklee · 2025-05-09T18:45:04Z

Maybe as another spin-off PR (or prepatch), we can introduce the interface and have one class implement it.

The trait provides tests for classes implementing `RuleContainer`. The test methods and data providers have been moved verbatim to the trait from `RuleSetTest`. Will be needed for #1194.

The trait provides tests for classes implementing `RuleContainer`. The test methods and data providers have been moved verbatim to the trait from `RuleSetTest`. Will be needed for #1194

JakeQZ · 2025-06-27T14:26:18Z

I think all pre-PRs that can be separated have been completed now.

oliverklee · 2025-06-28T19:12:13Z

src/RuleSet/RuleSet.php

 */
-abstract class RuleSet implements CSSElement, CSSListItem, Positionable, RuleContainer
+class RuleSet implements CSSElement, CSSListItem, Positionable, RuleContainer


I think we can move the changes to RuleSet (plus test coverage) to a pre-patch, too.

src/RuleSet/DeclarationBlock.php

tests/ParserTest.php

tests/Unit/RuleSet/DeclarationBlockTest.php

oliverklee · 2025-07-14T07:49:02Z

This PR needs a rebase.

Apart from that, I think we can move de-abstracting the RuleSet class (and creating a dedicated testcase for it by copying some existing tests) into a pre-PR.

JakeQZ · 2025-07-17T22:26:07Z

This PR needs a rebase.

It is impossible to rebase now, because git does it one commit at a time, and I probably included changes in the wrong commits in a previous rebase. I tried merge. Might be able to fix the resulting errors. Otherwise will have to start over.

JakeQZ

This needs starting over. It's a mess now.

tests/Unit/RuleSet/DeclarationBlockTest.php

JakeQZ · 2025-07-17T23:47:50Z

I tried undoing lastest merge, but it's still a mess. I can't even get back to where I was now. Will take some time redoing.

oliverklee · 2025-07-18T07:00:55Z

I tried undoing lastest merge, but it's still a mess. I can't even get back to where I was now. Will take some time redoing.

In my experience, when I had a PR from which I spun off some pre-PRs, I also sometimes had nasty merge conflicts when rebasing. In those cases, it made things a lot easier if I squashed the existing commits in the PR branch before rebasing.

... adding internal `render` method. Precursor to #1194.

JakeQZ · 2025-07-18T18:29:11Z

I tried undoing lastest merge, but it's still a mess. I can't even get back to where I was now. Will take some time redoing.

In my experience, when I had a PR from which I spun off some pre-PRs, I also sometimes had nasty merge conflicts when rebasing. In those cases, it made things a lot easier if I squashed the existing commits in the PR branch before rebasing.

I think I have somehow managed to rebase without including any changes from main. So now the diffs are showing that all the changes from main are being reverted. I did git rebase --abort when I got in a muddle.

If I could somehow reset the branch to the state it was in immediately after the commit "Reorder construction", then squashing may be possible, but I'm not sure how to do that.

oliverklee · 2025-07-18T20:12:32Z

If I could somehow reset the branch to the state it was in immediately after the commit "Reorder construction", then squashing may be possible, but I'm not sure how to do that.

git reflog allows you to list all recent commits and which actions lead to them, including rebases, branch switches etc. So this might be helpful. (It has saved my work a couple of times.)

... rather than inheritance. This will allow `DeclarationBlock` to instead extend `CSSBlockList` in order to support [CSS nesting](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_nesting). This is a slightly-breaking change, since now `CSSBlockList::getAllRuleSets()` will include the `RuleSet` property of the `DeclarationBlock` instead of the `DeclarationBlock` itself. Part of #1170.

JakeQZ · 2025-07-19T00:54:56Z

If I could somehow reset the branch to the state it was in immediately after the commit "Reorder construction"

git reset -hard {SHA-of-last-commit-before-attempted-rebase}

... undid the failed rebase/merge. I think when I tried merging instead (via GitHub), that moved the 'common ancestor' point, but when I reverted the merge, it was not moved back.

then squashing may be possible, but I'm not sure how to do that.

git reset -soft {SHA-of-first-branch-commit}
git commit --amend

... did the squashing.

In my experience, when I had a PR from which I spun off some pre-PRs, I also sometimes had nasty merge conflicts when rebasing. In those cases, it made things a lot easier if I squashed the existing commits in the PR branch before rebasing.

That helped immensely; thanks. Instead of (presumably) 9 rounds of conflict resolution, including for files with no overall conflict, there was just one round, for the two files that actually had conflicts. (I'm used to Perforce, which does the merge all at once, and also has a visual editor for 3-way merging. Keeping a cool head also helps ;))

Rebase now done !) Thanks <3

oliverklee · 2025-07-19T07:40:53Z

I'm used to Perforce, which does the merge all at once, and also has a visual editor for 3-way merging.

I use PhpStorm as my IDE, and IIRC it also have a nice 3-way-merge UI. (Disclaimer: I don't use UIs for git stuff and hence can only report what I hear and see from other people.)

JakeQZ added cleanup css4 Relating to features introduced in CSS4 labels Mar 19, 2025

JakeQZ added this to the 9.0.0: Drop support for PHP < 7.2 and clean up things milestone Mar 19, 2025

JakeQZ self-assigned this Mar 19, 2025

JakeQZ marked this pull request as draft March 19, 2025 00:56

JakeQZ mentioned this pull request Mar 19, 2025

Add support for nested selectors #1170

Open

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch from 7800603 to 73956e9 Compare March 19, 2025 02:06

oliverklee reviewed Mar 19, 2025

View reviewed changes

src/CSSList/CSSList.php Outdated Show resolved Hide resolved

oliverklee reviewed Mar 19, 2025

View reviewed changes

src/RuleSet/DeclarationBlock.php Outdated Show resolved Hide resolved

JakeQZ added css3 and removed css4 Relating to features introduced in CSS4 labels Mar 20, 2025

JakeQZ mentioned this pull request Mar 25, 2025

[TASK] Add trait providing standard implementation of Commentable #1206

Merged

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch 5 times, most recently from c1c27a9 to 3a278e0 Compare April 2, 2025 00:31

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch 5 times, most recently from c935f2c to 1f908cf Compare April 14, 2025 15:25

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch from 1f908cf to 1ce18b7 Compare May 6, 2025 17:40

JakeQZ commented May 8, 2025

View reviewed changes

config/phpstan-baseline.neon Outdated Show resolved Hide resolved

oliverklee reviewed May 9, 2025

View reviewed changes

src/CSSList/CSSBlockList.php Outdated Show resolved Hide resolved

oliverklee reviewed May 9, 2025

View reviewed changes

src/RuleSet/DeclarationBlock.php Show resolved Hide resolved

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch 2 times, most recently from a9790f1 to e123988 Compare June 26, 2025 21:40

JakeQZ mentioned this pull request Jun 26, 2025

[TASK] Add RuleContainerTest trait, use in RuleSetTest #1291

Merged

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch from 61d3317 to 17885d0 Compare June 27, 2025 12:37

JakeQZ marked this pull request as ready for review June 27, 2025 14:26

JakeQZ requested review from oliverklee and sabberworm June 27, 2025 14:26

oliverklee requested changes Jun 28, 2025

View reviewed changes

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch from bcf5cbe to 6974745 Compare June 30, 2025 00:47

JakeQZ commented Jul 17, 2025

View reviewed changes

tests/Unit/RuleSet/DeclarationBlockTest.php Outdated Show resolved Hide resolved

JakeQZ marked this pull request as draft July 17, 2025 23:27

JakeQZ added a commit that referenced this pull request Jul 18, 2025

[TASK] Make RuleSet concrete

6fdb2e3

... adding internal `render` method. Precursor to #1194.

JakeQZ mentioned this pull request Jul 18, 2025

[TASK] Make RuleSet concrete #1341

Open

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch 2 times, most recently from 6974745 to 184f865 Compare July 18, 2025 23:48

JakeQZ force-pushed the task/declarationblock-delegate-ruleset branch from 184f865 to 34c8708 Compare July 19, 2025 00:22

Update colorParsing test to use getAllDeclarationBlocks()

5448e91

[TASK] Use delegation for DeclarationBlock -> RuleSet #1194

Are you sure you want to change the base?

[TASK] Use delegation for DeclarationBlock -> RuleSet #1194

Conversation

JakeQZ commented Mar 19, 2025

Uh oh!

coveralls commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JakeQZ commented Apr 15, 2025

Uh oh!

JakeQZ commented May 8, 2025

Uh oh!

Uh oh!

oliverklee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oliverklee commented May 9, 2025

Uh oh!

JakeQZ commented Jun 27, 2025

Uh oh!

oliverklee Jun 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oliverklee commented Jul 14, 2025

Uh oh!

JakeQZ commented Jul 17, 2025

Uh oh!

JakeQZ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JakeQZ commented Jul 17, 2025

Uh oh!

oliverklee commented Jul 18, 2025

Uh oh!

JakeQZ commented Jul 18, 2025

Uh oh!

oliverklee commented Jul 18, 2025

Uh oh!

JakeQZ commented Jul 19, 2025

Uh oh!

oliverklee commented Jul 19, 2025

Uh oh!

Uh oh!

[TASK] Use delegation for `DeclarationBlock` -> `RuleSet` #1194

[TASK] Use delegation for `DeclarationBlock` -> `RuleSet` #1194

coveralls commented Mar 19, 2025 •

edited

Loading