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

Will the be updated to 5.1 #14

Closed
Denoder opened this issue May 16, 2015 · 48 comments
Closed

Will the be updated to 5.1 #14

Denoder opened this issue May 16, 2015 · 48 comments

Comments

@Denoder
Copy link

Denoder commented May 16, 2015

I see a bunch of changes implemented especially dealing with user auth and something affecting the blade compiler.

@freddyheppell
Copy link

I imagine it'll be updated to every major laravel version

@rappasoft
Copy link
Owner

Yeah it will be updated providing there will be an upgrade guide which I'm sure there will. I'm going to refresh and start from scratch with every major release.

Anthony Rappa

On May 16, 2015, at 13:38, Freddy Heppell notifications@github.com wrote:

I imagine it'll be updated to every major laravel version


Reply to this email directly or view it on GitHub.

@jekinney
Copy link

With the update to middleware with injecting dynamic parameters, and a acl that is provided, it is almost a must :)

@rappasoft
Copy link
Owner

Haha yes I haven't looked into all of the new features yet. But I'm sure they will all come in handy. I want it to be a boilerplate as well as a good learning tool for all the main components.

Anthony Rappa

On May 16, 2015, at 14:34, jekinney notifications@github.com wrote:

With the update to middleware with injecting dynamic parameters, and a acl that is provided, it is almost a must :)


Reply to this email directly or view it on GitHub.

@Denoder
Copy link
Author

Denoder commented May 16, 2015

__> - I decided to upgrade it myself :P - no problems so far.

@rappasoft
Copy link
Owner

Which guide did you use?

Anthony Rappa

On May 16, 2015, at 14:52, Christian notifications@github.com wrote:

__> - I decided to upgrade it myself :P - no problems so far.


Reply to this email directly or view it on GitHub.

@Denoder
Copy link
Author

Denoder commented May 16, 2015

i just used:
http://laravel.com/docs/master/upgrade

then i found issues with the blade extension since it removed "createPlainMatcher" in the blade compilers for you know.... reasons :/ - so now the matcher is:

$matcher = '/@endpermission/';
$matcher = '/@endrole/';

@Denoder
Copy link
Author

Denoder commented May 16, 2015

Also the Middleware now uses parameters so you could do something like this:

public function handle($request, Closure $next, $permissions = null)

@rappasoft
Copy link
Owner

Oh awesome I can get rid of some classes and make it simple now.

Anthony Rappa

On May 16, 2015, at 15:25, Christian notifications@github.com wrote:

Also the Middleware now uses parameters so you could do something like this:

public function handle($request, Closure $next, $permissions = null)


Reply to this email directly or view it on GitHub.

@rappasoft
Copy link
Owner

Do you know if theres updated docs for the features yet? I don't see them, I just want to see how the route params are implemented and such. I will push a feature branch tomorrow for you to test, but it seems the only thing I had to change was bootstrap.php and create a cache directory, nothing else should be effected, I think. I'm gonna try to get rid of the Registrar file too my moving the methods somewhere else, suggestions are welcome.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

Middleware parameters:
https://laracasts.com/series/intermediate-laravel/episodes/8

not much has been re-done for laravel 5.1 but ill find some more things to check out.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

also take a look at this compiler
http://laravel.com/api/5.0/Illuminate/View/Compilers/BladeCompiler.html

vs this compiler (5.1)
http://laravel.com/api/master/Illuminate/View/Compilers/BladeCompiler.html

as you can see they removed:
createPlainMatcher <- used in your Blade Access Extender
createOpenMatcher
createMatcher


Take a look at your user roles and permissions , try adding a permission, then removing one.
they changed this lists() method into a collection so to have they as an array, you would use lists()->all()

@rappasoft
Copy link
Owner

Ok i'll take a look. The bad thing is I'm not that good at writing tests, so there are none right now, I have to attempt to check each method call to make sure they work. I think I got the blade functions working from your previous comment, I think that was all that had to be done.

@rappasoft
Copy link
Owner

I'm not sure if I like the route middleware params, it doesn't seem flexible. For example, how would I pass an array as a parameter?

Edit: They're saying you can't, I could pass a JSON string and decode it in the middleware file, but that seems more complicated than it's worth.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

i am also not good at writing tests, so what i usually do is test all functions like a normal consumer and see if something breaks, when it does, I can find where it is located... that's how it works for me.

