Fix broken preview images for extension-less external files

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

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<Attachment>[]|null

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

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());
+    }
 }