Closed (fixed)
Project:
Project
Version:
5.x-1.x-dev
Component:
User interface
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
6 Feb 2008 at 18:07 UTC
Updated:
23 Feb 2008 at 02:31 UTC
Jump to comment: Most recent file
Comments
Comment #1
dwwAre you talking about making the project node itself (for example http://drupal.org/project/project) look something like this:
Development
??
Comment #2
aclight commentedI'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.
Comment #3
dww@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).
Comment #4
Amazon commentedOk, I'll remove the request for "Write secure code".
What about report security issue?
Comment #5
dwwSure, 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.
Comment #6
hunmonk commentedcan'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?
Comment #7
dwwuntested, 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.
Comment #8
hunmonk commentedchanged to hook_project_page_link_alter() as requested. tested, and seems to work just fine.
Comment #9
aclight commentedPatch 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.
Comment #10
aclight commentedBTW, in the patch in $8 for project_page_link_alter(), tabs are used for indentation instead of spaces.
Comment #11
aclight commentedHere'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.
Comment #12
hunmonk commented'#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.
Comment #13
aclight commentedOk, here's an updated patch with the changes suggested in #12.
Comment #14
hunmonk commentedcode 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.
Comment #15
dwwExcellent, looks great. Thanks everyone.
Comment #16
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.