diff --git a/src/Entity/Attachments/Attachment.php b/src/Entity/Attachments/Attachment.php index 1f8ad2972..7a4a170a5 100644 --- a/src/Entity/Attachments/Attachment.php +++ b/src/Entity/Attachments/Attachment.php @@ -285,13 +285,21 @@ public function isPicture(): bool return in_array(strtolower($extension), static::PICTURE_EXTS, true); } + if ($this->hasExternal()) { //Check if we can extract a file extension from the URL $extension = pathinfo(parse_url($this->getExternalPath(), PHP_URL_PATH) ?? '', PATHINFO_EXTENSION); - //If no extension is found or it is known picture extension, we assume that this is a picture extension - return $extension === '' || in_array(strtolower($extension), static::PICTURE_EXTS, true); + if (in_array(strtolower($extension), static::PICTURE_EXTS, true)) { + return true; + } + + //If no extension is found (e.g. for URLs which redirect to the actual file), we can only guess. We assume + //that it is a picture, unless the attachment type rules pictures out (like the "Datasheet" type, which only + //allows PDFs), as we would show a broken image otherwise. + return $extension === '' && ($this->getAttachmentType()?->allowsPictures() ?? true); } + //File doesn't have an internal, nor an external copy. This shouldn't happen, but it certainly isn't a picture... return false; } diff --git a/src/Entity/Attachments/AttachmentType.php b/src/Entity/Attachments/AttachmentType.php index 03bb8031a..454ed67ff 100644 --- a/src/Entity/Attachments/AttachmentType.php +++ b/src/Entity/Attachments/AttachmentType.php @@ -191,6 +191,32 @@ public function setFiletypeFilter(string $filetype_filter): self return $this; } + /** + * Checks if this attachment type allows pictures (i.e. if the filetype filter allows image files). + * If no filetype filter is set, this method returns true, as it is assumed that all file types are allowed. + * @return bool + */ + public function allowsPictures(): bool + { + if ($this->filetype_filter === '') { + return true; + } + + foreach (explode(',', $this->filetype_filter) as $allowed) { + $allowed = strtolower(trim($allowed)); + + if ($allowed === '*' || $allowed === '*/*' || str_starts_with($allowed, 'image/')) { + return true; + } + + if (in_array(ltrim($allowed, '.'), Attachment::PICTURE_EXTS, true)) { + return true; + } + } + + return false; + } + /** * Returns a list of allowed targets as class names (e.g. PartAttachment::class), where this attachment type can be assigned to. If null, there are no restrictions. * @return class-string[]|null diff --git a/tests/Entity/Attachments/AttachmentTest.php b/tests/Entity/Attachments/AttachmentTest.php index bef2df168..7d4173ac4 100644 --- a/tests/Entity/Attachments/AttachmentTest.php +++ b/tests/Entity/Attachments/AttachmentTest.php @@ -171,6 +171,31 @@ public function testIsPicture(?string $internal_path, ?string $external_path, bo $this->assertSame($expected, $attachment->isPicture()); } + public static function pictureFiletypeFilterDataProvider(): \Iterator + { + //An URL without a file extension is only assumed to be a picture, if the attachment type allows pictures + yield ['https://invalid.com/redirect?id=1234', '', true]; + yield ['https://invalid.com/redirect?id=1234', 'image/*', true]; + yield ['https://invalid.com/redirect?id=1234', '.jpg,.png', true]; + yield ['https://invalid.com/redirect?id=1234', 'application/pdf', false]; + yield ['https://invalid.com/redirect?id=1234', '.pdf,.txt', false]; + //An URL with a picture extension is always a picture, no matter what the attachment type says + yield ['https://invalid.com/picture.jpeg', 'application/pdf', true]; + } + + #[DataProvider('pictureFiletypeFilterDataProvider')] + public function testIsPictureRespectsFiletypeFilter(string $external_path, string $filter, bool $expected): void + { + $attachment_type = new AttachmentType(); + $attachment_type->setFiletypeFilter($filter); + + $attachment = new PartAttachment(); + $attachment->setAttachmentType($attachment_type); + $this->setProtectedProperty($attachment, 'external_path', $external_path); + + $this->assertSame($expected, $attachment->isPicture()); + } + public static function builtinDataProvider(): \Iterator { yield ['', false]; diff --git a/tests/Entity/Attachments/AttachmentTypeTest.php b/tests/Entity/Attachments/AttachmentTypeTest.php index c966d23f8..dad479981 100644 --- a/tests/Entity/Attachments/AttachmentTypeTest.php +++ b/tests/Entity/Attachments/AttachmentTypeTest.php @@ -26,6 +26,7 @@ use App\Entity\Attachments\PartAttachment; use App\Entity\Attachments\UserAttachment; use Doctrine\Common\Collections\Collection; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; final class AttachmentTypeTest extends TestCase @@ -83,4 +84,33 @@ public function testIsAllowedForTarget(): void $this->assertTrue($attachmentType->isAllowedForTarget(PartAttachment::class)); $this->assertTrue($attachmentType->isAllowedForTarget(UserAttachment::class)); } + + public static function allowsPictureDataProvider(): array + { + return [ + ['', true], + ['image/*', true], + ['image/png,image/jpeg', true], + ['application/pdf', false], + ['text/plain', false], + ['image/gif,application/pdf', true], + ['audio/mpeg', false], + ['video/mp4', false], + ['image/svg+xml', true], + ['application/zip', false], + ['image/webp,text/html', true], + ['*', true], + ['.pdf,.png', true], + ['.jpeg,.gif', true], + ['.pdf,.xml', false], + ]; + } + + #[DataProvider('allowsPictureDataProvider')] + public function testAllowsPicture(string $filetype_filter, bool $expected): void + { + $attachmentType = new AttachmentType(); + $attachmentType->setFiletypeFilter($filetype_filter); + $this->assertSame($expected, $attachmentType->allowsPictures()); + } }