fix: Argument #1 ($ip) must be of type string, null given (#52)

* fix: Argument #1 () must be of type string, null given

* Apply fixes from StyleCI

* chore: enable testing

---------

Co-authored-by: StyleCI Bot <[email protected]>

.github/workflows/backend.yml

@@ -6,8 +6,8 @@ jobs:
   run:
     uses: flarum/framework/.github/workflows/[email protected]
     with:
-      enable_backend_testing: false
+      enable_backend_testing: true
       enable_phpstan: true
-      php_versions: '["8.0", "8.1", "8.2", "8.3"]'
+      php_versions: '["8.0", "8.1", "8.2", "8.3", "8.4"]'
 
       backend_directory: .

.gitignore

@@ -2,3 +2,4 @@ node_modules
 vendor
 composer.lock
 js/dist
+.phpunit.result.cache

composer.json

@@ -58,20 +58,38 @@
         },
         "flarum-cli": {
             "modules": {
-                "githubActions": true
+                "githubActions": true,
+                "backendTesting": true
             }
         }
     },
     "require-dev": {
         "flarum/phpstan": "*",
         "fof/drafts": "*",
-        "fof/default-user-preferences": "*"
+        "fof/default-user-preferences": "*",
+        "flarum/testing": "^1.0.0"
     },
     "scripts": {
         "analyse:phpstan": "phpstan analyse",
-        "clear-cache:phpstan": "phpstan clear-result-cache"
+        "clear-cache:phpstan": "phpstan clear-result-cache",
+        "test": [
+            "@test:unit",
+            "@test:integration"
+        ],
+        "test:unit": "phpunit -c tests/phpunit.unit.xml",
+        "test:integration": "phpunit -c tests/phpunit.integration.xml",
+        "test:setup": "@php tests/integration/setup.php"
     },
     "scripts-descriptions": {
-        "analyse:phpstan": "Run static analysis"
+        "analyse:phpstan": "Run static analysis",
+        "test": "Runs all tests.",
+        "test:unit": "Runs all unit tests.",
+        "test:integration": "Runs all integration tests.",
+        "test:setup": "Sets up a database for use with integration tests. Execute this only once."
+    },
+    "autoload-dev": {
+        "psr-4": {
+            "FoF\\GeoIP\\Tests\\": "tests/"
+        }
     }
 }

src/Listeners/RetrieveIP.php

@@ -40,9 +40,9 @@ public function subscribe(Dispatcher $events)
         $events->listen(PostSaving::class, [$this, 'handlePost']);
     }
 
-    public function retrieveIP(string $ip): void
+    public function retrieveIP(?string $ip): void
     {
-        if ($this->geo->isValidIP($ip) && !$this->geo->recordExistsForIP($ip)) {
+        if ($ip !== null && $this->geo->isValidIP($ip) && !$this->geo->recordExistsForIP($ip)) {
             $this->queue->push(new Jobs\RetrieveIP($ip));
         }
     }

tests/integration/Api/NullIPTest.php

@@ -0,0 +1,114 @@
+<?php
+
+/*
+ * This file is part of fof/geoip.
+ *
+ * Copyright (c) FriendsOfFlarum.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FoF\GeoIP\Tests\integration\Api;
+
+use Carbon\Carbon;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+
+class NullIPTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        $this->extension('fof-geoip');
+
+        $this->prepareDatabase([
+            'users' => [
+                $this->normalUser(),
+            ],
+            'discussions' => [
+                ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'last_posted_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1],
+            ],
+            'posts' => [
+                ['id' => 1, 'discussion_id' => 1, 'created_at' => Carbon::createFromDate(1975, 5, 21)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '<t><p>foo bar</p></t>', 'ip_address' => null],
+            ],
+        ]);
+    }
+
+    public function userTypes(): array
+    {
+        return [
+            [null],
+            [1],
+            [2],
+        ];
+    }
+
+    /**
+     * @test
+     *
+     * @dataProvider userTypes
+     */
+    public function can_show_discussion_with_null_ip(?int $userId)
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/discussions/1', [
+                'authenticatedAs' => $userId,
+            ])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+        $data = json_decode($response->getBody(), true);
+
+        $included = $data['included'];
+
+        $posts = array_values(array_filter($included, function ($item) {
+            return $item['type'] === 'posts';
+        }));
+
+        $firstPost = $posts[0];
+
+        $this->assertEquals('posts', $firstPost['type']);
+        $this->assertEquals('<p>foo bar</p>', $firstPost['attributes']['contentHtml']);
+
+        // In this test scenario, only user Id 1 (admin) should be able to see the IP address
+        if ($userId === 1) {
+            $this->assertArrayHasKey('ipAddress', $firstPost['attributes'], 'IP address should be visible');
+            $this->assertNull($firstPost['attributes']['ipAddress']);
+        } else {
+            $this->assertArrayNotHasKey('ipAddress', $firstPost['attributes'], 'IP address should not be visible');
+        }
+    }
+
+    /**
+     * @test
+     */
+    public function can_edit_post_with_null_ip()
+    {
+        $response = $this->send(
+            $this->request('PATCH', '/api/posts/1', [
+                'authenticatedAs' => 1,
+                'json'            => [
+                    'data' => [
+                        'type'       => 'posts',
+                        'id'         => '1',
+                        'attributes' => [
+                            'content' => 'foo bar - edited',
+                        ],
+                    ],
+                ],
+            ])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+        $data = json_decode($response->getBody(), true);
+
+        $this->assertEquals('posts', $data['data']['type']);
+        $this->assertEquals('foo bar - edited', $data['data']['attributes']['contentHtml']);
+        $this->assertArrayHasKey('ipAddress', $data['data']['attributes']);
+        $this->assertNull($data['data']['attributes']['ipAddress']);
+    }
+}

tests/integration/setup.php

@@ -0,0 +1,18 @@
+<?php
+
+/*
+ * This file is part of fof/geoip.
+ *
+ * Copyright (c) FriendsOfFlarum.
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+use Flarum\Testing\integration\Setup\SetupScript;
+
+require __DIR__.'/../../vendor/autoload.php';
+
+$setup = new SetupScript();
+
+$setup->run();

tests/phpunit.integration.xml

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
+    backupGlobals="false"
+    backupStaticAttributes="false"
+    colors="true"
+    convertErrorsToExceptions="true"
+    convertNoticesToExceptions="true"
+    convertWarningsToExceptions="true"
+    processIsolation="true"
+    stopOnFailure="false"
+>
+    <coverage processUncoveredFiles="true">
+        <include>
+            <directory suffix=".php">../src/</directory>
+        </include>
+    </coverage>
+    <testsuites>
+        <testsuite name="Flarum Integration Tests">
+            <directory suffix="Test.php">./integration</directory>
+             <exclude>./integration/tmp</exclude>
+        </testsuite>
+    </testsuites>
+</phpunit>

tests/phpunit.unit.xml

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
+    backupGlobals="false"
+    backupStaticAttributes="false"
+    colors="true"
+    convertErrorsToExceptions="true"
+    convertNoticesToExceptions="true"
+    convertWarningsToExceptions="true"
+    processIsolation="false"
+    stopOnFailure="false"
+>
+    <coverage processUncoveredFiles="true">
+        <include>
+            <directory suffix=".php">../src/</directory>
+        </include>
+    </coverage>
+    <testsuites>
+        <testsuite name="Flarum Unit Tests">
+            <directory suffix="Test.php">./unit</directory>
+        </testsuite>
+    </testsuites>
+    <listeners>
+        <listener class="\Mockery\Adapter\Phpunit\TestListener" />
+    </listeners>
+</phpunit>