Affected files that need to be changed to lists()->all()
App\Http\Controllers\Backend\Access\PermissionController.php
App\Http\Controllers\Backend\Access\RoleController.php
App\Http\Controllers\Backend\Access\UserController.php

so they would end with:
lists('id')->all()


yea for the middleware u can't pass it as an array, they added it but it was made into a simple function, im trying to see if they can do something about that, cause that's limiting on many factors.

I prefer to use my middleware in the controllers cause then things look a lot more clumpy in the routes.

@rappasoft
Copy link
Owner

Maybe, I prefer it in the routes so I can see it all at a glance, plus my middlewares don't work in the controllers. I don't know which way it should go, i'm not crazy about the parameters, as long as the way it works now is still going to work, why change it?

@Denoder
Copy link
Author

Denoder commented May 17, 2015

the way it works now is fine. The only reason is that it "doesnt" work in the controllers. So i fixed it up myself to work in the controllers. It works in the controllers but at the same time now it doesn't work in the routes (efficiently)

The middleware is pretty one sided, I liked the before/after filters back in 4.x >_>.

@rappasoft
Copy link
Owner

How did you implement it? Maybe I can make it work both ways.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

not really much done:

    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Closure  $next
     * @return mixed
     */
    public function handle($request, Closure $next, $permission = null)
    {
        $assets = $this->getAssets($request);

        if (! access()->canMultiple($permission, $assets['needsAll']))
        {
            return $this->getRedirectMethodAndGo($request);
        }

        return $next($request);
    }

As i said it's pretty much one sided and will basically favor controllers....

This is basically the parameter within laravel:

    /**
     * Resolve the middleware name to a class name preserving passed parameters
     *
     * @param $name
     * @return string
     */
    public function resolveMiddlewareClassName($name)
    {
        $map = $this->middleware;

        list($name, $parameters) = array_pad(explode(':', $name, 2), 2, null);

        return array_get($map, $name, $name).($parameters ? ':'.$parameters : '');
    }

so in a controller it would be like this:

$this->middleware('access.routeNeedsPermission:testing_permission');

@rappasoft
Copy link
Owner

Why not do both? You can favor either or in the code.

public function handle($request, Closure $next, $permission = null)
    {
        $assets = $this->getAssets($request);

        if (! access()->canMultiple(! is_null($permission) ? $permission : $assets['permissions'], $assets['needsAll']))
        {
            return $this->getRedirectMethodAndGo($request);
        }

        return $next($request);
    }

@rappasoft
Copy link
Owner

Plus needsAll doesn't matter because you can't pass and array.

Edit: Or you can pass that as well. Permissions could be something like permission1|permission2, then do something like explode("|", $permission) to get the array of.

@rappasoft
Copy link
Owner

Did this quick, didn't test:

public function handle($request, Closure $next, $permissions = null, $needsAll = null)
    {
        $assets = $this->getAssets($request);
        $permissions = ! is_null($permissions) ? (strpos($permissions, "|") !== false ? explode("|", $permissions) : $permissions) : $assets['permissions'];
        $needsAll = ! is_null($needsAll) ? (bool)$needsAll : $assets['needsAll'];

        if (! access()->canMultiple($permissions, $needsAll))
            return $this->getRedirectMethodAndGo($request);

        return $next($request);
    }

@Denoder
Copy link
Author

Denoder commented May 17, 2015

this works:

$this->middleware('access.routeNeedsPermission:can_manage_news|can_manage_titles');

for route parameters you would have something like this:

[middleware => 'access.routeNeedsPermission:can_manage_news|can_manage_titles']

so you would in turn remove the:

'permssions' => [],

and instead use the one above.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

the same would apply to the roles

@Denoder
Copy link
Author

Denoder commented May 17, 2015

though the NeedsRoleOrPermssion file was already confusing for me so i didnt touch it >_>


i think it would be something like this:

    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Closure  $next
     * @return mixed
     */
    public function handle($request, Closure $next, $roles = null, $permissions = null, $needsAll = null)
    {

        $assets = $this->getAssets($request);
        $roles = ! is_null($roles) ? (strpos($roles, "|") !== false ? explode("|", $roles) : $roles) : $assets['roles'];
        $permissions = ! is_null($permissions) ? (strpos($permissions, "|") !== false ? explode("|", $permissions) : $permissions) : $assets['permissions'];
        $needsAll = ! is_null($needsAll) ? (bool)$needsAll : $assets['needsAll'];

        if ($assets['needsAll']) {

            if (! access()->hasRoles($roles, true) || ! access()->canMultiple($permissions, true))
                return $this->getRedirectMethodAndGo($request);

        } else {

            if (! access()->hasRoles($roles, false) && ! access()->canMultiple($permissions, false))
                return $this->getRedirectMethodAndGo($request);

        }
        return $next($request);
    }

