GHOP #136: Improve coder_format to re-format Drupal 6 core without wrong formattings

http://drupal.org/node/149826 contains the latest code of coder_format including a Windows installer now. The first two mentioned issues in #2 are fixed in that patch. Please use that version for your work on this task. I hope it will be committed ASAP.
Also, if you are working on Windows, you really should use the Windows Explorer integration to quickly and recursively clean/unclean the coding-style of Drupal core.

General:

• Please try to summarize all findings in one issue follow-up, so this issue is not so cluttered.
• When referring to wrong formattings or formattings in question, please add the filename of the patch you are referring to in your comments. (You attached two patches in #2 [which is good], but didn't include the filename in your list.)
• Please use <code> tags in your follow-ups where appropriate.
• To keep further attached patches as small as possible, you should begin with the proposed post-processor that removes white-space from all lines that only contain white-space.
• When posting patches for coder_format (itself), please create them from the base directory of the coder module. Also, please use the -p option for diff. See http://drupal.org/patch/create for further info. Diff header lines in your patches for coder_format should always look like this:
  

  diff -u -p -r1.2.4.2 coder_format.inc
  --- scripts/coder_format/coder_format.inc	9 Jan 2008 13:31:12 -0000	1.2.4.2
  +++ scripts/coder_format/coder_format.inc	16 Jan 2008 10:53:05 -0000
  @@ -829,8 +829,8 @@ function coder_preprocessor_line_breaks_
  

Questions:

#2: conf_path

Great.

#2: actions_16.patch: Extra spacing for variable assignments

First issue found is unimportant (may be dealt with later), the second is caused by a maximum character limit in coder_replace_multiple_vars().

#3: batch.inc_.patch: Assign by reference

Yes, there is no rule. But since variables assigned by reference in function signatures (i.e. mymodule_foo($op, &$node)), and functions declared by reference (i.e. &mymodule_foo();) both use this style, we should use it for variable declarations, too.

#4: bootstrap.inc_.patch: list()

No extra white-space looks good.

#4: bootstrap.inc_.patch: Capitalizing inline comments

Not part of this issue, might be added later.

#4: bootstrap.inc_.patch: Extra spacing for multi-line arrays

Of course there are things, that can't be taken into account in such a generic approach. Since we also have multi-line arrays for menu items and would not want to apply extra spacing to them, this can only be disregarded. However, if you have an idea, let me know.

#4: bootstrap.inc_.patch: (object) conversion

I don't see a problem here, coder_format properly re-formatted to (object)array()

#6: common.inc_.patch

- bug: indents after return: yep
- bug: double slashes in multi-line comments: yep
- bug: wrongly interpreted double-slashes: definitely! This bug in drupal_url_encode() is the cause for the wrong indent in all following lines.

Now, writing this answer was a bit cumbersome. How about creating a Wiki page at http://groups.drupal.org/coding-standards-and-performance-optimization containing a table (or the like) with all findings, questions and answers?

Sorry, small addition FYI: To disable a pre-/post-processor, simply comment out the key '#search', and it won't be executed. To receive debugging info on the command line, just add the item '#debug' => TRUE,.

@ezyang: http://drupal.org/node/149826 has been committed, so be sure to use a fresh checkout of coder_format (or wait approx. 12 hours until a new development snapshot has been packaged).

Reminder: Last patch was not created from coder.module's directory. If you create patches for Drupal contrib modules, everyone expects them to be based on the root of the corresponding module (except Drupal core). See http://drupal.org/patch/create Check your directory for further information.

I've committed that patch, because it included only basic changes to get this task started. Upcoming patches will be kept in this issue until the issue is set to RTBC. I don't know the GHOP rules in detail, but I think one patch including all improvements will give us a better overview about the results of this task.

Wiki page: The current "File" and "Location" won't work out, because lines in "Location" refer to a patch file, while "File" refers to another file. Proposed procedure:
- Let's always refer to actual files in Drupal core - thus, also refer to line number(s) in those files (instead of temporary patch files). Everyone is able to track these issues in their own setups then.
- Let's move "File" + "Location" to the beginning of the table, and rename "Location" to "Line(s)".
- "Resolution" is actually a "Status".
- Could you add one example (!) filename and line number for the first three rows?
- It probably makes sense to add the file path to filenames (includes/bootstrap.inc), and order all table rows alphabetically.

Posting here and there will keep this issue warm and publicly visible, which is good.

Patch files: Simply stating that a file does not include wrong formattings is enough. Please don't attach patches for those files. Instead, we might add a table row for each file indicating the status 'OK' - or what do you think? Too much clutter? Then a second table for reviewed, but 'ok' files would sound appropriate to me.

Please forget my (foolish) comment about line numbers. Let's replace them with function names instead. (Line numbers change quite often, and we still have 7 critical issues for D6)

Any updates regarding the white-space post-processor (to keep further patches smaller)?

Hyphen in -1 should not be separated

Counter example: includes/install.inc:320

You've added a comma in coder_preprocessor_inline_comment() without updating the explanation (badly needed):

    // [^;,\$] prevents matching of CVS keyword Id comment and double slashes
    //   in quotes (f.e. "W3C//DTD").

Re: cumulative patches: That's absolutely okay, just keep on posting any updates! This is why: By reviewing all Drupal core files, we are creating a definitive list of bugs or bogus results at the Wiki page. This is our test plan. It is not only checked during development, but will also completely checked again in front of committing all fixes. It is okay, if only you and me are able to track the changes between coder_format patches in follow-ups of this issue, as long as others are able to understand (through inline/PHPdoc comments) how coder_format is working.

I hope that the existing inline documentation is sufficient enough to understand how coder_format works. Since you are one of the first developers that tries to understand and enhance it, feel free to improve the inline documentation in places you think it is missing or not sufficient. That will surely help other developers to enhance the script in the future.

While randomly looking through files, I discovered another two bugs:

includes/common.inc:drupal_common_theme()

Wrong indent of closing braces (due to missing comma in surrounding multi-line array).

includes/install.inc:drupal_rewrite_settings()

Inline comment in inline comment must not be re-formatted.

However, I didn't look thoroughly through both files, so there may be more issues.

Great work so far! I really like the changes you made to coder_br()'s $result handling.

However:

• Please create a new follow-up if you want to post an updated patch.
• Try to avoid variable name abbreviations ($in_function_decl may be $in_function_declaration or at least $in_func_declaration)
• PHPdoc explanation for $in_function_declaration is missing. I don't understand why it is set to FALSE in case '(', and why exactly it has been added.
• There is no @note PHPdoc command, just remove it and the indent. Everything between the one-line function short-description and function parameters are considered "notes". :)
• I'm not sure, if the added indent handling for previous lines in coder_br() is secure. Personally, I'd feel better with a new post-processor that scans the whole file for lines containing only white-space (indent) and replaces them with a single new-line character. Additionally, the inline documentation is a bit meaningless, the order of arguments in the for statement is looking wrong, and all 'if' statements need to use curly braces even in situations where they are technically optional (see coder_postprocessor_if_curly_braces() ). Also, if I'm not totally wrong, accessing a single character in a string is done via $string{123}. Thus,
  

    // Perform backwards check.
    for ($i = strlen($result) - 1; $i--; $i >= 0) {
      if ($result[$i] == ' ') continue;
  

  should probably be

    // Check if previous character is white-space. [if you wrote it that way, you would probably understand my concerns]
    for ($i = strlen($result) - 1; $i >= 0; $i--) {
      if ($result{$i} == ' ') {
        continue;
      }
  

All other questions have been answered in the wiki page.

Wow! That's totally awesome! :)

Unfortunately, I've discovered some very odd results in common.inc:
- drupal_add_feed(): The line containing drupal_add_link(array('rel' => 'alternate', has been deleted.
- drupal_add_js(): The line containing array('basePath' => base_path()), has been deleted.
- watchdog_severity_levels(): The whole array has been deleted.

Your arguments about trimming white-space are good. I've marked the corresponding issue fixed.

I'm not sure if we really should investigate the new multiline array indent processor further. As mentioned before, multiline arrays are also used in hook_menu(), and from my knowledge, many developers would not be happy to see their menu items indented like this, f.e. in aggregator.module.

I'd say: Let's keep this in mind (and in the patch), disable the processor (for now), and focus on the other bugs. Would that be okay with you?

Re: drupal_common_theme() issues: Can't remember. If it was me, it happened not intentional during a clean-up.

Re: preprocessor_ml_array_add_comma: Performed only a quick check - unfortunately, it introduced a bug in common.inc:url().

eerm... IRC? *g*

Boy! That is a fantastic idea! It blows my head!

Since introducing unit tests for coder_format is a bit out of scope of this issue, I'll try to work with you in parallel on that. Unfortunately, I have no experience with SimpleTest yet.

• Cleaned coding style of coder_format SimpleTest files (of course, excluding .phpt files) using coder_format ;P
  • Only minor fixes were needed afterwards. A great example of a real world usage for coder_format.
  • Discovered new bug: Missing space in var $foo statements.
• Merged Text_Diff_Renderer_parallel.php into CoderTestFile.php.
• Cleaned-up README.txt, and added step-by-step instructions for unfamiliar guys like me ;)

The latest patch for coder_format is included in this patch. Since this is an AWESOME way to speed up our reviews, I would propose to include all these changes in future patches. I hope I can get into touch with Doug to coordinate this. Without coordination, we would need to place those new test files into the coder_format directory.

New patch,

- moves coder_format tests into scripts/coder_format/tests/
- creates a stub file tests/coder_format.test to let SimpleTest know of these tests.
- allows to add <?php tags in the first line of all coder_format tests for proper syntax highlighting in editors.
- includes some more code clean-up in the new SimpleTest functions (actually, I've created the previous patch in the wrong directory...).

I'll start to add the existing unit test cases from the wiki page now, so hopefully, you can focus on fixing the outstanding bugs later on.

Now we are talking! :)

SimpleTests have been committed.

Edward: This rocks!

This is the first time that Drupal 6 core does not output a fatal error after applying coder_format! Yeeeeha!

I'm already pretty sure that, once finished, all this work will gain very much attention among Drupal developers.

The new multiline array processing is quite impressive. I did not find any error.

I've tried to clean-up the inline documentation a bit. However, I'm not sure I understood the new variables correctly. If I'm not totally wrong, $in_multiline and $in_array are both controllers for inline comment placement? If they are, the PHPdoc for them could be a bit more detailed.

I agree with you regarding small unit tests. However, most of them require some context to check real world examples. Furthermore, we would need plenty of test files then. Especially regarding comments and parenthesis, there are many tests that have to be passed. After reviewing core files in includes/ once again, I've added some important tests now. Do whatever you think is best to do, but please don't delete 'em.. ;)

Regarding inline comments immediately following an function/if/elseif statement, I was not sure what the proper default should be. Sometimes a developer is commenting the statement itself, sometimes the contents of it. IMHO, moving them always above the remarked line should work out in 99% of all cases. To make this point clear, I added two functions (namely db_query_range() and _locale_import_read_po()) to comments.phpt.

I guess that --EXPECT-- is optional now?

I've replaced variables.phpt with some code that I actually wanted to use in my last patch.

IMHO it would be a good idea to stuff all possibilities into unit tests. There are only two days left until this task needs to be completed, and we might get some help from contributors to review Drupal core and find all wrong formattings. If all of them are covered by unit tests, you could concentrate on fixing coder_format instead of reviewing core files.

Finally, is it possible that the SimpleTest result table is reversed, i.e. actual results should be left? In failing tests, lines in the expected result are marked instead of lines in the actual result.

Applied coder_format on coder_format.

For reference: This is patch against complete Drupal core, using the latest version of coder_format in this thread.

Among possible wrong formattings applied by coder_format, you'll see that Drupal core indeed contains bad coding-style. :)

New patch,

- implemented logic to allow multiple tests in one test file
- splitted existing tests and added a some more
- fixed output of test results to be displayed in the page content
- added handling for class member function properties (such as abstract, private, public, protected)
- fixed class properties var
- improved some inline documentation

I've made these changes since they are not in the scope of this task.

Also, contrary to my earlier posts, I'd suggest to commit this patch now, so further patches will be smaller.

Please help ezyang to review Drupal core! Time's running out!

1) Download Drupal 6 tarball and extract it somewhere on your drive.
2) Checkout Coder from CVS (or download attached archive). If you choose to use an existing Drupal installation, make sure to create a backup first.
3) Open a command line.
4) Read the included README.txt for coder_format to find out how to apply coder_format recursively on Drupal core. Basically, it should work out like this (assuming Drupal core is in /websites/drupal):

