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

[5.8.] When you proxy VerifyEmail verificationUrl() method is called anyway #28469

Closed
ludo237 opened this issue May 9, 2019 · 6 comments
Closed

Comments

@ludo237
Copy link

ludo237 commented May 9, 2019

  • Laravel Version: 5.8.16
  • PHP Version: PHP 7.3.5-1+ubuntu16.04.1+deb.sury.org+1 (cli) (built: May 3 2019 10:00:05) ( NTS )
  • Database Driver & Version: MariaDB 10.3

Description:

I've a Proxy for VerifyEmail that generates a new custom signed URL and then send a mail message from within the proxy itself like see code below:

    public function __invoke(User $user) : MailMessage
    {
        $signedUrl = URL::temporarySignedRoute(
            "my.custom.route.name",
            Date::now()->addMinutes(60),
            ["id" => "whatever"]
        );
        
        return (new MailMessage())
            ->subject(__("views.emails.users.verify.title"))
            ->markdown("emails.users.verify", [
                "url" => $signedUrl,
            ]);
    }

The problem is that if I don't put a dummy route named verification.verify the test will break because the native method Illuminate\Auth\Notifications::VerifyEmail@verificationUrl is called regardless of what the Proxy does which should not be the case in my opionion

Steps To Reproduce:

  • Create a Proxy
  • Use the Proxy in your AppServiceProvider.php like so
   VerifyEmail::toMailUsing(new VerifyEmailNotificationProxy());
  • Don't create the default Auth routes or don't create any route called verification.verify
  • Watch the world burn 🤣
  • Put a dummy route inside web.php with name("verification.verify") to counter-test this problem
@driesvints
Copy link
Member

Can you please fill in the exact versions in the issue template?

@ludo237
Copy link
Author

ludo237 commented May 9, 2019

@driesvints Fixed

@driesvints
Copy link
Member

Can you please share some more code? Where does this __invoke resides? How is it called?

@ludo237
Copy link
Author

ludo237 commented May 10, 2019

The __invoke method is part of a class called VerifyEmailNotificationProxy inside my App\Proxies namespace (which of course in inside app/Proxies folder)

Inside the AppServiceProvider I call it like this

public function boot()
    {
        // Override the default behavior of VerifyEmail notification
        VerifyEmail::toMailUsing(new VerifyEmailNotificationProxy());
    }

That's just it. Again if I leave a dummy route named verification.verify it works, as soon as I remove it, because I have my custom route used inside the proxy, the VerifyEmail fails

@driesvints
Copy link
Member

I see what you mean now @ludo237. I can understand that you'd want to generate a new url and not use the one generated by the VerifyEmail class. However, I think this is a feature request and not a "bug" because the VerifyEmail class works properly.

It's best to post this at the laravel/ideas repository to get support for your idea. After that you may send a PR to the framework.

Thanks!

@ludo237
Copy link
Author

ludo237 commented May 10, 2019

I think it's half a bug and half a feature request. I mean the VerifyEmail class should ignore the URL generation if I proxy it in my opinion.

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

No branches or pull requests

2 participants