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

Support for Laravel 5.4.x #3

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

romanornr
Copy link

Lots of things changed from Laravel 5.0 to 5.4.

The old implementation doesn't work on Laravel 5.4.
I rewrote the controller.

Things like Session::set is now changed to session()->put()
Also the Events::fire is now events()
from Redirect:: into route()->
The instructions needed to change too.

I did't make use of the Auth:: helper. It can be done with $user = $request->user();
Since people tend to make a Models folder in Laravel.

With Request, it allows for a flexible implementation.

I also removed not required else statements.
After a return statements, the else is useless anyways.

@iangcarroll iangcarroll self-requested a review March 6, 2017 00:50
Copy link

@iangcarroll iangcarroll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just that one comment. Thanks for your help!

Session::flash('error', $e->getMessage());

return Redirect::route('u2f.auth.data');
//Session::flash('error', $e->getMessage());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you uncomment this and replace it with the session method on the request object?

$request->session()->flash('error', $e->getMessage());

@romanornr
Copy link
Author

romanornr commented Mar 8, 2017

The views didn't work out of the box in Laravel 5.4

My idea is to put the javascript into public/js/u2f folder.
In the register & auth view

    <script type="text/javascript" src="{!! asset('js/u2f/u2f.js') !!}"></script>
    <script type="text/javascript" src="{!! asset('js/u2f/app.js') !!}"></script>

This will grab the files from the /public/js/u2f folder

In this way in will work out of the box.
I didn't test yet if it really puts/create the /public/js/u2f folder by using composer

But having it like that will work.

Also please add PHP7.0 and 7.1 into Travis.
Drop PHP 5.4 and 5.5 Travis checks. Laravel doesn't support it anymore.

One more thing, The lib won't be backwards compatible with Laravel 5.0
I would recommend to release a new version like V2.0 since it isn't backwards compatible.

And store the 5.0 docs you had somewhere else if some people still use Laravel 5.0 in their projects. (doubt it)

It's a shame Laravel 5.4 doesn't follow Semantic versioning.

@@ -35,90 +29,83 @@ public function __construct(LaravelU2f $u2f, Config $config)
}

/**
* @author LAHAXE Arnaud

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😞

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add him back, and add yourself if you want. He wrote most of this repo :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to change my commit or add his author name in an extra commit. On mobile atm for a few days.

I was already wondering if you guys were planning to just merge it or I had to start my own fork & maintain it. I was working on recovery codes etc.

Anyways feel free to change stuff on my github & add his name back.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought you guys would just make the commit for the author....
Just added after 7 months seeing no extra commit figuring I would have to do it myself in this case.

@simplenotezy
Copy link

Does that mean that current build does not work on laravel 5.5?

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.

4 participants