[development] LinksDB vs. Links

[Morbus Iff]

> OI have also joined head and 4.7 branches

Noticed - good.

>>  * You've got an empty @file declaration.
> 
> Not put there by me - what is it?

I know of nothing that would put that there for you, much less commit 
the change to CVS. You can certainly remove it without worry, but you 
should consider filling out your module with documentation:

   http://drupal.org/node/1354

>>  * You're passing an unused array of string
>>    replacements to t() in linksdb_help().
> 
> Copied and butchered from someone else's module - blame them not me.

No excuse. If you don't fully understand the code
you're copying from, you shouldn't be using it.

>>  * You global $user in linksdb_menu() and you don't have to.
> 
> The module I learned from had it, and so do I.

See above.

>>  * Your use of weights in linksdb_menu(), while they certainly
>>    accomplish something, is abnormal. You never define weight 0,
>>    default local tasks are set to -10, and the tabs usually are
>>    ordered by declaration order, not manually defined weights.
> 
> I tried that and it seemed to re-order them alphabetically, which is 
> certainly not what I want when the first one begins with V...

Confirmed (by removing all weights from links/edit, links/category, 
links/moderate, and links/deadlinks, and seeing deadlinks alphabetized 
before moderate). Interesting - I'd say that's a bug in Drupal, since it 
doesn't jive with other array usages (such as form elements).

>>  * You use "links/category/edit/4" when it really should be
>>    "links/category/4/edit". Even with that said, you use arg(3)
>>    in your linksdb_categories_edit() when you should be using
>>    the naturally passed callback arguments of the menu (where
>>    you'd use linksdb_categories_edit($cid)). You use arg()
>>    throughout the module when you really shouldn't be.
> 
> That's not what I saw when learning - maybe it has changed in 4.7?

Well, arg() is certainly becoming more and more deprecated, but I 
believe the use of callback arguments from the menu path has been around 
since 4.6. In the future, we really hope to not use arg() at all. 
Regarding the use of "links/category/4/edit", that sort of system (where 
actions come after the unique entity they relate to) has been around 
forever, with node/3/edit and user/1/edit, etc.

A short demo using links/category/edit/4 as an example:

  $items[] = array(
    'path' => 'links/category/edit',
    'callback' => 'linksdb_categories_edit',
  );

  function linksdb_categories_edit($a, $b, $c, $d) { }

  url: links/category/edit (a/b/c/d empty).
  url: links/category/edit/1 (a=1/b/c/d empty).
  url: links/category/edit/1/2 (a=1/b=2/c/d empty).

And so on. To prevent warnings under PHP E_ALL, you'd really want to 
predeclare your parameters so that uninitialized values don't exist:

  function linksdb_categories_edit($a = NULL, $b = 2, ...) { }

This way, if $b isn't passed in (as per our first and second example 
URLs above), then the code would treat it as if it were set to 2.

If the arguments you need for your function aren't in the right order or 
some such, you can get more magical with "callback arguments", but it's 
proper use is too complicated to demo here. For you, you'd want to 
consider modifying your menus like what is shown in node_menu:

   http://api.drupal.org/api/HEAD/function/node_menu

>>  * You spelt "concise" wrong, and your sentence has no period.
> 
> Pedant

Quality assurance and control is not pedantry. Do I believe that things 
should be spelt and abbreviated correctly, that code should run cleanly 
under E_ALL, and have some semblance of intelligent design (heh) as 
opposed to a feeling of continual "adding on"ness? Yes! ;)

>>  * You don't know how to use drupal_get_form(). If you give it
>>    an ID of "reportform", then it'll happily use "reportform_submit".
>>    However, instead of naming it "linksdb_report", you override this
>>    automatic/free feature by forcing a callback function of "linksdb_
>>    report_support", which is unnecessary.
> 
> The documentation seems to be sadly lacking in how that side of things 
> work - I just used examples and got it working as best I could.

Are we referring to this one?

  http://api.drupal.org/api/HEAD/file/developer/topics/forms_api.html

Anyways, the short answer is: if you name your form "module_bubba", then 
you automatically get "module_bubba_validate" and "module_bubba_submit" 
for free. You also get a theme callback for free too. Thus, simply:

  function linksdb_report() {
    ... existing stuff ...
    return drupal_get_form('linksdb_report', $form);
  }

and you're already existing linksdb_report_submit() will be called.

