Make internal and external path for attachments nullable, to make clear that they have no internal or external path

src/Entity/Attachments/Attachment.php

@@ -153,21 +153,21 @@ abstract class Attachment extends AbstractNamedDBElement
     protected ?string $original_filename = null;
 
     /**
-     * @var string If a copy of the file is stored internally, the path to the file relative to a placeholder
+     * @var string|null If a copy of the file is stored internally, the path to the file relative to a placeholder
      * path like %MEDIA%
      */
-    #[ORM\Column(type: Types::STRING)]
+    #[ORM\Column(type: Types::STRING, nullable: true)]
-    protected string $internal_path = '';
+    protected ?string $internal_path = null;
 
 
     /**
-     * @var string The path to the external source if the file is stored externally or was downloaded from an
+     * @var string|null The path to the external source if the file is stored externally or was downloaded from an
-     * external source
+     * external source. Null if there is no external source.
      */
-    #[ORM\Column(type: Types::STRING)]
+    #[ORM\Column(type: Types::STRING, nullable: true)]
     #[Groups(['attachment:read'])]
     #[ApiProperty(example: 'http://example.com/image.jpg')]
-    protected string $external_path = '';
+    protected ?string $external_path = null;
 
     /**
      * @var string the name of this element
@@ -297,11 +297,13 @@ abstract class Attachment extends AbstractNamedDBElement
      * Checks if this attachment has a path to an external file
      *
      * @return bool true, if there is a path to an external file
+     * @phpstan-assert-if-true non-empty-string $this->external_path
+     * @phpstan-assert-if-true non-empty-string $this->getExternalPath())
      */
     #[Groups(['attachment:read'])]
     public function hasExternal(): bool
     {
-        return $this->external_path !== '';
+        return $this->external_path !== null && $this->external_path !== '';
     }
 
     /**
@@ -309,11 +311,13 @@ abstract class Attachment extends AbstractNamedDBElement
      * Does not check if the file exists.
      *
      * @return bool true, if there is a path to an internal file
+     * @phpstan-assert-if-true non-empty-string $this->internal_path
+     * @phpstan-assert-if-true non-empty-string $this->getInternalPath())
      */
     #[Groups(['attachment:read'])]
     public function hasInternal(): bool
     {
-        return $this->internal_path !== '';
+        return $this->internal_path !== null && $this->internal_path !== '';
     }
 
     /**
@@ -326,6 +330,10 @@ abstract class Attachment extends AbstractNamedDBElement
     #[SerializedName('private')]
     public function isSecure(): bool
     {
+        if ($this->internal_path === null) {
+            return false;
+        }
+
         //After the %PLACEHOLDER% comes a slash, so we can check if we have a placeholder via explode
         $tmp = explode('/', $this->internal_path);
 
@@ -341,6 +349,10 @@ abstract class Attachment extends AbstractNamedDBElement
     #[Groups(['attachment:read'])]
     public function isBuiltIn(): bool
     {
+        if ($this->internal_path === null) {
+            return false;
+        }
+
         return static::checkIfBuiltin($this->internal_path);
     }
 
@@ -411,12 +423,12 @@ abstract class Attachment extends AbstractNamedDBElement
         return parse_url($this->getExternalPath(), PHP_URL_HOST);
     }
 
-    public function getInternalPath(): string
+    public function getInternalPath(): ?string
     {
         return $this->internal_path;
     }
 
-    public function getExternalPath(): string
+    public function getExternalPath(): ?string
     {
         return $this->external_path;
     }
@@ -516,7 +528,7 @@ abstract class Attachment extends AbstractNamedDBElement
      */
     public function setInternalPath(?string $internal_path): self
     {
-        $this->internal_path = $internal_path ?? '';
+        $this->internal_path = $internal_path;
 
         return $this;
     }
