Merge rule collections instead of replacing them

.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
@@ -25,7 +25,7 @@ jobs:
         coverage: xdebug
 
     - name: Setup Pages
-      uses: actions/configure-pages@v3
+      uses: actions/configure-pages@v5
 
     - uses: actions/checkout@v3
 
@@ -48,8 +48,11 @@ 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
-      uses: actions/upload-pages-artifact@v1
+      if: matrix.php-version == '8.4'
+      uses: actions/upload-pages-artifact@v3
       with:
         path: report/
 
@@ -58,6 +61,8 @@ jobs:
 
     needs: build
 
+    if: github.ref == 'refs/heads/master'
+
     environment:
       name: github-pages
       url: ${{ steps.deployment.outputs.page_url }}
@@ -73,4 +78,4 @@ jobs:
     steps:
       - name: Deploy to GitHub Pages
         id: deployment
-        uses: actions/deploy-pages@v2
+        uses: actions/deploy-pages@v4

.gitignore

@@ -1,2 +1,5 @@
 vendor
 composer.lock
+.phpunit.cache
+.php-cs-fixer.cache
+report

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<string,RuleCollection> $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);
+            }
+        }
     }
 
     /**

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

tests/RuleAggregateTest.php

@@ -0,0 +1,69 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ProtonLabs\AdblockParser\Tests;
+
+use PHPUnit\Framework\Assert;
+use PHPUnit\Framework\TestCase;
+use ProtonLabs\AdblockParser\Rule;
+use ProtonLabs\AdblockParser\RuleAggregate;
+use ProtonLabs\AdblockParser\RuleCollection;
+
+class RuleAggregateTest extends TestCase
+{
+    public function testAddCollectionsKeepsRulesAlreadyKnownForTheSameDomain(): void
+    {
+        $ruleAggregate = new RuleAggregate([
+            'example.com' => $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;
+    }
+}

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<string> $url
+     * @param array<string> $urls
      */
     private function shouldBlock(array $urls, RuleAggregate $ruleAggregate): void
     {

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

tests/test-rules-shared-exception.txt

@@ -0,0 +1,2 @@
+! Allows a different path on the same domain
+@@||shared-example.com/allowed.gif