ollieread / multiauth

Laravel multi auth
441 stars 109 forks source link

prevent breakage of Auth::attempt #63

Closed tbitowner closed 9 years ago

ollieread commented 9 years ago

For what reason is this needed? This breaks the core purpose and intention of this package.

tbitowner commented 9 years ago

I wanted to use your package, but following your instructions broke my original code using Laravel's core Auth component. Doing it this way allows the programmer to keep the original Auth code and also use your library.

ollieread commented 9 years ago

I think you've misunderstood this package. The idea is that it extends to original Auth functionality by putting it behind a factory class, you need to modify your original code. As it says in the readme, everything works the same, except it's pushed down a level such as Auth::user()->attempt() or Auth::admin()->attempt().

Your code actually completely undoes that and would break every installation of this.

tbitowner commented 9 years ago

Maybe, but you broke everyone elses Auth code. If you wanted to keep your original syntax you could create another facade for it.

ollieread commented 9 years ago

Okay. I didn't break anything, if you're replacing a component of a system with a different component, it stands to reason that you're going to have to change your code.

If you had read the documentation of this package, you'd see that changing your code to match this will take minutes, if that, and that your PR actually completely breaks everything.

tbitowner commented 9 years ago

Does the original Auth::user() statement still return the authenticated user? I use that statement a lot.

ollieread commented 9 years ago

No. As the documentation states, it's whatever you've configured the multiauth config to do, so it'd be Auth::user()->get() if the name was user or Auth::admin()->get() if the name was admin.

The get() method only exists so you don't get Auth::user()->user() which works still, but looks messy.

tbitowner commented 9 years ago

Right, so I would have to go and change every Auth::user() statement to Auth::user()->get() correct?

ollieread commented 9 years ago

If that's how you configured it, yes. As I've said before, all of this is covered in the documentation.

Unfortunately, your choices are either modify your code, or don't use this package. While I appreciate the time you took to post a PR, I simply won't be making a change like that, as it one, defeats the purpose of the package, two, would break all existing installations and three, would cause horrendous errors and problems if both Auth and Multiauth were running at the same time.

tbitowner commented 9 years ago

Why would it cause horrendous errors? I am using both of them currently without any issues. I'll see if I can figure something else out to give the option to stick with the original syntax of your library.

ollieread commented 9 years ago

There's no change that I will merge in to allow that, the reason I do the things the way I do, is because it's the only option.

tbitowner commented 9 years ago

Why is it the only option?

ollieread commented 9 years ago

Because I spent a long time working out the best way to do this, look through the code of the package and you'll see the conflicts that will come up from using both.

tbitowner commented 9 years ago

Fair enough. Good library btw.