From db10b272f99ce01dda4837077aff06e5ee801db8 Mon Sep 17 00:00:00 2001 From: William Gotti Date: Thu, 24 Sep 2026 13:55:10 +0200 Subject: [PATCH 1/4] Merge rule collections instead of replacing them RuleAggregate::addCollections() array_merged two arrays keyed by registrable domain. String keys are overwritten rather than appended, so when a second call carried rules for a domain an earlier call had already contributed to, the earlier collection was dropped entirely. This is reachable through createFromFiles(), which calls addCollections() once per file: any domain listed in more than one file kept only the rules of the file read last. It also made exceptions unreliable, since an @@ rule in one file would silently replace the blockers of another instead of overriding them. Add the incoming rules to the existing collection instead. Routing them through RuleCollection::addRule() keeps exceptions and blockers in their own buckets, so getRulesToApplyForDomain() still returns exceptions first. Also ignore the caches and the coverage report the test tooling writes. --- .gitignore | 3 ++ src/RuleAggregate.php | 18 +++++-- tests/RuleAggregateFactoryTest.php | 16 +++++++ tests/RuleAggregateTest.php | 69 +++++++++++++++++++++++++++ tests/test-rules-shared-blocker.txt | 2 + tests/test-rules-shared-exception.txt | 2 + 6 files changed, 106 insertions(+), 4 deletions(-) create mode 100644 tests/RuleAggregateTest.php create mode 100644 tests/test-rules-shared-blocker.txt create mode 100644 tests/test-rules-shared-exception.txt diff --git a/.gitignore b/.gitignore index 7579f74..6230d1f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,2 +1,5 @@ vendor composer.lock +.phpunit.cache +.php-cs-fixer.cache +report diff --git a/src/RuleAggregate.php b/src/RuleAggregate.php index 48395a9..45f3811 100644 --- a/src/RuleAggregate.php +++ b/src/RuleAggregate.php @@ -53,14 +53,24 @@ public function getAllRules(): array } /** + * Rules of an already known domain are added to its existing collection. Replacing the collection + * instead would drop every rule a previous call contributed for that domain. + * * @param array $collections */ public function addCollections(array $collections): void { - $this->ruleCollections = array_merge( - $this->ruleCollections, - $collections, - ); + foreach ($collections as $domainIdentifier => $collection) { + if (!isset($this->ruleCollections[$domainIdentifier])) { + $this->ruleCollections[$domainIdentifier] = $collection; + + continue; + } + + foreach ($collection->getAllRules() as $rule) { + $this->ruleCollections[$domainIdentifier]->addRule($rule); + } + } } /** diff --git a/tests/RuleAggregateFactoryTest.php b/tests/RuleAggregateFactoryTest.php index 038998c..e10ccae 100644 --- a/tests/RuleAggregateFactoryTest.php +++ b/tests/RuleAggregateFactoryTest.php @@ -7,6 +7,7 @@ use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use ProtonLabs\AdblockParser\DomainParserInterface; +use ProtonLabs\AdblockParser\DummyDomainParser; use ProtonLabs\AdblockParser\Rule; use ProtonLabs\AdblockParser\RuleAggregateFactory; use ProtonLabs\AdblockParser\RuleApplier; @@ -38,6 +39,21 @@ public function testCreateRuleAggregate(): void ->shouldBlock('http://example.com//avmws_exception.js', $ruleAggregate)); } + public function testCreateFromFilesKeepsRulesOfEveryFileForASharedDomain(): void + { + $factory = new RuleAggregateFactory(new RuleFactory(new DummyDomainParser())); + + $ruleAggregate = $factory->createFromFiles([ + __DIR__ . '/test-rules-shared-blocker.txt', + __DIR__ . '/test-rules-shared-exception.txt', + ]); + + $applier = new RuleApplier(new DummyDomainParser()); + + Assert::assertTrue($applier->shouldBlock('http://shared-example.com/blocked.gif', $ruleAggregate)); + Assert::assertFalse($applier->shouldBlock('http://shared-example.com/allowed.gif', $ruleAggregate)); + } + public function createRuleApplier(): RuleApplier { $domainParser = $this->createMock(DomainParserInterface::class); diff --git a/tests/RuleAggregateTest.php b/tests/RuleAggregateTest.php new file mode 100644 index 0000000..dc50d3e --- /dev/null +++ b/tests/RuleAggregateTest.php @@ -0,0 +1,69 @@ + $this->collectionOf(new Rule('first', false, 'example.com')), + ]); + + $ruleAggregate->addCollections([ + 'example.com' => $this->collectionOf(new Rule('second', false, 'example.com')), + ]); + + Assert::assertSame( + ['first', 'second'], + array_map( + static fn (Rule $rule): ?string => $rule->getRegex(), + $ruleAggregate->getRuleCollections()['example.com']->getBlockers(), + ), + ); + } + + public function testAddCollectionsKeepsExceptionsAndBlockersOfTheSameDomain(): void + { + $ruleAggregate = new RuleAggregate([ + 'example.com' => $this->collectionOf(new Rule('blocker', false, 'example.com')), + ]); + + $ruleAggregate->addCollections([ + 'example.com' => $this->collectionOf(new Rule('exception', true, 'example.com')), + ]); + + $collection = $ruleAggregate->getRuleCollections()['example.com']; + Assert::assertCount(1, $collection->getBlockers()); + Assert::assertCount(1, $collection->getExceptions()); + } + + public function testAddCollectionsAddsUnknownDomains(): void + { + $ruleAggregate = new RuleAggregate([ + 'example.com' => $this->collectionOf(new Rule('first', false, 'example.com')), + ]); + + $ruleAggregate->addCollections([ + 'other.com' => $this->collectionOf(new Rule('second', false, 'other.com')), + ]); + + Assert::assertSame(['example.com', 'other.com'], array_keys($ruleAggregate->getRuleCollections())); + } + + private function collectionOf(Rule $rule): RuleCollection + { + $collection = new RuleCollection(); + $collection->addRule($rule); + + return $collection; + } +} diff --git a/tests/test-rules-shared-blocker.txt b/tests/test-rules-shared-blocker.txt new file mode 100644 index 0000000..270cb4e --- /dev/null +++ b/tests/test-rules-shared-blocker.txt @@ -0,0 +1,2 @@ +! Blocks a path on a domain that the other file also has rules for +||shared-example.com/blocked.gif diff --git a/tests/test-rules-shared-exception.txt b/tests/test-rules-shared-exception.txt new file mode 100644 index 0000000..40fb031 --- /dev/null +++ b/tests/test-rules-shared-exception.txt @@ -0,0 +1,2 @@ +! Allows a different path on the same domain +@@||shared-example.com/allowed.gif From 1438113d72ba84655cd11c93138beef1be660f43 Mon Sep 17 00:00:00 2001 From: William Gotti Date: Thu, 24 Sep 2026 14:20:24 +0200 Subject: [PATCH 2/4] Test against PHP 8.3 to 8.5 and make the style check pass The matrix only covered 8.1 and 8.2, while consumers of this package already run 8.5. Add 8.3, 8.4 and 8.5. The suite and php-cs-fixer were both verified against 8.4 and 8.5 before widening the matrix. Publish the coverage report from a single leg. Every leg produces the same report and uploads it under the same artifact name, so five of them would race where two already did. The style check was already failing on master, over an array indentation and a @param naming an argument that does not exist. --- .github/workflows/master.yml | 5 ++++- tests/RuleApplierTest.php | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index e853c80..6cfad52 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -15,7 +15,7 @@ jobs: strategy: matrix: - php-version: [ '8.1', '8.2' ] + php-version: [ '8.1', '8.2', '8.3', '8.4', '8.5' ] steps: - name: Setup PHP @@ -48,7 +48,10 @@ jobs: - name: Check code style run: composer run-script lint-codestyle + # Every leg produces the same report and they all upload under the same artifact + # name, so publish from a single version. - name: Archive code coverage results + if: matrix.php-version == '8.4' uses: actions/upload-pages-artifact@v1 with: path: report/ diff --git a/tests/RuleApplierTest.php b/tests/RuleApplierTest.php index e859057..ea6a4e4 100644 --- a/tests/RuleApplierTest.php +++ b/tests/RuleApplierTest.php @@ -40,9 +40,9 @@ public function testBlockByAddressParts(): void ); $this->shouldNotBlock( [ - 'http://example.com/banner/img', - 'http://example.com/banner/foo/imgraph', - 'http://example.com/banner/foo/img.gif', + 'http://example.com/banner/img', + 'http://example.com/banner/foo/imgraph', + 'http://example.com/banner/foo/img.gif', ], $ruleAggregate, ); @@ -197,7 +197,7 @@ public function testParserException(): void } /** - * @param array $url + * @param array $urls */ private function shouldBlock(array $urls, RuleAggregate $ruleAggregate): void { From 285cf4de7668ec1720325c15633587a3d43030f2 Mon Sep 17 00:00:00 2001 From: William Gotti Date: Thu, 24 Sep 2026 15:21:08 +0200 Subject: [PATCH 3/4] Update the Pages actions to versions that still run upload-pages-artifact@v1 calls upload-artifact@v3 internally, which GitHub now fails outright, so every matrix job errored before running anything. Bump it to v3, which uses upload-artifact@v4, and deploy-pages to v4 to match on the download side. configure-pages goes to v5 for the same reason. --- .github/workflows/master.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 6cfad52..69319cc 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -25,7 +25,7 @@ jobs: coverage: xdebug - name: Setup Pages - uses: actions/configure-pages@v3 + uses: actions/configure-pages@v5 - uses: actions/checkout@v3 @@ -52,7 +52,7 @@ jobs: # name, so publish from a single version. - name: Archive code coverage results if: matrix.php-version == '8.4' - uses: actions/upload-pages-artifact@v1 + uses: actions/upload-pages-artifact@v3 with: path: report/ @@ -76,4 +76,4 @@ jobs: steps: - name: Deploy to GitHub Pages id: deployment - uses: actions/deploy-pages@v2 + uses: actions/deploy-pages@v4 From f22cb2b1f19d14398072569d4a698c0857a0c882 Mon Sep 17 00:00:00 2001 From: William Gotti Date: Thu, 24 Sep 2026 15:26:13 +0200 Subject: [PATCH 4/4] Only deploy the coverage pages from master The github-pages environment is protected to master, so the deploy job was rejected on every pull request with "Branch refs/pull/N/merge is not allowed to deploy to github-pages". The build job still uploads the artifact on pull requests, so that step stays covered before a change reaches master. --- .github/workflows/master.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 69319cc..f367788 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -61,6 +61,8 @@ jobs: needs: build + if: github.ref == 'refs/heads/master' + environment: name: github-pages url: ${{ steps.deployment.outputs.page_url }}