for example everytime my editor goes in a node that has broken links he see in the message box the report about broken links but the role editor does not have ANY of the given permissions specific for linkchecker module

Link check of http://www. domain .com/testpage failed once (status code: 301).

perhaps a new permission should be set to allow a role to see reports on node edit

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Status: Active » Postponed (maintainer needs more info)

If someone has permission to edit a node / field, he see the message on node edit form. What's wrong with this?

GiorgosK’s picture

I would just like more granular report accessing the checker links
since in my use case the person who corrects the content
does not know enough to actually correct the links

there is the seperate site administrator that would try to resolve the broken links issues

GiorgosK’s picture

Category: bug » feature
Status: Postponed (maintainer needs more info) » Active

if that is not good enough reason please close issue

cboyden’s picture

Status: Active » Needs review
FileSize
2.94 KB

Several of my site admins have also asked for this feature. Here's a patch I'm testing on these installations now - it adds a permission to see broken link messages in context, then checks for that permission before setting messages on the node, comment, and block edit forms.

Status: Needs review » Needs work

The last submitted patch, linkchecker-hide-messages-1886890-4.patch, failed testing.

cboyden’s picture

The patch is failing because it hides the broken link messages and those tests check for that message text to appear. I will try patching the test to create and load a user with the new permission first.

cboyden’s picture

Status: Needs work » Needs review
FileSize
3.4 KB

This patch includes a change to the LinkCheckerInterfaceTest->setUp() function that adds the 'see contextual messages' permission for the test user.

Status: Needs review » Needs work

The last submitted patch, linkchecker-hide-messages-1886890-7.patch, failed testing.

cboyden’s picture

Status: Needs work » Needs review
hass’s picture

Status: Needs review » Needs work

This patch looks good to me, but can you add an additional test to verify that the permission grant/revoke are both checked, please? By adding this permission in these late state we remove the message for everyone who has the module installed. We should add an upgrade hook to add this permission to all roles. This should also happen on hook_install.

cboyden’s picture

Status: Needs work » Needs review
FileSize
11.04 KB

I added install and update hooks - they grant the permission to all roles except anonymous. Also added a test, based on the existing interface test, that revokes the permission and checks that the messages are not showing on the edit forms.

cboyden’s picture

Status: Needs review » Needs work

The new update hook throws an error if the module has been installed but is disabled when it runs.

hass’s picture

The reason should be that you run functions that are located inside user.module.

These are not available when running .install files. You need to load the user module file before using these functions.

e.g. drupal_load('module', 'user');

bwood’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.7 KB

Here's a re-roll of #11 which works with the latest -dev.

hass’s picture

Status: Needs review » Needs work

#13

kyleheney’s picture

Any chance this gets committed to an updated release version?

Thanks

cboyden’s picture

Loading the user module didn't fix the error in the update hook when the module had been installed and later disabled. I've attached an updated patch that's rerolled against the latest dev and also skips granting the permission if the module has been disabled.

joelpittet’s picture

Status: Needs review » Needs work

The permission should probably be a bit more specific to linkchecker because it could conflict with contextual, context, or other modules.

There are some nitpicky coding standards on the array ending parenthesis indent that could be cleaned up as well.

cboyden’s picture

Status: Needs work » Needs review
FileSize
9.91 KB

Thanks for the review @joelpittet, I've attached an updated patch that changes the permission name to "see linkchecker messages in context" and fixes the array closing indents.

hass’s picture

  1. +++ b/linkchecker.install
    @@ -532,3 +540,22 @@ if (!function_exists('array_replace')) {
    +  if (module_exists('linkchecker')) {
    

    I do not understand this logic... we run a linkchecker update, but linkchecker is disabled? Ok, this is possible, but why not adding the permission to every role?

  2. +++ b/linkchecker.test
    @@ -536,14 +550,12 @@ class LinkCheckerInterfaceTest extends DrupalWebTestCase {
    -    $this->assertRaw(format_plural($fail_count, 'Link check of <a href="@url">@url</a> failed once (status code: @code).', 'Link check of <a href="@url">@url</a> failed @count times (status code: @code).', array('@url' => $url1, '@code' => $status)), 'Link check failed once found.');
    ...
    -    $this->assertRaw(format_plural($fail_count, 'Link check of <a href="@url">@url</a> failed once (status code: @code).', 'Link check of <a href="@url">@url</a> failed @count times (status code: @code).', array('@url' => $url1, '@code' => $status)), 'Link check failed multiple times found.');
    

    Why has this test removed?

cboyden’s picture

If the module has been installed, but later disabled, its update hooks still run. But because the module is disabled, you get an error when running the previous version of the update hook. See https://www.drupal.org/node/737816#comment-7398132 and the rest of that issue for discussion of the problem. It was reported in 2010 and looks like it hasn't yet been fixed in D7 or D8. A possible solution, instead of skipping the permission grant, would be to bypass the problematic functions and update the permissions directly in the database. But that seems like overkill.

I will take a look at the missing test, it might have gotten messed up in one of the re-rolls.

cboyden’s picture

Updated patch fixes the missing test and also some coding standards issues in the updated code (bracket indentation, spaces, class comments).

cboyden’s picture

Patch re-rolled against latest dev.