Rather than using 3 aliased taxonomy pages, we could instead be using Views (pointed out by Gabor and Derek).

This would give us some sexier tabs.

Rather then rolling a patch, the new .inc file is attached. Should be added in Head and 6.x in a new directory drupalorg/views I think.

Built it here: http://d6.drupal.org/security which different tids, then exported and edited the tids

Comments

pwolanin’s picture

StatusFileSize
new11.46 KB

oops - can't attach a .inc - rename before adding to CVS

dww’s picture

Status: Needs review » Needs work

Mostly looks great, thanks! A few (minor) questions/issues:

A) Why is the path for the feed "security-contrib/rss.xml" when the page is "security/contrib"? Seems like security/contrib/rss.xml would be better, no? Same for security-psa

B) Why rss.xml instead of just /feed? E.g. security/contrib/feed ? I guess that's historical, e.g. for security/rss.xml right?

C) Minor typo: "this includes all module, themes ..." s/module/modules/ -- oh, and in the same spot: /s/install profiles/installation profiles/

D) Do we want "contrib" in the path or "contributions"? We tend to avoid abbreviations, although in this case, "contrib" is so wide-spread in Drupal, I don't think it's really a problem.

E) Not sure if it makes anything any easier in this case, but it seems like this could be a single view with multiple displays -- a lot of the view is the same for each one. All that's really different are the display-specific things like the menu path/title, page headers, etc, and the tid to use in the filter. I guess all that's left to save are a few bits of meta info about the view itself, and the sort handler. Ok, probably not worth it. ;)

F) "These posts by the Drupal security team are also sent to the security announcements e-mail list." -- shouldn't the "security announcements e-mail list" part be a link to join (or instructions on how)?

G) 'Security advisories for drupal core' -- ucfirst(drupal) ;)

H) For the filters, I'd use the "Node: Published or admin" filter, so that as the sec team is working on SAs, we can see unpublished SAs in the list already.

I) Do we want the "Security advisories for Drupal core" in the primary navigation menu like that? Perhaps we do, but it seems we should be sure we want that instead of getting it as a side effect of how the view menu items are configured.

J) Should we prefix all the view names with "drupalorg" to avoid namespace conflicts?

The vast bulk of this is trivial, and I'd be happy to just fix it while I commit. However, I wanted feedback on most of these before I go off and decide myself.

Yay for using views to solve our problems on d.o!

pwolanin’s picture

A/B: security/rss.xml is for historical reasons. Similarly, I left the contrib path as security-contrib/rss.xml and psa and security-psa/rss.xml since those are the aliases in place right now and I thought it friendlier not to break anyone who has already subscribed to either of those. I don't care much, however.

E: I'd guess it's a little lighter to load a View w/ one display then multiple?

I: I didn't intend for it to be in the Nav menu - not sure how to disable it other than manually.

I'll try to implement the other fixes.

pwolanin’s picture

re: F) the block on the side of the page has that info:

All security announcements are posted to a mailing list as well. Once logged in, go to your user profile page and subscribe to the security newsletter on the Edit » My newsletters tab.

dww’s picture

Re: A/B: I think it's better to make the feed paths sane and match the path structure of the actual pages. Things are in flux right now, and we're about to do a PSA about all the changes -- I don't mind breaking a few links for folks who happen to have subscribed already. That said, I'm in favor of keeping rss.xml instead of feed so that we're internally consistent but don't break security/rss.xml which is important.

pwolanin’s picture

Status: Needs work » Needs review
StatusFileSize
new11.35 KB

Here's an updated file.

Note that just before this is deployed someone should delete all the existing aliases (security/rss.xml, etc.)

dww’s picture

Fixed a handful of problems that were still in here:

K) You were never populating the $views array in hook_views_default_views(), just reclobbering the $view object. ;)

L) We need to define hook_views_api() so views knows where to find the default views file.

M) There were some cut and paste typos where the menu titles for all 3 views said "Security advisories for Drupal core".

I also made the feed paths match the pages, and added some redirect code to prevent link rot (which we can remove whenever we want). Finally, I ported all this to the HEAD layout of drupalorg, too.

I'm going to commit and deploy these now, I'll mark this fixed once it's working on d.o.

dww’s picture

Status: Needs review » Fixed

http://drupal.org/security
http://drupal.org/security/contrib
http://drupal.org/security/psa

I removed all the old aliases and I fixed up the blocks, too.

dww’s picture

p.s. I asked merlinofchaos about (I) (Do we want the "Security advisories for Drupal core" in the primary navigation menu like that? Perhaps we do, but it seems we should be sure we want that instead of getting it as a side effect of how the view menu items are configured.), and explained that on d.o I just disabled that menu item from the navigation menu. He said:

[09:12am] merlinofchaos: dww: Pretty much yes. I don't offer a menu callback option that has no menu item at all, since it is not too difficult to simply disable the menu item. Mildly annoying but menu is already too confusing.
[09:12am] dww: ok, fair enough.
[09:12am] merlinofchaos: I don't know how I'd describe it.

Status: Fixed » Closed (fixed)

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

  • Commit 0c588c3 on 6.x-3.x, 7.x-3.x-dev by dww:
    #398484 by pwolanin, dww: Added default views for security announcements...