Remove permissions that are now implemented in core node module

The core Node module creates the following permissions (key and description)

'create weblinks content' = Web Links: Create new content
'edit own weblinks content' = Web Links: Edit own content 
'edit any weblinks content' = Web Links: Edit any content 
'delete own weblinks content' = Web Links: Delete own content
'delete any weblinks content' = Web Links: Delete any content 

Here is a summary of all the permissions we provide in weblinks_weblinks_permissions()

'access web links'

- used in weblinks_menu() to give to access /weblinks page, all /weblinks/x pages, weblinks/goto/x and weblinks/user/x.
- used in weblinks_blocks_block_view() to return nothing if not allowed.
- Key should be renamed to 'access weblinks' so that it is consistent with all the other permissions.

'create weblinks'

- Can be removed as this is provided by 'create weblinks content' in core.
- Some usages may need to be altered to the core text value.

'edit own weblinks'

- Can be removed as this is provided by 'edit own weblinks content' in core.
- Some usages may need to be altered to the core text value.

'view my weblinks'

- used in weblinks_menu() to grant access to tab at user/x/weblinks
- used in weblinks_blocks_block_view() to return empty 'user' block if no access.

'edit group weblinks'

- in weblinks_weblinks_preprocess() sets $edit_group but this variable is not used. Can remove this variable.
- in weblinks_group_access() used to determine menu access for group adding/editing

return module_exists('taxonomy') && (user_access('administer weblinks') || user_access('edit group weblinks') || user_access('administer taxonomy'));

- This could be improved: can now check _weblinks_get_vocid() which has become more strict than just module_exists('taxonomy')
- Why does the user need two of our permissions 'administer weblinks' and'edit group weblinks'? This makes the 'edit group weblinks' redundant if you also need 'administer weblinks'. I propose we drop 'administer weblinks' from weblinks_group_access()

'administer weblinks'

- weblinks.module, in weblinks_node_access() gives update & delete authority on any weblinks node,
- in weblinks_node_view() adds edit and delete links
- Provides four admin urls in weblinks_menu(), used in weblinks_group_access() and in weblinks_get_tree()
- weblinks.user.inc, in _weblinks_user_access_check() when adding 'edit' and 'delete' operations to the table of user links
- weblinks_checker.module, in weblinks_checker_menu() to control access to Checker admin url

'view click count'

- only used in weblinks_weblinks_preprocess() when adding the click data

I will start on a patch to do the above.

Thanks, yes when I came to make the code change i realised I had mis-read that code line. It is eactly right and can stay as-is.

To avoid the automated tests crashing with exceptions when testing the permissions, we need to protect three bits of code in weblinks.test, by using if (isset($fieldset[0]))

Here's a patch which does that, which I would like to commit first, then the testing can run properly.

Here's a patch which addresses all of the above. In addition, it also has the following:

1. Made consistent use of 'Web Links' in all the permissions titles
2. Expanded and/or corrected the various help texts, to assist the admins to understand what the permission does
3. The permission key text 'view click count' is renamed to be 'view weblinks click count' so that all of our text keys have 'weblinks' in the string. This is important to ensure we do not clash with permission from other contrib modules, and it also assists when searching or filtering the node_permission db table
4. In weblinks_menu() for admin/config/content/weblinks/links change the permission from 'access weblinks' to 'administer weblinks' in as this is an admin link
5. The user table now checks both 'any' and 'own' weblinks content permissions, and 'edit' and 'delete' are separately checked
6. weblinks.install has an unpdate function grant the core node permission to any roles which had the old weblinks permissison. Also the two renamed text strings are updated

This patch will fail the automated testing because I have not changed the test files to use the new core permission names. This is on purpose, just to show that our tests do check the permissions. I have also tested each of the permissions manually as our test coverage is not 100%.

As expected we had failed tests. Here's the full patch with the test files updated too.

Thanks. I forget to run a spell checker. I have also just noticed that in my git diff I included weblinks.admin.inc when it should have been weblinks.user.inc. There are no changes in admin.inc, but the patch does need the changes to user.inc
I will do that right now - so hold off testing for two mins

Ah, yes I was a bit concerned about this. It's becuse you have already granted the core permission 'create weblinks content' to a role which I am trying to update from 'create weblinks'. I think the function is therefore going to have to do more than just simple updates, it first has to delete the weblinks versions if a core version exists. I'll work on that.

In the meantime, here is an individual patch just for the user.inc changes which got missed, if you want to test them. You can apply this patch on top of the one you've just done.

