Feat: attachement endpoint

[TatevikGr]

Summary by CodeRabbit

• New Features

  • Added an attachment download endpoint that streams files with correct headers (Content-Type, Content-Disposition, Content-Length).

• Bug Fixes

  • Consolidated and standardized JSON error responses with proper HTTP statuses (404 for missing attachments/subscribers/files, 403 for access denied, others mapped appropriately).

• Tests

  • Added integration tests and fixtures covering successful streaming downloads, header/body validation, and not-found/error scenarios.

Thanks for contributing to phpList!

[coderabbitai]

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'checks'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

📝 Walkthrough

Walkthrough

Adds attachment download functionality via a new AttachmentController exposing GET /attachments/download/{id}?uid=..., which streams files in 8192-byte chunks with proper Content-Type/Content-Disposition and optional Content-Length using AttachmentDownloadService. ExceptionListener is refactored to use an EXCEPTION_STATUS_MAP and adds specific handling for AttachmentFileNotFoundException and SubscriberNotFoundException (404). Adds integration tests and a Doctrine fixture for attachments. composer.json updates phplist/core to dev-feat/attachment.

Sequence Diagram

sequenceDiagram
    actor Client
    participant Controller as AttachmentController
    participant Service as AttachmentDownloadService
    participant DB as Database
    participant FS as FileSystem
    participant Response as StreamedResponse

    Client->>Controller: GET /attachments/download/{id}?uid=...
    Controller->>Controller: authenticate & validate request
    Controller->>DB: load Attachment by id
    DB-->>Controller: Attachment entity
    Controller->>Service: loadDownloadPayload(attachment, uid)
    Service->>DB: verify subscriber/uid
    DB-->>Service: verification result
    Service->>FS: open attachment file
    FS-->>Service: file stream + metadata
    Service-->>Controller: DownloadPayload(stream, mime, filename, size?)
    Controller->>Response: set headers (Content-Type, Content-Disposition, Content-Length?)
    Controller->>Response: stream file in 8192-byte chunks (flush)
    Response-->>Client: 200 OK + file content

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title mentions 'attachement endpoint' which is the core feature added (AttachmentController with download endpoint), but contains a typo ('attachement' instead of 'attachment').

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/attachement-endpoint

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

src/Common/EventListener/ExceptionListener.php (1)

60-66: Consider masking internal error details in 500 responses

Returning $exception->getMessage() verbatim for unhandled exceptions could leak internal details (class names, DB errors, file paths) to API consumers. A generic message like "Internal server error" is safer for production, with the real message logged server-side instead. No rush — just something to keep in mind.

Suggested tweak

         if ($exception instanceof Exception) {
             $event->setResponse(
                 new JsonResponse([
-                    'message' => $exception->getMessage()
+                    'message' => 'Internal server error'
                 ], 500)
             );
         }

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@composer.json`:
- Line 45: The composer.json currently pins phplist/core to the temporary branch
"dev-feat/attachment"; before merging, change that version constraint (the
"phplist/core" entry) to the intended long-term target (e.g., "dev-dev" or a
specific tagged release), update the composer.lock by running composer
update/install, commit the updated composer.json and composer.lock, and ensure
CI passes so the build no longer depends on the feature branch.

🧹 Nitpick comments (2)

src/Common/EventListener/ExceptionListener.php (1)

72-82: Consider consolidating these two 404 handlers & fix the double space.

Both AttachmentFileNotFoundException and SubscriberNotFoundException produce identical 404 responses. You could combine them to reduce duplication. Also, line 82 has a double space before elseif.

♻️ Optional consolidation

-        } elseif ($exception instanceof AttachmentFileNotFoundException) {
-            $response = new JsonResponse([
-                'message' => $exception->getMessage(),
-            ], 404);
-            $event->setResponse($response);
-        } elseif ($exception instanceof SubscriberNotFoundException) {
-            $response = new JsonResponse([
-                'message' => $exception->getMessage(),
-            ], 404);
-            $event->setResponse($response);
-        }  elseif ($exception instanceof Exception) {
+        } elseif ($exception instanceof AttachmentFileNotFoundException
+            || $exception instanceof SubscriberNotFoundException
+        ) {
+            $response = new JsonResponse([
+                'message' => $exception->getMessage(),
+            ], 404);
+            $event->setResponse($response);
+        } elseif ($exception instanceof Exception) {

src/Messaging/Controller/AttachmentController.php (1)

77-92: Stream is never closed after reading.

Once the while-loop drains the stream, $downloadable->content is left open. If the service returns a fopen-backed stream, the handle leaks until GC. Explicitly closing it is a small but good habit.

🔧 Suggested fix

             while (!$stream->eof()) {
                 echo $stream->read(8192);
                 flush();
             }
+
+            $stream->close();