@rappasoft
Copy link
Owner

What confuses you about it? I can probably simplify it, but it's a combination of the other two files in one.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

yea i noticed. I like to skim files so anything that seems really long i usually just don't like to deal with unless i need to.

(Not a fan of multiple lines of code thats if and else)

@Denoder
Copy link
Author

Denoder commented May 17, 2015

Im glad L5.1 will have LTS because if i have to do another major upgrade to like L5.5 or L6 (or something of that variation) and the folder structure changes..... i will destroy all frameworks.

@rappasoft
Copy link
Owner

Did they change the folder structure this time? I didn't see it in the upgrade docs.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

na, i wa sreferring to the transition from L4 to L5.
They did change the Commands folder to Jobs so now its:

Jobs/Jobs.php

@rappasoft
Copy link
Owner

I guess that wasnt a required change to upgrade then.

I might do it anyway.

On May 17, 2015, at 16:13, Christian notifications@github.com wrote:

na, i wa sreferring to the transition from L4 to L5.
They did change the Commands folder to Jobs so now its:

Jobs/Jobs.php


Reply to this email directly or view it on GitHub.

@Denoder
Copy link
Author

Denoder commented May 17, 2015

it's not "required" they just did it to implicate that its not a command more of a job. so not changing it wont do anything.

@jekinney
Copy link

You can't pass an array per say into the middleware, but you can pass multiple parameters. Concatenate as many as you need separated by a colon (:) I believe.

@jekinney
Copy link

Taylor is also working on the docs now. Switch to master from 5.0 and that is the 5.1 docs in work.

@rappasoft
Copy link
Owner

Here's the branch: https://github.com/rappasoft/laravel-5-boilerplate/tree/feature/upgrade_to_5.1

I'm not done but you can pull it and see if you see anything wrong. Read the commit notes for what I added.

@Denoder
Copy link
Author

Denoder commented May 18, 2015

works for me:
also did throttling ever get implemented?

7b42a29

@rappasoft
Copy link
Owner

Not yet, have to research the best way to do it. I'll probably reverse engineer the zizaco/Confide version.

@rappasoft
Copy link
Owner

Also, working on way to use the access middleware fully in both routes and controllers.

@Denoder
Copy link
Author

Denoder commented May 18, 2015

what do you mean by work both in routes and controllers. Dont they already work? I mean they work for me in the controllers.

@rappasoft
Copy link
Owner

All the parameters that come with the access library, for example:

Route::group([
            'middleware' => 'access.routeNeedsRole',
            'role'       => ['Administrator'],
            'redirect'   => '/',
            'with'       => ['flash_danger', 'You do not have access to do that.']
        ], function ()
        {
            Route::get('dashboard', ['as' => 'backend.dashboard', 'uses' => 'DashboardController@index']);
            require_once(__DIR__ . "/Routes/Backend/Access.php");
        });

Will be able to be:

$this->middleware('access.routeNeedsRole:{role:Administrator,redirect:/,with:flash_danger|You do not have access to do that.}');

Instead of having the route group.

@Denoder
Copy link
Author

Denoder commented May 18, 2015

oh, the other stuff >_> - dint really like it. I was just mostly in it for the roles/permissions portion since it already redirects by itself.

@rappasoft
Copy link
Owner

It just gives more flexibility.

@rappasoft
Copy link
Owner

I have pushed all of the route/controller middleware updates: https://github.com/rappasoft/laravel-5-boilerplate/tree/feature/upgrade_to_5.1

@rappasoft
Copy link
Owner

Anything else you think I should change?

@Denoder
Copy link
Author

Denoder commented May 20, 2015

not at the moment.

@rappasoft
Copy link
Owner

Cool, when it goes stable i'll merge it and give it a 1.0 tag finally.

@hopewise
Copy link

hopewise commented Feb 1, 2016

Hello,
Is the Throttling feature ready to use in this boilerplate? I just read that such feature is built in with L5.1 https://mattstauffer.co/blog/login-throttling-in-laravel-5.1

@hopewise
Copy link

Any one?

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

No branches or pull requests

5 participants