>>  * No standardization in your code. You use $fid, $v for linksdb_
>>    report_submit, $formid, $v in _moderate_submit, and $form_id,
>>    $values in your _edit_submit. They're all the same thing.
> 
> It depends what mood I'm in and it doesn't affect how the code works in 
> any way.  Also, as I learn new tricks in drupal the variables sometimes 
> change with it.

This is quite laughable.

>>  * Your SQL isn't capitalized.
> 
> Pedant

No, it helps visually separate SQL syntax from user data.

>>  * You rarely need to use drupal_goto anymore, and
>>    almost never in the _submit hook for FormsAPI.
> 
> Except when you don't want the URL to contain the data you
 > passed as pathinfo after you have submitted.

Eh? Say again? If you want to return to "links", just:

   return 'links';

The returned value from a Forms API _submit
function is treated as the destination URL.

>>  * Your SQL queries will not work on PostgreSQL. It needs
>>    db_query('word "%s" word') not db_query("word '%s' word").
> 
> PostgreSQL is not something I have access to for
 > testing, and have no knowledge of whatsoever.

Makes no difference. It's a Drupal core standard, and if you really want 
to argue that twiddling the style of your quotes is something you just 
CAN'T do because "zomg, it's not ME, y'know?", then you have no right 
being the maintainer of your own module.

>>  * You use double spaces after sentences in _report_submit.
> 
> I am english.  I use english grammar.

I am unable to parse if this is English as in American, or English as in 
"I'm from England". Regardless, however, the double spacing has nothing 
to do with grammar, and everything to do with typesetting. Ultimately, 
it's a bad habit picked up from grade school, with teachers who don't 
actually know WHY they want their students to do that.

See:
  http://desktoppub.about.com/cs/typespacing/a/onetwospaces.htm
  http://en.wikipedia.org/wiki/Full_stop#Spacing_after_full_stop

>>  * Listen, I know I said you shouldn't use drupal_goto a few bullets
>>    ago. That doesn't mean you go and use drupal_set_header with a
>>    Location: instead. Man alive.
> 
> Does drupal_goto work with external URLs?

Yes, because it passes through to url() underneath. Either way, as per 
my followup message, you should probably be using header() instead.

>>  * You use "links/report/$link->id" and then 'links/refer/' .
>>    $link->id. Which is it? Concatenation or interpolation?
> 
> It depends on my mood - they both do the same job.

While I will agree that coding is an art form (I believe in "code 
shui"), it is how you attack it logically, via design, algorithms, and 
language that is the beauty representative of your mood, not the chaotic 
use of inconsistency within your language constructs. The argument that 
"it doesn't matter, the computer reads them as the same" is irrelevant.

>>  * Is it site staff or site administrators? You use both.
> 
> Which do you suggest?  I'm happy with either.

I believe internally we use "site administrators" more often.

>>  * theme_link() - Gah, you don't use theme('image') here.
> 
> theme whattage? Never heard of it.  What does it do?

http://api.drupal.org/api/HEAD/function/theme_image

It is the proper way to spit out an <img> via module
code. You'd use it something along the lines of:

  // just the image itself.
  theme('image', path_to_theme().'/front_project_placeholder.png')

  // an image with a link to node/1234.
  l(theme('image', path_to_theme().'/front_project_placeholder.png'),
  'node/'.$node->nid, NULL, NULL, NULL, NULL, TRUE);

>>  * # for comments? What is this, Perl?
> 
> Why not? they're perfectly valid.  And perl is the other language I use 
> day in and day out so I just stick to one comment style.

perl is the compiler. You mean "And Perl is the other ..." <g> (Sadly, 
the Perl FAQ indicates it is user choice to make this distinction).

> Why not post some issues instead of slating me in public mr 'I'm oh so 
> perfect and have never made even a slight slip in my life when learning 
> a new system'?

You'll note that a number of my comments had nothing to do with Drupal, 
including your chaotic use of concatenation vs. interpolation, "<Root>" 
vs. "&lt;Root&gt;", and so forth. These are not slight slips when 
learning a new system, these are just plain old bad habits.

Anyways, to create a patch to "fix" your module would take me roughly an 
entire day of work, as opposed to the "throwing of the bone" a few 
15-minute emails can do. Likewise, these slipups are not bugs, per se, 
but simply poor self-control, and the best way to fix those are by 
actually doing them all manually, not via someone else's patch file.

-- 
Morbus Iff ( don't heckle the super-villain )
Technical: http://www.oreillynet.com/pub/au/779
Culture: http://www.disobey.com/ and http://www.gamegrene.com/
icq: 2927491 / aim: akaMorbus / yahoo: morbus_iff / jabber.org: morbus