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

Enhancement permission generator #509

Merged
merged 13 commits into from Mar 17, 2015
Merged

Enhancement permission generator #509

merged 13 commits into from Mar 17, 2015

Conversation

omero
Copy link
Contributor

@omero omero commented Mar 13, 2015

No description provided.

@omero
Copy link
Contributor Author

omero commented Mar 13, 2015

PR for #506
if i'm not wrong @esod wrote this command, do you have any suggestions, something we can do better or something isn't necessary ?

@jmolivas
Copy link
Member

Looks good to me any comments ?

@esod
Copy link
Contributor

esod commented Mar 14, 2015

Nice. The changes improve the flow of the script and it's great to see the script making additions to existing permissions.yml files.

I think restrict access should be either true, false, or none. none is something I'm working on with the install generator if I can ever get the test to pass...

On line 53 of PermissionTrait.php

  • $title = $this->getStringUtils()->camelCaseToUcFirst($title);
  • $title = $this->getStringUtils()->anyCaseToUcFirst($title);

camelCaseToUcFirst() doesn't exist unless it's in another branch. I wrote anyCaseToUcFirst() in StringUtils.php for the permission_title. I thought title: needed to be identical to the permission except for the UC first, so I used application code to generate it. But I see in book.permissions.yml that the permission and the title don't need to be identical:

I can't be sure, exactly, since I only knew to cut and paste the changes into my local console.dev, but I'd check the twig templates to make sure the indents are correct.

@omero
Copy link
Contributor Author

omero commented Mar 16, 2015

Thanks for the comments @esod, i did the changes that you suggest ... camelCaseToUcFirst() exist in the master branch https://github.com/hechoendrupal/DrupalAppConsole/blob/master/src/Utils/StringUtils.php#L86 , I not see the anyCaseToUcFirst() but i rename the function in this PR, I agree that the function can apply for any, and not exclusive for camel case

jmolivas added a commit that referenced this pull request Mar 17, 2015
@jmolivas jmolivas merged commit ffe14b2 into hechoendrupal:master Mar 17, 2015
@jmolivas
Copy link
Member

Thanks for the PR @omero and the provided help @esod

@omero omero deleted the enhancement-permission-generator branch June 29, 2015 00:55
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