@@ -543,10 +555,8 @@ abstract class Attachment extends AbstractNamedDBElement
     If you set a new (nonempty) file path any associated internal file will be removed!')]
     public function setURL(?string $url): self
     {
-        $url = $url ?? '';
-
         //Don't allow the user to set an empty external path if the internal path is empty already
-        if ($url === '' && !$this->hasInternal()) {
+        if (($url === null || $url === "") && !$this->hasInternal()) {
             return $this;
         }
 
@@ -554,7 +564,7 @@ abstract class Attachment extends AbstractNamedDBElement
         if ($this::checkIfBuiltin($url)) {
             $this->setInternalPath($url);
             //make sure the external path isn't still pointing to something unrelated
-            $this->setExternalPath('');
+            $this->setExternalPath(null);
         } else {
             $this->setExternalPath($url);
         }
@@ -571,7 +581,7 @@ abstract class Attachment extends AbstractNamedDBElement
     {
         //If we only clear the external path, don't reset the internal path, since that could be confusing
         if($external_path === null || $external_path === '') {
-            $this->external_path = '';
+            $this->external_path = null;
             return $this;
         }
 
@@ -585,7 +595,7 @@ abstract class Attachment extends AbstractNamedDBElement
         }
 
         $this->external_path = $external_path;
-        $this->internal_path = '';
+        $this->internal_path = null;
         //Reset internal filename
         $this->original_filename = null;
 

src/Repository/AttachmentRepository.php

@@ -75,7 +75,8 @@ class AttachmentRepository extends DBElementRepository
     {
         $qb = $this->createQueryBuilder('attachment');
         $qb->select('COUNT(attachment)')
-            ->where('attachment.external_path <> \'\'');
+            ->andWhere('attaachment.internal_path IS NULL')
+            ->where('attachment.external_path IS NOT NULL');
         $query = $qb->getQuery();
 
         return (int) $query->getSingleScalarResult();

src/Services/Attachments/AttachmentSubmitHandler.php

@@ -429,7 +429,7 @@ class AttachmentSubmitHandler
         //Save the path to the attachment
         $attachment->setInternalPath($file_path);
         //reset any external paths the attachment might have had
-        $attachment->setExternalPath('');
+        $attachment->setExternalPath(null);
         //And save original filename
         $attachment->setFilename($file->getClientOriginalName());
 

tests/Entity/Attachments/AttachmentTest.php

@@ -121,24 +121,24 @@ class AttachmentTest extends TestCase
     }
 
 
-    public function extensionDataProvider(): \Iterator
+    public static function extensionDataProvider(): \Iterator
     {
         yield ['%MEDIA%/foo/bar.txt',   'http://google.de',           null,        'txt'];
         yield ['%MEDIA%/foo/bar.JPeg',  'https://foo.bar',            null,        'jpeg'];
-        yield ['%MEDIA%/foo/bar.JPeg',  '',                           'test.txt',  'txt'];
+        yield ['%MEDIA%/foo/bar.JPeg',  null,                           'test.txt',  'txt'];
         yield ['%MEDIA%/foo/bar',       'https://foo.bar/test.jpeg',  null,        ''];
         yield ['%MEDIA%/foo.bar',       'test.txt',                   'bar',       ''];
-        yield ['',                      'http://google.de',           null,        null];
+        yield [null,                      'http://google.de',           null,        null];
-        yield ['',                      'https://foo.bar',            null,        null];
+        yield [null,                      'https://foo.bar',            null,        null];
-        yield ['',                      ',https://foo.bar/test.jpeg', null,        null];
+        yield [null,                      ',https://foo.bar/test.jpeg', null,        null];
-        yield ['',                      'test',                       null,        null];
+        yield [null,                      'test',                       null,        null];
-        yield ['',                      'test.txt',                   null,        null];
+        yield [null,                      'test.txt',                   null,        null];
     }
 
     /**
      * @dataProvider extensionDataProvider
      */
-    public function testGetExtension($internal_path, $external_path, $originalFilename, $expected): void
+    public function testGetExtension(?string $internal_path, ?string $external_path, ?string $originalFilename, ?string $expected): void
     {
         $attachment = new PartAttachment();
         $this->setProtectedProperty($attachment, 'internal_path', $internal_path);
@@ -147,15 +147,15 @@ class AttachmentTest extends TestCase
         $this->assertSame($expected, $attachment->getExtension());
     }
 
-    public function pictureDataProvider(): \Iterator
+    public static function pictureDataProvider(): \Iterator
     {
-        yield ['',                        '%MEDIA%/foo/bar.txt',                               false];
+        yield [null,                        '%MEDIA%/foo/bar.txt',                               false];
-        yield ['',                        'https://test.de/picture.jpeg',                      true];
+        yield [null,                        'https://test.de/picture.jpeg',                      true];
-        yield ['',                        'https://test.de/picture.png?test=fdsj&width=34',    true];
+        yield [null,                        'https://test.de/picture.png?test=fdsj&width=34',    true];
-        yield ['',                        'https://invalid.invalid/file.txt',                  false];
+        yield [null,                        'https://invalid.invalid/file.txt',                  false];
-        yield ['',                        'http://infsf.inda/file.zip?test',                   false];
+        yield [null,                        'http://infsf.inda/file.zip?test',                   false];
-        yield ['',                        'https://test.de',                                   true];
+        yield [null,                        'https://test.de',                                   true];
-        yield ['',                        'https://invalid.com/invalid/pic',                   true];
+        yield [null,                        'https://invalid.com/invalid/pic',                   true];
         yield ['%MEDIA%/foo/bar.jpeg',    'https://invalid.invalid/file.txt',                  true];
         yield ['%MEDIA%/foo/bar.webp',    '',                                                  true];
         yield ['%MEDIA%/foo',             '',                                                  false];
@@ -165,7 +165,7 @@ class AttachmentTest extends TestCase
     /**
      * @dataProvider pictureDataProvider
      */
-    public function testIsPicture($internal_path, $external_path, $expected): void
+    public function testIsPicture(?string $internal_path, ?string $external_path, bool $expected): void
     {
         $attachment = new PartAttachment();
         $this->setProtectedProperty($attachment, 'internal_path', $internal_path);
@@ -173,9 +173,10 @@ class AttachmentTest extends TestCase
         $this->assertSame($expected, $attachment->isPicture());
     }
 
-    public function builtinDataProvider(): \Iterator
+    public static function builtinDataProvider(): \Iterator
     {
         yield ['', false];
+        yield [null, false];
         yield ['%MEDIA%/foo/bar.txt', false];
         yield ['%BASE%/foo/bar.txt', false];
         yield ['/', false];
@@ -186,14 +187,14 @@ class AttachmentTest extends TestCase
     /**
      * @dataProvider builtinDataProvider
      */
-    public function testIsBuiltIn($path, $expected): void
+    public function testIsBuiltIn(?string $path, $expected): void
     {
         $attachment = new PartAttachment();
         $this->setProtectedProperty($attachment, 'internal_path', $path);
         $this->assertSame($expected, $attachment->isBuiltIn());
     }
 
-    public function hostDataProvider(): \Iterator
+    public static function hostDataProvider(): \Iterator
     {
         yield ['%MEDIA%/foo/bar.txt', null];
         yield ['https://www.google.de/test.txt', 'www.google.de'];
@@ -203,24 +204,25 @@ class AttachmentTest extends TestCase
     /**
      * @dataProvider hostDataProvider
      */
-    public function testGetHost($path, $expected): void
+    public function testGetHost(?string $path, ?string $expected): void
     {
         $attachment = new PartAttachment();
         $this->setProtectedProperty($attachment, 'external_path', $path);
         $this->assertSame($expected, $attachment->getHost());
     }
 
-    public function filenameProvider(): \Iterator
+    public static function filenameProvider(): \Iterator
     {
         yield ['%MEDIA%/foo/bar.txt',  'https://www.google.de/test.txt',   null,      'bar.txt'];
         yield ['%MEDIA%/foo/bar.JPeg', 'https://www.google.de/foo.txt',   'test.txt', 'test.txt'];
         yield ['',                     'https://www.google.de/test.txt',   null,       null];
+        yield [null,                     'https://www.google.de/test.txt',   null,       null];
     }
 
     /**
      * @dataProvider filenameProvider
      */
-    public function testGetFilename($internal_path, $external_path, $original_filename, $expected): void
+    public function testGetFilename(?string $internal_path, ?string $external_path, ?string $original_filename, ?string $expected): void
     {
         $attachment = new PartAttachment();
         $this->setProtectedProperty($attachment, 'internal_path', $internal_path);
@@ -240,7 +242,7 @@ class AttachmentTest extends TestCase
         //Ensure that changing the external path does reset the internal one
         $attachment->setInternalPath('%MEDIA%/foo/bar.txt');
         $attachment->setExternalPath('https://example.de');
-        $this->assertSame('', $attachment->getInternalPath());
+        $this->assertSame(null, $attachment->getInternalPath());
 
         //Ensure that setting the same value to the external path again doesn't reset the internal one
         $attachment->setExternalPath('https://google.de');