Windows:

# cd path\to\coder_format\
# php coder_format.php --batch-replace c:\websites\drupal

Linux:

# cd /path/to/coder_format
# php coder_format.php --batch-replace /websites/drupal

Indeed, Edward, your performance has been awesome! I really enjoyed to work with you on this task.

Given the concerns that were raised during this task's proposal, I can only say: The goals have been accomplished (by far exceeded), and development for coder_format has become much easier because of the implementation of SimpleTests.

@ezyang: If you could just post a patch including your latest improvements for inline PHP tags, that would be great. I'll do a code clean-up for coder_format, and a thorough review for Drupal core then. If you want to (I hope so), we can then join forces after you've completed your other GHOP tasks.

Heh... from now on, applying coder_format on a file can't be considered as an accident anymore!

All tests pass, so I've committed all changes in #52. Attached is a new patch against Drupal core for review.

I will add (and commit) unit tests for all wrong formattings I can find throughout Drupal core. Unfortunately, I've already found these:
(i) A case statement in a case statement (filter.module) gets wrong indents.
(ii) Line breaks are added to inline PHP that includes a control structure (like an if).
(iii) Line breaks are added after curly braces in a quoted string (usually found in some JavaScript generating code). Confusingly, the new unit test passes, but locale.inc is completely broken due to _locale_rebuild_js().

