From bb269d08a9208b8f0df618b3bb6455c73dd79c30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20B=C3=B6hmer?= Date: Thu, 20 Feb 2025 22:49:22 +0100 Subject: [PATCH] Make internal and external path for attachments nullable, to make clear that they have no internal or external path --- src/Entity/Attachments/Attachment.php | 46 ++++++++++------- src/Repository/AttachmentRepository.php | 3 +- .../Attachments/AttachmentSubmitHandler.php | 2 +- tests/Entity/Attachments/AttachmentTest.php | 50 ++++++++++--------- 4 files changed, 57 insertions(+), 44 deletions(-) diff --git a/src/Entity/Attachments/Attachment.php b/src/Entity/Attachments/Attachment.php index 3d92bb98..9518d2f9 100644 --- a/src/Entity/Attachments/Attachment.php +++ b/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)] - protected string $internal_path = ''; + #[ORM\Column(type: Types::STRING, nullable: true)] + protected ?string $internal_path = null; /** - * @var string The path to the external source if the file is stored externally or was downloaded from an - * external source + * @var string|null The path to the external source if the file is stored externally or was downloaded from an + * 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; diff --git a/src/Repository/AttachmentRepository.php b/src/Repository/AttachmentRepository.php index a4969bc0..0a6b1db2 100644 --- a/src/Repository/AttachmentRepository.php +++ b/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(); diff --git a/src/Services/Attachments/AttachmentSubmitHandler.php b/src/Services/Attachments/AttachmentSubmitHandler.php index d079644e..fc54dd2f 100644 --- a/src/Services/Attachments/AttachmentSubmitHandler.php +++ b/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()); diff --git a/tests/Entity/Attachments/AttachmentTest.php b/tests/Entity/Attachments/AttachmentTest.php index 1c21abb0..bac28fd4 100644 --- a/tests/Entity/Attachments/AttachmentTest.php +++ b/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 ['', 'https://foo.bar', null, null]; - yield ['', ',https://foo.bar/test.jpeg', null, null]; - yield ['', 'test', null, null]; - yield ['', 'test.txt', null, null]; + yield [null, 'http://google.de', null, null]; + yield [null, 'https://foo.bar', null, null]; + yield [null, ',https://foo.bar/test.jpeg', null, null]; + yield [null, 'test', 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 ['', 'https://test.de/picture.jpeg', true]; - yield ['', 'https://test.de/picture.png?test=fdsj&width=34', true]; - yield ['', 'https://invalid.invalid/file.txt', false]; - yield ['', 'http://infsf.inda/file.zip?test', false]; - yield ['', 'https://test.de', true]; - yield ['', 'https://invalid.com/invalid/pic', true]; + yield [null, '%MEDIA%/foo/bar.txt', false]; + yield [null, 'https://test.de/picture.jpeg', true]; + yield [null, 'https://test.de/picture.png?test=fdsj&width=34', true]; + yield [null, 'https://invalid.invalid/file.txt', false]; + yield [null, 'http://infsf.inda/file.zip?test', false]; + yield [null, 'https://test.de', 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');