Everytime the TQL finishes parsing the query, it displays (using drupal_set_message()) the query in a readable format at the top of the page (depending on your theme). This is cool but is not necessary for the module to function properly.
It should be optional.

CommentFileSizeAuthor
#6 244106.patch2.22 KBroychri
#2 244106.patch2.33 KBroychri
#1 244106-snapshot1.png28.81 KBroychri

Comments

roychri’s picture

StatusFileSize
new28.81 KB

Here is a snapshot of what I am talking about.

roychri’s picture

Status: Active » Needs review
StatusFileSize
new2.33 KB

Attached is a patch to add this functionality.

rötzi’s picture

Status: Needs review » Reviewed & tested by the community

Good addition.

Just two little comments:
1. The implementation of hook_perm is not necessary as you (correctly) use the 'administer site configuration' permission.
2. The description of the form element has a typo: ... query i nthe ...

roychri’s picture

Thanks for the review.

About the hook_perm, I would argue that most modules have their own administer permission which allow for greater granularity when assigning roles. A sub administrator role could be created and give permission to administer some module and not others. Adding this permission allow that and therefore is more flexible.

I used 'administer site configuration' permission for the "access" value in the hook_menu by mistake as I had planed to use "administer tql".

Your thoughts?

rötzi’s picture

The "permissions" page is already difficult to use as there are so many permissions where sometimes you don't know what they do exactly (This will be better in Drupal 7 when permissions get a comment). Therefore you should not use new permission unless really useful.

Additionally, the basic settings of a module are normally set once when the module is installed. This is done by someone with the 'administer site configuration' anyway, so there is no need to introduce a special permission.

There are various modules in core and in contrib which use the 'administer site configuration' for this purpose already: "blogapi", "contact", "throttle", "upload", "system", "wikitools", "talk", ...
If there were different permissions for each of these modules, setting up user roles would get increasingly difficult.

So in general I try not to introcude permissions unless there is a good use case which needs it.

roychri’s picture

StatusFileSize
new2.22 KB

Make sense, thanks for the explanation.

Here's the revised patch. I will apply and commit to HEAD and DRUPAL-5 branch.

roychri’s picture

Status: Reviewed & tested by the community » Fixed

Patch applied and commited to HEAD and DRUPAL-5 branch successfully.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.