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

[Fix] Define nullable parameter as nullable #15

Conversation

Baspa
Copy link
Member

@Baspa Baspa commented Jan 13, 2025

No description provided.

@Baspa Baspa self-assigned this Jan 13, 2025
Baspa and others added 3 commits January 13, 2025 09:50
…nt-tenancy-configurations' of github.com:vormkracht10/filament-mails into 12-bug-mail-attachment-download-route-breaks-in-different-tenancy-configurations
@Baspa Baspa merged commit 837e041 into main Jan 13, 2025
@Baspa Baspa deleted the 12-bug-mail-attachment-download-route-breaks-in-different-tenancy-configurations branch January 13, 2025 08:54
@faizananwerali
Copy link

Issue Description

There is a problem in this part.

public function __invoke(?string $tenant, string $mail, string $attachment, string $filename)

The tenant part in the route is conditionally included from a higher hierarchy route. When tenant is not present, the controller incorrectly resolves the parameters. Specifically, the $mail value is assigned to $tenant, and the rest of the parameters are shifted, causing a mismatch.

For example, the following URL:

https://admin-dev.vec.test/mails/32/attachment/30/sample%20test%208%20-%20Verified.pdf

Fails because $tenant is not included in the route, resulting in a misalignment of the parameters in the controller.


Proposed Solution

To handle this scenario, I propose the following changes to dynamically determine if tenant is present and resolve the parameters correctly:

class MailDownloadController extends Controller
{
    public function __invoke(...$arguments)
    {
        if (count($arguments) === 4) {
            [$tenant, $mail, $attachment, $filename] = $arguments;
        } else {
            [$mail, $attachment, $filename] = $arguments;
            $tenant = null;
        }

        $tenant = $tenant ? (string) $tenant : null;
        $mail = (string) $mail;
        $attachment = (string) $attachment;
        $filename = (string) $filename;

        // Handle the request...
    }
}

This solution uses the argument count to determine if tenant is included and ensures the parameters are assigned correctly. It maintains compatibility with routes both with and without tenant.

@Baspa
Copy link
Member Author

Baspa commented Jan 13, 2025

@faizananwerali got a new PR ready (#16). Feel free to submit a PR next time! :) Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Mail attachment download route breaks in different tenancy configurations
2 participants