add hook_project_page_link_alter(), implement to add security link to all project pages
Amazon - February 6, 2008 - 18:07
| Project: | Project |
| Version: | 5.x-1.x-dev |
| Component: | User interface |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Drupal security needs to be displayed more prominently on the Drupal site.
Let's add "Write Secure Code" to developer section of the the project interface.
Let's also add report security issue to the Support section. We will work out how to best integrate with the security site.

#1
Are you talking about making the project node itself (for example http://drupal.org/project/project) look something like this:
Development
??
#2
I'm sure I'm not the only one who would be against doing something like this in the project module, at least if it's not optional and possible for a site administrator to disable. My site that runs project* has no need for such a link, as the software available there does not run on servers and therefore security is not really a concern.
I think having a "How to write secure code" link in the development section doesn't make sense. I'm all for making it easier for developers to find that information, but I don' think this is the place to do it.
A link in the developers section to report a security issue might be a good idea, again if it's something a site admin can turn off.
#3
@aclight: yes, sorry i didn't already make it clear i was opposed to this... i was just trying to figure out what Amazon had in mind before I aimed my cannons at the proposal. ;) Certainly, this has no business being hard-coded into project.module. And, yeah, I don't think this is a good place for developers to find the info (since they rarely look at their own project pages, anyway, it seems).
A "Report a security issue" link seems better, but again, it'd have to be configurable to be useful in project.module itself. More likely, we should do it via hook_link_alter() in drupalorg.module (if possible).
#4
Ok, I'll remove the request for "Write secure code".
What about report security issue?
#5
Sure, i guess that's feasible. Someone would have to investigate if this can be done via hook_link_alter(), and if so, it can just get hard-coded into drupalorg.module. Otherwise, we'd have to make a new admin setting for this so it's customizable per-site, and do it here in project.module.
#6
can't be done via hook_link_alter the way project does things at the moment. here's an initial set of patches that would add this functionality. i've added a hook_project_page_links() to project module, then leveraged it in drupalorg module to add the security link.
as an alternative to this approach, we might want to rewrite the project page links in the same format that node links are done as, then invoke hook_link_alter() manually.
thoughts?
#7
untested, but visual inspection looks ok. The more I think about it, I suspect we'll run into trouble using hook_link_alter() since these aren't the usual node links. So, the new hook here look like a step in the right direction.
However, if we're adding a hook here, let's make it an alter hook and pass in the array of links via reference to let other modules change these links without hacking the code. For example, to remove mention of software-specific stuff on projects where that's irrelevant.
#8
changed to hook_project_page_link_alter() as requested. tested, and seems to work just fine.
#9
Patch in #8 looks good and works as expected.
However, though it's a bit of scope creep, shouldn't we first build a nested array of links for the various sections, and *then* pass that to hook_project_page_link_alter()? After calling the hook, then we output to HTML. This way modules could not only add links to the 'Resources', 'Support', and 'Development' sections we already have, but they would be able to either get rid of one or more of these sections entirely or add on additional sections.
I'll try rolling a patch that does this and submit shortly.
#10
BTW, in the patch in $8 for project_page_link_alter(), tabs are used for indentation instead of spaces.
#11
Here's a set of updated patches that implement the idea I suggested in #9. I also rewrote the theme_project_release_download_table() in project_release.module so that that function now only returns the actual download table itself, and then I wrote a new function that implements the hook_project_page_link_alter() hook. I've tested both of these patches on a local install of the drupalorg_tesing profile with the drupalorg module installed, and things seem to be working well. I've also tested on a site with actual releases and release tables, and again this works.
Note that I've changed the weights of the various link sections to spread them out a bit and to make sure that the download table comes above the 'Releases' section of links.
For sites which override theme_project_release_download_table() in their template.php, this patch might cause display of certain items on the project nodes to be out of order. I'm not sure if this is something we're concerned about or not. If so, let me know and I can write up some text in the UPGRADE.txt file (or another file, if preferred) and reroll with that text.
#12
'#value' => theme('item_list', $values['links'], t($values['name'])),<-- i'm pretty sure we don't want to t() a variable like that. shouldn't we t() it when we first declare the name?other than that, this looks good to me. much more flexible and clean than the current solution. tested, and seems to work perfectly.
#13
Ok, here's an updated patch with the changes suggested in #12.
#14
code looks good. tested, works beautifully. committed to 5.x-1.x, deployed on d.o
also committed the drupalorg.module patch, so the security links are live as well.
#15
Excellent, looks great. Thanks everyone.
#16
Automatically closed -- issue fixed for two weeks with no activity.