Change usage for 'access weblinks' permission

I decided to do part 2 first. This is something that is limiting Weblinks Blocks use at the moment, and would be very useful to fix. Block content can now be displayed without the use requiring 'access weblinks' permission, as discussed. However, the 'more...' links are only displayed if the user is allowed to follow that link to the full pages. I have added an extra line to the admin config option explaining this. Also fixed one place which produced an 'undefined variable' $content if the user managed to find a url which gives them no main page content.

I am not entirely sure that item (1) is worthwile doing. It does not add any functionality, and only saves the admin user from ticking one checkbox. Anyway, we can discuss that after part (2) becuase the code changes clash.

Here's an update to the Blocks automated testing. This is just the new tests, not including the altered code in #2 above, so am expecting this run to fail one test (show block to ordinary user).

I decided to add a few more tests and hey! I discovered a fault with the number of links shown when the user does not have full weblinks access. The code change I made to avoid adding the 'more' link had the effect of leaving the additional weblink to be shown (the extra one returned because we get limit+1 links in the query). Here's a patch with the new tests and the block code changes from #2, so this will fail one test - showing the number of links as four not three.

Shows the benefit of automated testing! I probably (maybe) would have found this bug in manual testing, and I expect Gerhard would have also, but it's worthwhile getting the machine to do it too.

Here's a patch with the new tests and the corrected weblinks_blocks code. Simple to fix, in _weblinks_blocks_content() - just reorganise the logic around filling the $items array. Also it's better (and more easily understandable) to only put in the right number rather than put all in then remove the last one.

One item is added to the $items array in each of the foreach ($result as $row) loops. If the number of links defined is greater than the limit then the query will have returned one extra. So we use the condition if ($limit == 0 || count($items) < $limit) which will add all links (if there is no limit, ie limit=0) or only add if we currently have fewer than the limit count($items) < $limit). Then the elseif clause will add the 'more' link on the final foreach loop, if we had one more link returned by the query. If the number of links defined is exactly the same as the limit then the last foreach loop will add the final link and there will be no elseif clause to run.

The syntax is $options = $items = array(); I think you mis-read this and thought it had $variables. It is shorthand for

$variables = array();
$items = array();

I don't particularly like that style, but it was there and I didn't change it. The $variables array is replaced entirely in each loop.

Thanks for testing. Setting to 'needs work' while I work on part (1)

For part 1, where 'administer weblinks' should imply or include 'access weblinks' there are several options:

(a) Alter the weblinks .module and .test files to allow 'administer weblinks' as an alternative to 'access weblinks'. This is done in patch 12a attached. It was more work than expected, because it required a separate access callback function. This then required specifying the original default user_access for callback on the other items, because the value is inherited from parent menu levels.

(b) Leave all weblinks access as per now, but give a warning when saving the permissions for, to highlight any roles which have one but not the other. This is in patch 12b.

(c) As for (b) but also automatically change the submitted form, so the the additional permission is always added. I don't think this can be done in hook_form_alter and it might need a separate submit handler, hence I'd like your opinion before I start coding that.

This is the final issue which I would like to sorted before we release 7.x.

Was not easy to answer.

I agree, it is tricky to decide the best solution. Ideally, I prefer giving a message and letting the user decide, as that is cleaner code-wise, but as you point out, the UI is not very helpful for this, and you either get the message or see the permissions but not both at the same time.

I was also wondering about 'Maintain Web Links Groups' - should we do the same for this? You can't do much maintaining of groups if you cannot see the main pages.

Altering behavior of permissions 'behind the scenes' in the code is not really the best practice, as the administrator then does not see the actual effect of ticking or unticking a permisison. I started to code the 'or edit groups' additional permission and it just seems unnecessarily complicated.

... so I think I am coming to the conclusion that we either give a message(s) or leave it as-is and make no changes at all. Both of those give a clearer view of what the code is doing - and that has to be better for our users' understanding of how they configure the module.

Yes, I think the right action is to give warnings and also add to the descriptions. Here's a patch which does this for the both of the higher-level permissions.

Thank You.