Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Mail attachment download route breaks in different tenancy configurations #12

Closed
faizananwerali opened this issue Jan 12, 2025 · 3 comments · Fixed by #15
Closed
Assignees
Labels
bug Something isn't working

Comments

@faizananwerali
Copy link

faizananwerali commented Jan 12, 2025

What happened?

The MailDownloadController doesn't properly handle different tenancy configurations in Filament v3. The controller assumes a specific URL pattern, but Filament v3 supports multiple tenancy URL patterns which need to be handled appropriately.

When trying to access mail attachments, the following error occurs:

Too few arguments to function Vormkracht10\FilamentMails\Controllers\MailDownloadController::__invoke(), 3 passed in /Users/faizanrupani/LaravelValetProjects/vec-backend/vendor/laravel/framework/src/Illuminate/Routing/Controller.php on line 54 and exactly 4 expected

This affects three different URL patterns that need to be supported:

  1. Without tenancy:

    https://admin-dev.vec.test/mails/4/attachment/1/test1.txt
    
  2. With path-based tenancy:

    https://admin-dev.vec.test/customers/mails/4/attachment/1/test1.txt
    
  3. With domain-based tenancy:

    https://customers-dev.vec.test/mails/4/attachment/1/test1.txt
    

Current implementation:

// Route definition
Route::get('mails/{mail}/attachment/{attachment}/{filename}', MailDownloadController::class)
    ->name('mails.attachment.download');

// Controller
public function __invoke(string $tenant, string $mail, string $attachment, string $filename)
{
    /** @var MailAttachment $attachment */
    $attachment = MailAttachment::find($attachment);

    $file = Storage::disk($attachment->disk)->path($attachment->storagePath);

    return response()->download(
        file: $file,
        name: $filename,
        headers: [
            'Content-Type' => $attachment->mime,
        ]
    );
}

How to reproduce the bug

  1. Install filament-mails package in a Laravel application with Filament v3

  2. Create a new mail record with an attachment

  3. Try to access the attachment using any of these configurations:

    Without Tenancy

    composer require filament/filament:"^3.0"
    # Configure Filament without tenancy
    # Try accessing: /mails/{id}/attachment/{attachment}/{filename}

    With Path-based Tenancy

    composer require filament/filament:"^3.0"
    # Configure Filament with path-based tenancy
    # Try accessing: /{tenant}/mails/{id}/attachment/{attachment}/{filename}

    With Domain-based Tenancy

    composer require filament/filament:"^3.0"
    # Configure Filament with domain-based tenancy
    # Try accessing: mails/{id}/attachment/{attachment}/{filename} on tenant subdomain
  4. The request will fail with an argument count error

Package Version

2.0.0

PHP Version

8.2

Laravel Version

11.x

Which operating systems does with happen with?

Linux

Notes

This bug affects the mail attachment download functionality in all tenancy configurations. The solution needs to work seamlessly with:

  • Non-tenanted applications
  • Path-based tenancy
  • Domain-based tenancy

These are core tenancy configurations supported by Filament v3, and the package should handle all of them correctly.

@faizananwerali faizananwerali added the bug Something isn't working label Jan 12, 2025
@Baspa
Copy link
Member

Baspa commented Jan 12, 2025

@faizananwerali can you tell me which specific error you got?

@faizananwerali
Copy link
Author

faizananwerali commented Jan 12, 2025

I've setup my filament app without tenets. When downloading the attachments, got error.

Too few arguments to function Vormkracht10\FilamentMails\Controllers\MailDownloadController::__invoke(), 3 passed in /Users/faizanrupani/LaravelValetProjects/vec-backend/vendor/laravel/framework/src/Illuminate/Routing/Controller.php on line 54 and exactly 4 expected

Url of file: https://admin-dev.vec.test/mails/5/attachment/3/test1.txt

Screenshot 2025-01-12 at 5 13 29 PM Screenshot 2025-01-12 at 5 13 39 PM

@Baspa Baspa self-assigned this Jan 13, 2025
@Baspa
Copy link
Member

Baspa commented Jan 13, 2025

Thanks! Will take a look at it today @faizananwerali

@Baspa Baspa linked a pull request Jan 13, 2025 that will close this issue
@Baspa Baspa closed this as completed in #15 Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants