[development] LinksDB vs. Links

[Matthew Jenkins]

On 2 Aug 2006, at 15:46, Morbus Iff wrote:

>> 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.

I agree, but where else am I going to learn from?  It's changed now 
anyway.

>
>>>  * 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.

It's removed.

>
>>>  * 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

I was hoping there'd be a nice way like that - arg() always seemed like 
a but of a kludgy hack.  I'll see about updating it to use that system 
for the future.

>
>>>  * 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

Ok, not lacking - but bl**dy confusing nontheless.
>
> 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.

Cool - makes it easier to read.  The way I understood it was that the 
first element was passed to the function defined by the third element 
as the ID of the form.

>
>>>  * 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.

I've changed it anyway

>
>>>  * 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.

I never knew that.  I'll use that from now on then as it's nicer.

>
>>>  * 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.

I have reversed all my quotes.  It's force of habbit from using MySQL 
all the time and using "..." to embed variables into the string.

>
>>>  * 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

both american sites and meaningless to UK english grammar, which states 
that after a full stop you have 2 spaces.

>
>>>  * 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.

Then I shall too.

>
>>>  * 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);
>

does path_to_theme() return the path to the module? or the path to the 
theme in use?  If the latter, then it's useless to me as the image is 
in the module not the theme.

>>>  * # 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).

Perl: Practical Extraction and Reporting Language

Note the last word that begins with L...

Regardless of the capitalisation, perl still means that.

And anyway, perl isn't the compiler...

perl5.8.8 is the bytecode compiler.  perl is merely a symbolic link to 
it (or it is on sensible systems)

>
>> 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.

I wouldn't want a patch file.

But a more private form of communication would be appreciated.

>
> -- 
> 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
>
> Scanned by MailDefender - managed email security from intY - 
> www.maildefender.net
>


Information in this electronic mail is confidential and may be legally privileged. It is intended solely for the addressee. Access to this mail by anyone else is unauthorised. If you are not the intended recipient any use, disclosure, copying or distribution of this message is prohibited and may be unlawful. When addressed to our customers, any information contained in this message is subject to intY's Terms & Conditions. Please rely on your own virus scanning and procedures with regard to any attachments to this message.

Scanned by MailDefender - managed email security from intY - www.maildefender.net