PHP-CSS-Parser icon indicating copy to clipboard operation
PHP-CSS-Parser copied to clipboard

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

Open JakeQZ opened this issue 8 months ago • 1 comments

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

JakeQZ avatar Mar 19 '25 00:03 JakeQZ

Coverage Status

coverage: 59.595% (-0.2%) from 59.816% when pulling e666f5e2f46f71b103b71db1f07fc3c343018198 on task/declarationblock-delegate-ruleset into b2028eb9519718a97c006c0536e0998f570b3bc7 on main.

coveralls avatar Mar 19 '25 00:03 coveralls

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

JakeQZ avatar Apr 15 '25 23:04 JakeQZ

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.

JakeQZ avatar May 08 '25 22:05 JakeQZ

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

oliverklee avatar May 09 '25 18:05 oliverklee

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

#1256.

JakeQZ avatar May 09 '25 23:05 JakeQZ

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

JakeQZ avatar Jun 27 '25 14:06 JakeQZ

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.

oliverklee avatar Jul 14 '25 07:07 oliverklee

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 avatar Jul 17 '25 22:07 JakeQZ

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.

JakeQZ avatar Jul 17 '25 23:07 JakeQZ

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.

oliverklee avatar Jul 18 '25 07:07 oliverklee

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.

JakeQZ avatar Jul 18 '25 18:07 JakeQZ

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

oliverklee avatar Jul 18 '25 20:07 oliverklee

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

JakeQZ avatar Jul 19 '25 00:07 JakeQZ

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

oliverklee avatar Jul 19 '25 07:07 oliverklee

I think this is now ready for a final review.

JakeQZ avatar Jul 21 '25 16:07 JakeQZ

This issues from the last review should all now be resolved.

JakeQZ avatar Jul 24 '25 18:07 JakeQZ