Look at the comment just above:

  // Tab for Admins to have quick access back to main weblinks page.
  $items['admin/config/content/weblinks/links'] = array(

This is not the main weblinks url, it is the extra one we provide on the 'config' page, for admins to get back to the main page. As that config page is only available to admins we should restrict this tab too.

Here's the full patch with improved update function. For each of the two old permissions we are deleting, first get the roles (if any) which already have the new core permission. Then any rows for these roles with old permission are deleted, before we update any which are left. I've explained this is the code too.

Thanks for testing - good to hear that it is basically working. I agree that it seems fine with all my testing, but I always appreciate your thoroughness in checking.

In reply to #19, you'll notice that the variable has a double $. This is a method to create a new variable name from an existing variable's value. In the code below, we loop through an array of two items, because the processing required is virtually identical, hence better for maintainability if done in a loop rather than duplicate blocks of near-identical code. The only differences required are the permission names and a variable to count the total changes.

foreach (array('create' => 'create weblinks' , 'edit_own' => 'edit own weblinks') as $key => $old_perm) {
  $new_perm = $old_perm . ' content';
  // First get all roles which already have the core permission, as the old
  // weblinks permission for any of these roles should be deleted not updated.
  $query = ...
  $roles = $query->execute()->fetchCol();
  // Delete the rows which have the old permission for these roles.
  $$key = 0;
  if (sizeof($roles)) {
    $$key = db_delete('role_permission')
      ->condition('permission', $old_perm)
      ->condition('rid', array($roles))
      ->execute();
  }
  // Then update any remaining old permissions to the new core one.
  $$key += db_update('role_permission')
  ... etc
}

The line $$key = 0; can be worked out like this: When you see a double $ resolve the second one with the variable, i.e. $key has the value 'create' so $$key is the variable $create. Hence that line is the same as $create = 0; But the benefit is that on the second loop, that line becomes $edit_own = 0; These variables hold the sum of the two db operations, and are written out in the final message

$result = t("Permissions updated: 'create weblinks' @create, 'edit own weblinks' @edit_own, 'access web links' @access, 'view click count' @view_click_count",
  array(
    '@create' => $create,
    '@edit_own' => $edit_own,
etc...

In reply to #20

The 'edit' and/or 'delete' links are not provided for other authorized users, applicable permissions assigned. The links are only provided for Web Links Admins.

Do you mean they are not provided in the current patch but should be? or they should only be provided for admins but currently are given to more users?

The existing code (pre-patch) gives both of these links if the user is looking at their own links and has 'edit own weblinks' permission or they have 'administer weblinks' regardless of who's links they are viewing. All I changed here was to separate out the 'edit' and 'delete' permission checks.

It's always nice when one learns something new :-)

the links only appear when an user has the Web Links Admin permission

This is not the behaviour which I get. I've create a role which has 'edit own weblinks content' and does not have 'administer weblinks' and I get the edit link. If I add 'delete own weblinks content' I then also get the delete link. This is working as expected for me, so somehow your site is behaving differently.

Here is some debug for you to add at the top of your _weblinks_user_access_check so we can see in more detail what is happening.

  dd('-- _weblinks_user_access_check --');
  dd('$user ' . $user->uid . ' ' . $user->name);
  dd($user->roles, '$user->roles');
  dd(user_access($op . ' own weblinks content', $user) ? 'TRUE' : 'FALSE', "user_access('$op own weblinks content')");
  dd(user_access($op . ' any weblinks content', $user) ? 'TRUE' : 'FALSE', "user_access('$op any weblinks content')");
  dd(($account->uid == $user->uid) ? 'TRUE' : 'FALSE', '$account->uid == $user->uid');
  dd(user_access('administer weblinks', $user) ? 'TRUE' : 'FALSE', "user_access('administer weblinks')");

Ah, and there I was thinking you were talking about some code I had changed in the patch ;-)

Yes, those edit and delete links are only shown when the user has 'administer weblinks' permission. If we were to do this properly, it would mean not having a static variable $admin but checking the $node being displayed each time, as a user may have 'edit own weblinks content' but not have 'edit any weblinks content'. So in this case we should only show the links on their own nodes, not for others.

That may be a useful addition, as providing these links does save one click during edit/delete which could be helpful for weblinks editors. I can add this to the next patch.

Here's the new patch, plus an interdiff showing the chnages between #18 and #27.
I realised that if we were improving the edit and delete links on the main page we should do the same on the user page. To test this fully you need to give a user the permission to view other users account pages. Then you can see whether they get the edit and/or delete links if they have 'edit any weblinks content' etc.

Thanks for testing, but your error surprises me. I've tried to replicate but cannot - are you sure you did not have the old permissions remaining from before the patch. Can you clear the cache and 'rebuild menus' from the devel menu link, then try again.

Maybe we should add calls to drupal_flush_all_caches() and menu_rebuild() at the end of update_7004 ?

Here's a patch with menu_rebuild() added, my commented lines removed and a typo fixed in blocks.test

Excellent, thank you.
I made two minor changes - (a) moved 'View click count' up in the display list of permissions to be after 'Access weblinks' as this feels a better place for it, and (b) expanded the description for 'Administer' rather than just repeat the permission title.

Thanks. I'd never commit something which you had not seen which was anything other than minor and I am pretty certain you are OK with. If I ever do, don't hesitate to call me on it ;-)