Currently all our admin pages are just plopped right in the middle of flag.module. However, since these functions are rarely needed, it'd be much more efficient to split them out into a flag.admin.inc file and pull that file in when necessary via hook_menu().

This request is on behalf of the d.o administrators, who would like Flag to be more efficient for use on drupal.org.

CommentFileSizeAuthor
#13 flag-DRUPAL-6.cleanup.patch12.19 KBsun
#9 flag_admin_split5.patch41.37 KBquicksketch
#5 flag-DRUPAL-6--1.admin_.patch41.07 KBsun
#3 flag_admin_split6.patch37.65 KBquicksketch
#3 flag_admin_split5.patch39.48 KBquicksketch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

+1 subscribe

;) Sorry, couldn't help myself.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Note that killes also said that he'd prefer that field.actions.inc and field.tokens.inc also be included only when necessary.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
FileSize
flag_admin_split5.patch39.48 KB
flag_admin_split6.patch37.65 KB

Here are patches for splitting out the Admin pages for both Drupal 5 and Drupal 6. The D6 version was a breeze, D5 was easy too, but I'm not sure if the method I used is appropriate for including when necessary (oh... Drupal 5....). If someone could confirm this is an acceptable approach and test it out I think we're good to go on these.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Active » Needs review

flag.actions.inc currently is only 25 lines so I don't think it's doing much harm. But wtf is it just sitting in the root directory of the module? I'll make a separate issue for that. :P

#404096: Move flag.actions.inc into /includes

This one is ready for review though.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
flag-DRUPAL-6--1.admin_.patch41.07 KB

First part.

Didn't check how actions.inc and tokens.inc are included.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Heh. Oops. ;)

My patch contains some additional clean-up, not just pure moval of functions. However, I can also redo that afterwards.

Yes, that works for D5. However,

if (strpos($_GET['q'], 'admin/build/flag') === 0) {

would be faster.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented
Status: Needs review » Needs work

@sun: Thanks for helping, however, yes, PLEASE avoid the "other cleanup" urge in patches like this. ;) Also, please read the battle plans and work on patches that no one else is working on. ;)

@quicksketch: I still think using hook_init() like that in the D5 backport is a hack. I'd rather see you use a pass-through menu callback in cases like this, but do whatever you want to do. However, I think your D6 patch is wrong. In hook_menu(), I believe you want to specify:

  $path = drupal_get_path('module', 'flag') .'/includes';
  ...
    'file' => 'flag.admin.inc',
    'file path' => $path,
  ...

Specifying just "file" with a relative path seems wonky. Maybe that works, but it seems better to be explicit.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I never specify "file path", neither does core. I don't think it's necessary. http://api.drupal.org/api/function/node_menu/6

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs work » Needs review
FileSize
flag_admin_split5.patch41.37 KB

Yeah, the D5 version was indeed super wonky. Here's a reroll that uses a pass-through function and makes me feel much better about the approach.

dww’s picture

dww
we/he/they
CreditAttribution: dww commented

@quicksketch: Cool, looks good to me. I haven't tested but yeah, that looks a lot better. And cool, duly noted on 'file path' -- I just have an aversion to implicit relative paths from circa 1990. ;)

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented

the 'file path' is not required
http://drupal.org/node/146172

If no file path is specified, the path of the module defining the callback is used.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs review » Fixed

Committed #3 for D6 and #9 for D5. Thanks for the help everyone. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Fixed » Needs review
FileSize
flag-DRUPAL-6.cleanup.patch12.19 KB

If you want to have 'em, here's a patch containing the clean-ups only.

Pasqualle’s picture

Pasqualle
Location 🇭🇺 Budapest
CreditAttribution: Pasqualle commented

module_invoke is meant to be used to invoke hooks, using module_invoke for anything else is confusing..
http://lists.drupal.org/pipermail/development/2008-June/thread.html#30235

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs review » Fixed

How about start a new issue to discuss a general clean-up (the patch overall looks good) since this issue was originally well-scoped and is fixed.

Status: Fixed » Closed (fixed)

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