Drupal coding style, warnings from Coder module

Patch to tweak code so it is more in line with Drupal coding standards. This also silences the Coder module's warnings.
This is my first patch (please advise if any errors on my part).

Perhaps I've done something wrong, but your patch fails for me due to a missing right parenthesis/unexpected ; at line 78 of recipe.module. I see in your patch:
@@ -75,7 +75,7 @@
function recipe_help($path, $arg) {
switch ($path) {
case 'node/add/recipe':
- return variable_get("recipe_help", "");
+ return variable_get('recipe_help', '';
}
}

What I did was:
1) move my versions out of the way
2) cvs update
3) patch -p1 < recipe_code_style_review-D6.patch

Should I have applied your patch against my version instead?

[edited]
Hmm. I can see the parens are being removed by the patch.

From a fresh cvs checkout (HEAD), recipe.module contains:

$ grep -A5 'function recipe_help' recipe.module
function recipe_help($path, $arg) {
switch ($path) {
case 'node/add/recipe':
return variable_get("recipe_help", "");
}
}

Next I downloaded your patch:
wget http://drupal.org/files/issues/recipe_code_style_review2-D6.patch

Here is the change which causes the PHP error:
$ grep -A5 'function recipe_help' recipe_code_style_review-D6.patch
function recipe_help($path, $arg) {
switch ($path) {
case 'node/add/recipe':
- return variable_get("recipe_help", "");
+ return variable_get('recipe_help', '';
}

NOTE: It looks like you corrected the quotes style issue, but unintentionally removed the trailing/right parenthesis.
The module will then generate a PHP error.

Can you double-check your patch by downloading it from here? Thanks.
If I edit the + line so it is the same as the - (no parenthesis change), our code style fixes patch nice and generates no errors.

[comment deleted]

Hass, your changes look good to me and I've been using the module heavily for a week. I don't have commit.. can you check this in, or does a third party need to review?

OK marble has offered me commit access to Recipe. A question before committing..

My understanding so far is that t() should be used on any string which may be displayed to any user. Is my understanding correct? If yes, what is the purpose of your removal of t() on the $schema 'description' fields?

(Is t() ineffective on schema files? Or was the motivation because it does not seem needed, since schema is not normally shown to users? It is shown to admin users, at least if using Table Wizard. My apologies if I have overlooked something fundamental in Drupal docs.. I am absorbing as fast as I can).

Thanks.

OK this is committed.

Also, hass I figured out why I couldn't apply your patch before (not the bracket typo issue, something else: I kept getting error 'Hunk #1 failed at 1.'). The problem was not in your patch of course. The cvs version of recipe.css was full of Windows style Control-M line endings, causing the fail. I'm guessing your Eclipse install handled the error automatically so you never saw it (I'm on Linux using CLI cvs). I removed those, applied your patch, and all was well. Thanks for your help.

scott@stout:~/cvs/drupal6/contributions/modules/recipe$ cvs commit -m 'bug report #668518 by hass, tzoscott: Fixed coding style issues in module, install, translations, css. Removed control-m from css.' recipe.css recipe.install recipe.module
/cvs/drupal-contrib/contributions/modules/recipe/recipe.css,v <-- recipe.css
new revision: 1.4; previous revision: 1.3
/cvs/drupal-contrib/contributions/modules/recipe/recipe.install,v <-- recipe.install
new revision: 1.11; previous revision: 1.10
/cvs/drupal-contrib/contributions/modules/recipe/recipe.module,v <-- recipe.module
new revision: 1.86; previous revision: 1.85
cvs commit: Using deprecated info format strings. Convert your scripts to use
the new argument format and remove '1's from your info file format strings.

hass, I backed out your change to recipe.module line 399, #options - it broke Recipe Admin screen with a PHP error.
I reverted it back for revision 1.87.. see diff between recipe.module 1.86 and 1.87:

scott@stout:~/cvs/drupal6/contributions/modules/recipe$ cvs diff -u -r 1.86 -r 1.87
cvs diff: Diffing .
Index: recipe.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/recipe/recipe.module,v
retrieving revision 1.86
retrieving revision 1.87
diff -u -r1.86 -r1.87
--- recipe.module 28 Jan 2010 03:19:34 -0000 1.86
+++ recipe.module 28 Jan 2010 14:09:29 -0000 1.87
@@ -1,5 +1,5 @@
<?php
-// $Id: recipe.module,v 1.86 2010/01/28 03:19:34 tzoscott Exp $
+// $Id: recipe.module,v 1.87 2010/01/28 14:09:29 tzoscott Exp $

/**
* @file
@@ -396,7 +396,7 @@
'#type' => 'select',
'#title' => t('Index depth'),
'#default_value' => variable_get('recipe_index_depth', 0),
- '#options' => array(0 => t('All Terms') + drupal_map_assoc(array(1, 2, 3, 4, 5, 6))),
+ '#options' => array(0 => t('All Terms'), 1 => "1", 2 => "2", 3 => "3", 4 => "4", 5 => "5", 6 => "6"),
'#description' => t('Defines how many levels of terms should be displayed on any given recipe index page. For example, if you select 1 then only one level of the recipe index tree will be displayed at a time.'),
);
$form['recipe_recent_box_enable'] = array(
cvs diff: Diffing po
cvs diff: Diffing translations
scott@stout:~/cvs/drupal6/contributions/modules/recipe$

hess, please leave this as fixed.

If you want to get in that one last issue you flagged for changes, the bit about (,drupal_map_assoc(array(1, 2, 3, 4, 5, 6))) you'll need to open a new issue with a patch, and have *tested* that patch (your last patch caused the Recipe Admin screen to produce PHP errors and die around line 399). I don't have the time right now to ensure it does not affect interoperability with Taxonomy (as unlikely as that would be, it needs to be tested)

When you've tested it, please open it as a new issue with patch... thanks.

Regarding this issue, the code is checked in. There is no work left. It is an ex-issue. :-)

Hi Hess,

I'm sure you tested the code at times, but you did not test your PATCH that you uploaded, before uploading it.
That fact revealed itself on 3 occasions:

1) #6 above; the module would not load at all due to missing bracket. I found PHP died. It could never have been tested. You made a new patch (#10) to fix that.
2) 15 minutes after you post #10, you post #11... you tested it and noticed that #10 was also missing a bracket. Again you rushed to upload the patch w/o testing it.
3) #20. While testing your #11 patch, the module WOULD run... but the admin/settings/recipe page would not. The PHP around line 399 was not legal, and PHP would die on the Admin page.

For point 3 above, this was the offending code change:
- '#options' => array(0 => t('All Terms'), 1 => "1", 2 => "2", 3 => "3", 4 => "4", 5 => "5", 6 => "6"),
+ '#options' => array(0 => t('All Terms') + drupal_map_assoc(array(1, 2, 3, 4, 5, 6))),
... You can not invoke drupal_map_assoc here... just try it and go to your Recipe admin screen...

That is 3 times.. It is clear that you are not testing your patches. Specifically you need to: i) rename or backup your modified code, ii) cvs update, ii) apply your patch against CVS, iv) then test everything "again" (it might seem like testing twice, but this time the test subject is the patch itself).

If the testing against the patch is OK, then upload it. I don't know if what's stated here is documented somewhere on d.o, but if it is not it has to be an informal process... I do not see any alternative really.

Thanks.

My fault too.. I see that you have made enough contributions around here that I _assumed_ your .patch was tested.
So you could take my complaint as a half-compliment. :-)
It's probably best to give me a heads up if something's not fully tested.

Anyways, no big deal.

unassigning, anyone can verify this...