laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.54k stars 11.02k forks source link

[5.8.15] Trait method credentials has not been applied, because there are collisions with other trait methods #28377

Closed bobbybouwmann closed 5 years ago

bobbybouwmann commented 5 years ago

Description:

The merged PR #28370 is a breaking change for Spark applications. In spark we have the following code:

class PasswordController extends Controller
{
    use SendsPasswordResetEmails, ResetsPasswords {
        SendsPasswordResetEmails::broker insteadof ResetsPasswords;
    }

    // ...
}

Because both traits now have a credentials method we get the following error

Whoops\Exception\ErrorException : Trait method credentials has not been applied, because there are collisions with other trait methods on Laravel\Spark\Http\Controllers\Auth\PasswordController

For extra context: https://laracasts.com/discuss/channels/spark/trait-method-credentials-has-not-been-applied-because-there-are-collisions-with-other-trait-methods

I think we either have to revert this PR or rename the method to something else.

Note: this is only a problem in the 5.8 branch, not in master

driesvints commented 5 years ago

I don't consider this a bug in the framework tbh but rather an update should be done to spark instead. Another override like the one above should be applied to the traits I believe.

bobbybouwmann commented 5 years ago

It's at least a bug in Spark itself, because both traits are included there and they both have the same method. But overriding it in the controller in Spark should fix this as well ;)

driesvints commented 5 years ago

Going to close this. It's best that you open up an issue on the Spark repo for this.

rickmacgillis commented 5 years ago

I can confirm this bug on my end, and have sent an email to support. I encourage everyone who has this issue to write a message to Spark's support as they don't have issues on GitHub. If you own a license for Spark Aurelius, you'll be able to see the private repo at https://github.com/laravel/spark-aurelius along with their email address.

The more people who report it, the sooner this will get resolved.

EDIT: It just dawned on me as to how I'd go about fixing this issue. Given that the traits are both being used by a Spark class, yet that class in Spark doesn't use the credentials method, but instead the traits rely on those methods individually, then I'm not sue that Spark can actually fix the situation on their end.

Both credentials methods are marked as protected methods. However, they're both traits that have the method name in question, and even private methods can get overridden in the context of a trait. So, from what I can tell, there's a bit of a trade-off we'll need to make. Here are a couple solutions to the problem.

  1. Rename the methods in question. This has the issue of no longer being backwards compatible with other people's code. However, it completely removes the issue at hand.

  2. Have Spark exhibit code duplication and try to keep the code consistent with the current Laravel Framework This is a very sloppy decision, but it keeps everyone from having to change their code.

One thing to note is that when importing traits to use their methods, you can rename their methods. However, the renamed methods only work in the class or trait that's renaming them. Therefore this functionality doesn't work very well for backwards compatibility.

Presumably if Spark cannot build something to remove the need for using that trait and without the need to duplicate code, then it might be necessary to rename the methods despite the backwards incompatibility.

Thoughts?

nbyloff commented 5 years ago

I am keeping my version locked at 5.8.14 until Spark addresses it. I imagine since this is a new addition to SendPasswordResetEmails a fix could be as simple as:

class PasswordController extends Controller
{
    use SendsPasswordResetEmails, ResetsPasswords {
        SendsPasswordResetEmails::broker insteadof ResetsPasswords;
        ResetsPasswords::credentials instead of SendsPasswordResetEmails;
    }

    // ...
}
driesvints commented 5 years ago

I've sent in a PR for this: https://github.com/laravel/spark-aurelius/pull/324

driesvints commented 5 years ago

Spark 8.1.0 is released now.

rickmacgillis commented 5 years ago

Thank you, @driesvints!

driesvints commented 5 years ago

Thanks go to @themsaid as well :)

bobbybouwmann commented 5 years ago

Thanks @driesvints @themsaid for the quick fix! You guys rock!

adam1010 commented 5 years ago

For those of us that don't have access to going Spark updates, here's the patch:

    use SendsPasswordResetEmails, ResetsPassword {
        SendsPasswordResetEmails::broker insteadof ResetsPassword;
        ResetsPassword::credentials insteadof SendsPasswordResetEmails;
    }
khanof89 commented 5 years ago

I am not sure right or wrong but simply creating a credentials function in spark/src/Http/Controllers/Auth/PasswordController.php did the trick

public function credentials(Request $request) { return $request->only('email'); }

danisjohn99 commented 5 years ago

For those of us that don't have access to going Spark updates, here's the patch:

use SendsPasswordResetEmails, ResetsPassword {
    SendsPasswordResetEmails::broker insteadof ResetsPassword;
    ResetsPassword::credentials insteadof SendsPasswordResetEmails;
}

change to "ResetsPasswords" not "ResetsPassword"

use SendsPasswordResetEmails, ResetsPasswords { SendsPasswordResetEmails::broker insteadof ResetsPasswords; ResetsPasswords::credentials insteadof SendsPasswordResetEmails; }

then it will work .otherwise exception will come