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

Add use LaratrustUserTrait with a command call + some fixes #4

Merged
merged 4 commits into from
Jun 15, 2016
Merged

Add use LaratrustUserTrait with a command call + some fixes #4

merged 4 commits into from
Jun 15, 2016

Conversation

KKSzymanowski
Copy link
Collaborator

Hi

Regarding issue #3:

New commands

  • laratrust:make-role - Creates a Role model based on a stub
  • laratrust:make-permission - As above, creates a Permission model
  • laratrust:add-trait - Adds a LaratrustUserTrat use statement to User model in a manner described here (class name taken from config, so not necessarily \App\User)
  • laratrust:setup - Calls(in that order) following commands, informing before each step what is going to happen now.
    • laratrust:migration
    • laratrust:make-role
    • laratrust:make-permission
    • laratrust:add-trait

Here's how it works, in HD of course:
Laratrust setup

Major fixes

  • Replace \Laratrust call from blade directive with app('laratrust').

The reason is, user isn't supposed to be obligated to add an alias so we cannot rely on it's existance. I know Zizaco implemented it with a an alias, but take a look for example at the @lang directive. It's exactly what I've done.

  • Warn user if the Laratrust migration already exists.

Do you think we should block duplication, not just warn about it?
I think it can be more reliable by checking if LaratrustSetupTables class exists, but for now I think it's fine.

Minor fixes

  • Resolve BladeCompiler out of IoC container instead of accessing it via a Facade.

I personally agree that using Facades(or whatever you might wanna call it) can implicate bad programming habbits(Taylor on Facades) and when they're not necessary, they should be replaced with Dependency Injection. That's one reason. Second reason why I did it this way is the sheer performance gain.

  • Updated DocBlocks

To be honest, Traitor is my first serious composer package and this is my first serious GitHub contribution so feel free to point out any mistakes or ways to improve this proposal.

Traitor is quite heavily tested so there souldn't be any problems on it's side. However I couldn't figure out how to test these commands. Do you have some ideas?

@santigarcor santigarcor merged commit 180af83 into santigarcor:master Jun 15, 2016
@santigarcor
Copy link
Owner

@KKSzymanowski i find this PR awesome, it will reduce the configuration complexity a lot.

To be honest i'm no quite good at testing yet, but i made some digging and i think we could make something like this

I'll be merging this PR to test it out and work on the Docs

@santigarcor
Copy link
Owner

@KKSzymanowski you should add the code you are doing in the AddLaratrustUserTraitUseCommand in the method alreadyUsesLaratrustUserTrait to your Traitor package

@KKSzymanowski
Copy link
Collaborator Author

Yep, I don't know why I didn't. The idea started with AddTraitUse being one of Laratrust simple helper classes but it turned out to be far more complicated than the first draft, so I extracted it and probably forgot about that one method.

@Mathius17
Copy link
Contributor

@KKSzymanowski amazing work you did, congrats! I was just about to tell you that we shouldn't add the LaratrustUserTrait to the User class manually, but instead use the class inside config.auth. But that is exactly what you did! That wowed me.

On another note, what terminal / command line are you using? Looks pretty neat (I'm a mac user but I'd like to know)

@KKSzymanowski
Copy link
Collaborator Author

@Mathius17 In the video? It's nothing special - it's the built in terminal in PhpStorm.

Only thing I changed with it was the background color(to match the Tibau theme) and probably font size.

Changed the default keybinging to quicky open it with F12 and it works great apart from cases when I do a lot of testing with phpunit, then scrolling up shows some bullshit(all that's needed to fix it is a terminal restart).

About configuration - one last thought. I forgot completely about this but we should create Role and Permission models honoring user configuration(model name and table name). Expect next PR soon.

@santigarcor
Copy link
Owner

@KKSzymanowski Yesterday i was reviewing the code, and i thought that, we should do it as the user trait, following the config

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.

None yet

3 participants