Setting to RTBC so this hopefully gains some more attention, reviewers, subscribers, and follow-ups.

@whoever: Feel free to post any pointers to wrong formattings. If you do so, please provide the same information like in the wiki page.

/me keeps dreaming of a patch against Drupal 6 core, and coder_format being added to the scripts directory in core...

Gotcha! Spent some time debugging the locale mystery...

I needed to switch the order of conditions for opening curly braces:

-          if (!$after_php && (!$in_variable && !$in_quote && !$in_object && substr(rtrim($result), -1) != '$' || substr(rtrim($result), -1) == ')')) {
+          $c = substr(rtrim($result), -1);
+          if (!$after_php && !$in_quote && (!$in_variable && !$in_object && $c != '$' || $c == ')')) {

All tests still pass, but locale_rebuild_js() in locale.inc looks finally correct now. However, I still don't get why the remaining conditions need to be enclosed by extra braces.

Fixed coder_preprocessor_switch_duplicate_exit() and cleaned-up some of the regular expressions.

Also, please note that http://drupal.org/node/213072 has been committed, so usage of coder_format.php has slightly changed.

Post-processor coder_postprocessor_if_curly_braces() is finally working now, yay!

Patch in #53 has become quite outdated in the meantime. So, A + B might already be solved.

Regarding C, you'll find a in-depth discussion here: http://drupal.org/node/150626
After reviewing plenty of reformatted code by coder_format I have seen many places (in core and contrib) where these changes really improved the readability, and the sense of a comment. Of course, there might be inline comments that may loose their context. However, given your snippet, IMHO this is exactly a great example of an inline comment that rather confuses than explains something. If that DIV was opened in another function or somewhere between the lines, a helpful comment could rather look like:

-    $row .= "</div>\n"; // info div.
-
+    // Close surrounding container, opened in foo_open_container()
+    $row .= "</div>\n";

So, generally spoken, these re-formattings help developers to ensure that their comments are in fact helpful to others who try to understand the code.