The Coder module throws all kinds of errors and warnings on this module.

Files: 
CommentFileSizeAuthor
#41 1566024_41.coding_standards_test.patch2.78 KBjonathan1055
PASSED: [[SimpleTest]]: [MySQL] 96 pass(es).
[ View ]
#39 1566024_39.coding_standards_test.patch2.81 KBjonathan1055
PASSED: [[SimpleTest]]: [MySQL] 96 pass(es).
[ View ]
#37 1566024-37-scheduler-documentation.patch21.03 KBpfrenssen
PASSED: [[SimpleTest]]: [MySQL] 85 pass(es).
[ View ]
#31 1566024_31.coding_standards_countdown.patch3.23 KBjonathan1055
PASSED: [[SimpleTest]]: [MySQL] 52 pass(es).
[ View ]
#27 1566024_27.coding_standards_module.patch15.46 KBjonathan1055
PASSED: [[SimpleTest]]: [MySQL] 52 pass(es).
[ View ]
#27 1566024.interdiff.18_to_27.module.txt7.57 KBjonathan1055
#19 1566024_19.coding_standards_install.patch2.14 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to create dependency directory.
[ View ]
#18 1566024_18.coding_standards_module.patch14.29 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_module.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 1566024_18.coding_standards_install.patch2.26 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_install.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 1566024_18.coding_standards_countdown.patch3.23 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_countdown.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#18 1566024_18.coding_standards_vertical_tabs_js.patch286 bytesjonathan1055
PASSED: [[SimpleTest]]: [MySQL] 52 pass(es).
[ View ]
#18 1566024_18.coding_standards_views_inc.patch1.14 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_views_inc.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 1566024_17.coding_standards_module.patch12.7 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/scheduler.git].
[ View ]
#17 1566024_17.coding_standards_install.patch2.12 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_17.coding_standards_install.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 1566024_17.coding_standards_countdown.patch3.01 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/scheduler.git].
[ View ]
#17 1566024_17.coding_standards_vertical_tabs_js.patch256 bytesjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_17.coding_standards_vertical_tabs_js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 1566024_17.coding_standards_views_inc.patch1.13 KBjonathan1055
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_17.coding_standards_views_inc.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 check_node code diff.jpg278.93 KBjonathan1055
#10 source tidyup single comment.jpg37.83 KBjonathan1055
#10 long lines concatenated.jpg52.9 KBjonathan1055

Comments

Yes it does, but so does it also on the weblinks module ;-)
I've got a few tidy-up patches which I worked on last year then left, which I should dig out and re-roll.
Thanks for the nudge.
Jonathan

Status:Active» Fixed

Hi @NancyDru and @jonathan1055. Let me know if you see any changes that should be reverted. I've run it through coder review after leveraging the textmage drupal php bundle. I then ran it through simpletest and everything passes. http://drupalcode.org/project/scheduler.git/commit/07792fc

Priority:Minor» Normal
Status:Fixed» Needs work

Hi Rick,
Wow, that is a useful tool. So many little things fixed, like a space either side of . , commas at the end of each array item even the last, aligning the = in consecutive code lines, corrections to indentation, spaces for tabs, the list goes on.

I have looked at the diff quickly and most appears ok, however there is one major problem in scheduler_handler_field_scheduler_countdown.inc, right at the top, the definition of all the constants in class scheduler_handler_field_scheduler_countdown extends views_handler_field seem to have been merged onto one line and the spaces between CONST and the name have been erased. That does not look at all right to me! We have

<?php
class scheduler_handler_field_scheduler_countdown extends views_handler_field {CONSTSECOND_SCALE = 1;CONSTMINUTE_SCALE = 60;CONSTHOUR_SCALE = 3600;CONSTDAY_SCALE = 86400;CONSTWEEK_SCALE = 604800;
?>

I will look in detail at the rest of the changes, but this needs to be undone or fixed for sure, because the dev version at the moment is not usable.

Jonathan

Status:Needs work» Fixed

Hi Jonathan!

Good catch :) Sometimes lint tools don't always work as expected!
http://drupalcode.org/project/scheduler.git/commit/0c5e505

-Rick

Status:Fixed» Needs work

Hi Rick,
It is still not right, you have split the single line into separate rows (which I think is easier to read but was not the actual cause of the syntax errors). The real problem is that we need a space between CONST and the name of the constant. From the last diff:

<?php
-class scheduler_handler_field_scheduler_countdown extends views_handler_field {CONSTSECOND_SCALE = 1;CONSTMINUTE_SCALE = 60;CONSTHOUR_SCALE = 3600;CONSTDAY_SCALE = 86400;CONSTWEEK_SCALE = 604800;
+class
scheduler_handler_field_scheduler_countdown extends views_handler_field {
CONSTSECOND_SCALE = 1;
CONSTMINUTE_SCALE = 60;
CONSTHOUR_SCALE = 3600;
CONSTDAY_SCALE = 86400;
CONSTWEEK_SCALE = 604800;
?>

Could you post a link to the tools that you used to make these changes - as they need to know that this changed the syntax and created runtime errors.
Jonathan

Status:Needs work» Fixed

Hi Jonathan.

This last issue was my bad. I had simply noted the single line formatting and didn't more closely look at the syntax. I was simply trying to add a fix too quickly and I'll make sure to slow down in the future!

Here is the commit
http://drupalcode.org/project/scheduler.git/commitdiff/0247236?hp=0c5e50...

We should be good to go now.

-Rick

PS. I will definitely send along your suggestions to the coder review module and to the textmate Drupal bundle because I'm kinda surprised that both missed detecting this issue.

Thanks very much. Yes this is good now.
I don't think it is a particular failing of Coder Review not to detect the problem. But the utility that merged the line and joined the text strings together is definitely very much at fault!

I think we need to review all of the changes made by the textmate tool. I'm not a fan of huge long lines all concatenated together as it makes it harder to read the file and see what the functions are doing.

Do you get options when applying that source code utility, to say what changes you want it to make and what things to leave as is?

Hi Jonathan,

Apparently the drupal textmate bundle was using an older version of the coder module's include file
https://github.com/psynaptic/php-drupal.tmbundle/blob/7.x-1.x/Support/sc...

I'm going to suggest that they use the 2.x branch of coder to see if that makes a difference (I'm assuming it will). In the meantime, I'm going to try and port this over this process to Sublime Text 2 because I've switched to that IDE and I think this would be very useful :)

-Rick

Status:Fixed» Needs work
StatusFileSize
new52.9 KB
new37.83 KB

I've looked in detail at all the automatic changes, and the vast majority are fine. In addition to the changes I listed in #3 we also get:

  • filter_xss() added to several form #description values for security.
  • Inline comments are moved to the line above which might influence the exact wording of the comments
  • Text strings which are concatenated are shifted onto one line, as in this example:

This last alteration is the one thing I am not really convinced about and I can't see the benefit of these changes. Firstly it often means that the lines extend beyond the window of text editors, making it harder to read and edit. Secondly, the lines have just been concatenated, so we end up with breaks in the strings in the middle of lines, which looks odd. But if we then fixed this, we would need to consider the translations - current translation strings would no longer be valid and the folks would have to re-enter the new strings, which is just making extra work for no benefit. What is the drupal standard about line length? I'm sure comments are meant to wrap at 80, but I'm not sure what the preferred maximum length of code lines is.

Should we back out the few specific bad cases of long lines? Most of the concatenations are ok.

Jonathan

Hi @jonathan1055,

You're right in that the filter_xss calls may be a little excessive given that $description_format and $description_blank are coming directly from t() output and therefore should already be sanitized. But for extra protection, they could be left there is case a future feature changes that and we want to be as cautious as possible.

Regarding inline comments. I do know that the changes are in alignment with the standard (http://drupal.org/node/1354#inline). I could do a sweep of the comments to see if there are ways to tighten up the language a bit.

Regarding the wrapping part. I think you're right in that it makes more sense to break it back up. Again, the original output was returned using the textmate module that used coder-7.x-1.0's coder_format_string_all() function. So perhaps that function was a bit overzealous and it wasn't designed to check the length of the resulting string before bringing it all into a single line. Perhaps this is where the human eye needs to be used.

If anyone can point to a specific place in the coding standards that spells it out more clearly, please let me know so I can reference it in the future.

-Rick

StatusFileSize
new278.93 KB

Here is an example where the = signs have all been lined up, but which later looks wrong after a code change. This example is from #2011692: Prevent cron errors when scheduled nodes no longer exist

I think that all the lined up = to make it look neat actually makes things worse later, because now we either leave it looking wrong as above, or have to change a line in the patch which does not really need changing from a code point of view. I would rather not have the automated script do this cosmetic change. I did not see anything in the Drupal coding guidelines which encourage additional spaces simply to align the = signs.

Hi jonathan1055. You're right. From now only I'll leverage the coder review script to do the first pass but then examine all the changes in detail before committing. I'll fix this on the next release.

Version:7.x-1.0» 7.x-1.1

Rick, I didn't mean to make any more work for you. I was just making an observation about the extra changes which we might not want. But if it's relatively straight-forward to use coder to make the initial pass then use some file-merge software to pick and choose which to implement, then that sounds like a good idea.

Also now that we have used coder to clean all our files, providing we also use it on each new patch then we should be able to keep the code up to standards without having to do another big one-off fix.

Jonathan

I'm also not a fan of inserting extra spaces to line up assignment operators. It is not against coding standards (ref. the example under Function Calls) but is usually discouraged since it makes maintaining the code harder. If you for example need to add a variable name which is one character longer you would feel inclined to reformat the entire block. I also think it makes the code harder to read, but I seem to be quite alone in that regard :)

The automated tools such as codesniffer are great, but they do not catch everything, and can generate false positives. For example, it often suggests to split up arrays over multiple lines, but if this array is declared inline in a function call this is often not really appropriate.

@pfrenssen: I dislike the lining up as well, and agree that it is harder to read. Not to mention that even spaces take time to load.

Title:Coding StandardsCoding Standards and Coder Review
Status:Needs work» Needs review
StatusFileSize
new1.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_17.coding_standards_views_inc.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new256 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_17.coding_standards_vertical_tabs_js.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.01 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/scheduler.git].
[ View ]
new2.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_17.coding_standards_install.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new12.7 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/scheduler.git].
[ View ]

I thought it would be worth cleaning up the remainder of the coding standard warnings which are generated by the Coder Review module (7.x-2.0), and also undo some of the automated changes from above which we agree we do not like.

Attached are five patch files, for scheduler.module, scheduler.install, scheduler_handler_field_scheduler_countdown.inc, scheduler_vertical_tabs.js and scheduler.views.inc, all made against 7.x-1.1+10 of 28th July

For the currrent full release 7.x-1.1 there are 3 critical warnings, 57 normal warnings, 49 minor warnings.
In the latest dev 7.x-1.1+10 this is reduced to 1 critical warning, 8 normal warnings, 25 minor warnings.
With these patches I have cleaned nearly everything and we have just 1 normal warning and 7 minor warnings.

The 1 normal warning is in scheduler_vertical_tabs.js where the @file block was missing. I have added it but Coder Review does not see it (because there is no php opening tag). This is already a known issue with Coder Review #1834598: Coder_Review does not recognize @file docblock in .js files

There is 1 minor warning in scheduler_handler_field_scheduler_countdown.inc - Coder says that the class name should use CamelCase not underscores. However, this class is extending a views class which has underscores, see http://api.drupal.org/api/views/views.api.php/group/views_handlers/7 So I suppose for consistency we keep the underscores if and until Views change their class naming. Any other ideas?

The remaining 6 minor warnings are in scheduler.test. I want to expand the existing tests and there is also another issue which creates more tests #1069668: Default time with user override so I think it is best to leave this file as-is for now.

In addition to the Coder Review items, I have also undone the long concatenated strings mentioned above, and undone the alignment of = signs which we did not like.

StatusFileSize
new1.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_views_inc.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new286 bytes
PASSED: [[SimpleTest]]: [MySQL] 52 pass(es).
[ View ]
new3.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_countdown.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_install.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new14.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1566024_18.coding_standards_module.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The patches for .module and .install needed a re-roll against the new 1.1+15 dev release. So I have done them all again, this time in git diff a/ b/ format.

It would be really good to get these tested and committed, so that we have a cleaner code base on which to work the remaining issues. Also with the D8 port starting, having code adhering to standards will be a better starting point.

Jonathan

StatusFileSize
new2.14 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to create dependency directory.
[ View ]

The .install patch above is incorrect. Use this one instead.

This is awesome stuff, makes the code so much more readable and pleasant to browse through!

Review of 1566024_18.coding_standards_module.patch:

+++ b/scheduler.moduleundefined
@@ -107,18 +113,33 @@ function scheduler_help($section) {
-      $output = '<p>' . t('Some scheduler actions are set for each different content type. These are accessed via the <a href="@link">admin content type</a> list.', array('@link' => url('admin/structure/types'))) . '<br/>' . t('The options and settings below are common to all content types.') . '</p>';
+      $output = '<p>'
+        . t('Some scheduler actions are set for each different content type. These are accessed via the <a href="@link">admin content type</a> list.',
+            array('@link' => url('admin/structure/types'))) . '<br/>'
+        . t('The options and settings below are common to all content types.') . '</p>';

This is more readable but it is not the way this is normally done in Drupal. Each line should look like $output .= '...'; and the options array of the t() function should be inline. Code sniffer always throws warnings for these but just ignore them.
For a good example, see taxonomy_help().

+++ b/scheduler.moduleundefined
@@ -107,18 +113,33 @@ function scheduler_help($section) {
-      $output = '<p>' . t("When you have set up Drupal's standard crontab job cron.php then Scheduler will be executed during each cron run. " . "However, if you would like finer granularity to scheduler, but don't want to run Drupal's cron more often then you can use the " . "lightweight cron handler provided by Scheduler. This is an independent cron job which only runs the scheduler process and does not " . "execute any cron tasks defined by Drupal core or any other modules."
-      ) . '</p>' . '<p>' . t("Scheduler's cron is at /scheduler/cron and a sample crontab entry to run scheduler every minute might look like:") . '</p>' . '* * * * * wget -q -O /dev/null "' . $base_url . '/scheduler/cron"' . '<p>' . t('or') . '</p>' . '* * * * * curl -s -o /dev/null "' . $base_url . '/scheduler/cron"<br/><br/>';
+      $output = '<p>'
+        . t("When you have set up Drupal's standard crontab job cron.php then Scheduler will be executed during each cron run. "
+        . "However, if you would like finer granularity to scheduler, but don't want to run Drupal's cron more often then you can use the "
+        . "lightweight cron handler provided by Scheduler. This is an independent cron job which only runs the scheduler process and does not "
+        . "execute any cron tasks defined by Drupal core or any other modules.") . '</p>'
+        . '<p>' . t("Scheduler's cron is at /scheduler/cron and a sample crontab entry to run scheduler every minute might look like:") . '</p>'
+        . '* * * * * wget -q -O /dev/null "' . $base_url . '/scheduler/cron"'
+        . '<p>' . t('or') . '</p>'
+        . '* * * * * curl -s -o /dev/null "' . $base_url . '/scheduler/cron"<br/><br/></p>';

Don't split long strings over multiple lines. Instead, split on HTML tags. I found a nice example demonstrating this in Drupal core: rdf_help().

+++ b/scheduler.moduleundefined
@@ -707,6 +734,9 @@ function scheduler_node_update($node) {
+/**
+ * Implements hook_node_delte().
+ */

Spelling mistake.

+++ b/scheduler.moduleundefined
@@ -1076,8 +1121,8 @@ function scheduler_set_target($source, $node, $target, $value) {
-    // if strtotime returned correct timestamp, we proceed with
-    // processing. Otherwise do nothing..
+    // If strtotime returned correct timestamp, we proceed with
+    // processing. Otherwise do nothing.

This can be reformatted to the 80-char limit.

+++ b/scheduler.moduleundefined
@@ -1085,7 +1130,22 @@ function scheduler_set_target($source, $node, $target, $value) {
/**
+ * Implements hook_ctools_plugin_directory().
+ *
+ * Declare a form pane (panels content type) for use in ctools and page manager.
+ * This allows the Scheduler fieldset to be placed in a panel.
+ */
+function scheduler_ctools_plugin_directory($owner, $plugin_type) {
+  if ($owner == 'ctools' && $plugin_type == 'content_types') {
+    return 'plugins/content_types';
+  }
+}

With hook implementations, traditionally the docblock only contains 'Implements hook_foo().' and the actual explanation is put inside the function:

<?php
/**
* Implements hook_foo().
*/
function scheduler_foo() {
 
// Foo the foo.
 
...
}
?>

+++ b/scheduler.moduleundefined
@@ -1085,7 +1130,22 @@ function scheduler_set_target($source, $node, $target, $value) {
+/**
  * Implements hook_i18n_sync_options().
+ *
+ * Keep the scheduler dates synchronised between separate nodes which have been
+ * defined as translations of each other.
  */
function scheduler_i18n_sync_options($entity_type, $bundle_name) {

Same as above.

+++ b/scheduler.moduleundefined
@@ -1103,7 +1163,8 @@ function scheduler_i18n_sync_options($entity_type, $bundle_name) {
/**
- * Implements drupal_alter('field_attach_prepare_translation', $entity, $context).
+ * Implements drupal_alter('field_attach_prepare_translation').
+ *
  * Prefill the node translation form with values from the translation source node.
  */

Same as above. When implementing alter hooks, the documentation is 'Implements hook_ALTER_HOOK_alter().', so in this case:

/**
* Implements hook_field_attach_prepare_translation_alter().
*/

Status:Needs review» Needs work

The last submitted patch, 1566024_19.coding_standards_install.patch, failed testing.

Status:Needs work» Needs review

Reviewed 1566024_19.coding_standards_install.patch. This is looking perfect, have split it up in two commits, one for the coding standards (2434000) and one for the removal of the old update hooks (ca6f5ef).

Committed to both the 7.x and 8.x branches.

Status:Needs review» Needs work

Review of 1566024_18.coding_standards_countdown.patch:

+++ b/scheduler_handler_field_scheduler_countdown.incundefined
@@ -2,11 +2,17 @@
- * Implements scheduler_handler_scheduler_countdown field handler
+ * Display scheduled dates in 'countdown' format for use in Views.
+ *
+ * Defines class scheduler_handler_field_scheduler_countdown.
+ * For background information on the development of this handler see
+ * http://www.ericschaefer.org/blog/2011/01/09/custom-field-handlers-for-views-2-drupal
+ *
+ * @see scheduler.views.inc

Links can also be prefixed with @see. This makes sure that they are clickable when rendered as online documentation using doxygen.

Reviewed 1566024_18.coding_standards_vertical_tabs_js.patch. Looks great, committed: e72d206.

Reviewed 1566024_18.coding_standards_views_inc.patch: excellent work, committed: 2a12b16. Realized I forgot to mention your name in the previous commit messages :-( I did not forget about the git attribution so at least you got some credit for your work. Sorry, will be more careful!

So, in summary, most of the patches were commited, but these two still need work:

  • 1566024_18.coding_standards_module.patch
  • 1566024_18.coding_standards_countdown.patch

Version:7.x-1.1» 7.x-1.x-dev
Status:Needs work» Needs review
StatusFileSize
new7.57 KB
new15.46 KB
PASSED: [[SimpleTest]]: [MySQL] 52 pass(es).
[ View ]

Thanks for reviewing, and for committing the three patches. It's very good to have the code cleaned up, as it makes checking new changes so much easier. I have re-rolled the .module patch to include everything you have mentioned in the review above. I've also made an interdiff so you can see the new changes. The patch is against the latest dev 7.x-1.1+19 of 4th Oct.

I will do the countdown changes when this one has been committed.

Status:Needs review» Needs work

Excellent work, thank you very much. It looks great now. Thanks for supplying an interdiff as well, this makes reviewing this a breeze.

Committed: fc5b785

I'm currently going over all the 'needs review' issues, from oldest to newest but will give this issue priority. Due to these coding standards fixes the other patches need to be rerolled, but it seems better to me to get this in first so we do not have to repeat this painful process in the future.

Setting back to needs work for the countdown changes.

Thanks very much. Yes the interdiff helps - I got that idea from you and Angel.

I have also been going through the 'needs review' issues, and re-rolling into a/ b/ format for automated apply patch testing. I have been doing this for the ones which do not conflict with this .module patch (which I was hoping you would commit soon, thanks), and making a note of the ones which do not conflict.

For the countdown header, you said to use @see in front of the links. Do you mean like this:

/**
* @file
* Display scheduled dates in 'countdown' format for use in Views.
*
* Defines class scheduler_handler_field_scheduler_countdown.
* For background information on the development of this handler
* @see http://www.ericschaefer.org/blog/2011/01/09/custom-field-handlers-for-views-2-drupal
*
* @see scheduler.views.inc
*/

[edit: I had to put a space at end of url, otherwise the <br> gets added to the link url, which then fails]

Yes that is exactly what I mean! These @see statements are parsed by Doxygen viewers and turn into clickable links automatically.

Here is an example: in the Migrate module you find 2 @see statements in the documentation for MigrateMailIgnore::mail(): a link and a reference to a function in Drupal core:

class MigrateMailIgnore extends DefaultMailSystem {
  /**
   * On an email request, do nothing and say we did.
   *
   * @see http://php.net/manual/en/function.mail.php
   * @see drupal_mail()
   *
   * @param $message
   *   A message array, as described in hook_mail_alter().
   * @return
   *   TRUE if the mail was successfully accepted, otherwise FALSE.
   */
  public function mail(array $message) {}

If you then look at the automatically generated Drupalcontrib API documentation for this method you can see that this has been rendered as two links underneath the section "See also". This is very helpful to help people find related information.

Status:Needs work» Needs review
StatusFileSize
new3.23 KB
PASSED: [[SimpleTest]]: [MySQL] 52 pass(es).
[ View ]

Here is the new countdown patch. Coder Review still produces one warning - that the class name should use CamelCase not underscores.

Line 16: Classes and interfaces should use UpperCamel naming. [style_class_names]
class scheduler_handler_field_scheduler_countdown extends views_handler_field {

However, this class is extending a views class which has underscores, see http://api.drupal.org/api/views/views.api.php/group/views_handlers/7 Should we change it to match standards or leave it as is to be consistent with the Views base class? Even if we fix the Scheduler class name, Coder Review will still highlight the Views class name as being against standards. That's annoying because it means we always have to ignore that message in Coder Review.

Jonathan

No that's fine, you can ignore that warning. This is something that should be fixed first in Views really, but chances are slim that that will every happen in the D7 version of Views.

Great job, thanks! I found a few nitpicks, but I'll fix these quickly before committing. No need to hold up this issue on those minor details.

  1. +++ b/scheduler_handler_field_scheduler_countdown.inc
    @@ -46,13 +51,23 @@ class scheduler_handler_field_scheduler_countdown extends views_handler_field {
    -  ); function query() {

    Wow. Inline function declaration? That's a first :)

  2. +++ b/scheduler_handler_field_scheduler_countdown.inc
    @@ -46,13 +51,23 @@ class scheduler_handler_field_scheduler_countdown extends views_handler_field {
    +  /**
    +   * Add the new timestamp_field into the SQL query. It will be calculated as
    +   * publish_on - unix_timestamp() so the result is the number of seconds from
    +   * now until the event.
    +   */
    +  function query() {

    If you follow the coding standards to the letter, a docblock should start with a summary that fits on one line, followed by a blank line, then a more detailed summary.

  3. +++ b/scheduler_handler_field_scheduler_countdown.inc
    @@ -79,14 +97,17 @@ class scheduler_handler_field_scheduler_countdown extends views_handler_field {
       function render($values) {

    This is missing @param and @return documentation.

Status:Needs review» Fixed

Committed as 8d82a73 in 7.x-1.x. Thanks!

Thank you too. Yes I'm fine with you being fussy, no problem. I did know about the one-line intro, but I think I learned that after writing that patch.

The one remaining file which still has several not-to-standard items is .test. We could fix them here, or shall we leave that to be fixed as part of #2109945: Reorganising and expanding the automated tests?

Jonathan

Hi Pieter,
I see in 1.1+26 you have made a further commit to improve the header block of _scheduler_strtotime(). http://drupalcode.org/project/scheduler.git/commit/b6c6fe0
However, the text was actually wrong before, it says:

This expects the time string to be in format 'Y-m-d H:i:s'

This is an out-of-date comment, the function now uses the variable scheduler_date_format. Given that you have fixed part of the header, I think it would be a good idea to correct this too. It is mentioned twice in the header.
Jonathan

Status:Fixed» Needs review
StatusFileSize
new21.03 KB
PASSED: [[SimpleTest]]: [MySQL] 85 pass(es).
[ View ]

Good catch. Reopening this to have a look at the other documentation to make sure it complies with coding standards.

I've opened a branch for this issue, so that we do not have to do rerolls when new patches are committed in the dev branch but can just merge in the changes. I have done two commits: one cleaning up documentation (including the fix for #36), and one cleaning up whitespace.

I attach the current diff for review.

Excellent work. The diff patch above looks good. I'm very pleased you are doing this detailed work :-)

Thanks for fixing #36. I would not have asked normally, because this issue is meant to deal with coding standards and layout. But because you improved the header partly, I thought it might as well be corrected completely.

I have spotted another wrong comment, in scheduler_node_validate()

+  // Adjust the entered times for timezone consideration. Note, we must check
+  // to see if the value is numeric. If it is, assume we have already done the
+  // strtotime conversion. This prevents us running strtotime on a value we have
+  // already converted. This is needed because Drupal 6 removed 'submit' and
+  // added 'presave' and all this happens at different times. If the value is
+  // passed as an array this means we are using the Date Popup module and a
+  // validation error has occurred. In this case we should skip validation as
+  // it is being handled by Date Popup.

Some of this no longer applies to the function now. It seems to have been added when converting from D5 to D6. I would say delete the two sentences
"adjust the entered times for timezone consideration."
and
"This is needed because DRUPAL6 removed 'submit' and added 'presave' and all this happens at different times."

If you don't want to fix this here, that's ok. but thought I would mention it now that I've seen it.

StatusFileSize
new2.81 KB
PASSED: [[SimpleTest]]: [MySQL] 96 pass(es).
[ View ]

I know we have #2109945: Reorganising and expanding the automated tests but here is a patch to fix the current .test file. Might as well get it clean and passing the Coder Review checks, so that it is easier to ensure future changes also pass. I took the opportunity to add some comments in the basic test functions as there were none.

  1. +++ b/scheduler.test
    @@ -20,6 +20,9 @@ class SchedulerTestCase extends DrupalWebTestCase {
    +  /**
    +   * Information for the test listing page.
    +   */

    This documentation standard was just changed yesterday! This should be replaced with {@inheritdoc} from now on. See Drupal SimpleTest coding standards. Here is a direct link to yesterday's update.

  2. +++ b/scheduler.test
    @@ -28,6 +31,9 @@ class SchedulerTestCase extends DrupalWebTestCase {
    +  /**
    +   * Standard setUp.
    +   */

    Also {@inheritdoc}.

  3. +++ b/scheduler.test
    @@ -72,10 +87,15 @@ class SchedulerTestCase extends DrupalWebTestCase {
    +    // Modifiy the scheduler row to a time in the past, then run cron.

    Spelling mistake "Modifiy".

Looking good overall, there are also some query optimizations which are not strictly coding standards related, but I'm fine with them going in, no need to open a separate issue for that.

StatusFileSize
new2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 96 pass(es).
[ View ]
new52.46 KB

Thanks for the link about the changed SimpleTest documentation. It slightly bothered me that I was adding those docblocs just to have them there, and not actually adding anything.

The sql optimization was suggested in Coder Review, so to keep that output as clean as possible I thought it was worth it. The link in the review is http://drupal.org/node/224333#select_count

Status:Needs review» Active

Excellent, thanks! I didn't know Coder Review even suggests query optimizations :)

Committed the above patches:

  • fd7510b: Issue #1566024 by pfrenssen: Updated documentation.
  • 11d3b19: Issue #1566024 by jonathan1055: Document the basic functionality test.
  • 1bd8b09: Issue #1566024 by jonathan1055: Optimize count queries in the basic functionality test.

Some of the existing patches will need to be rerolled due to these changes that touch much of the code base, but that can be picked up when a patch needs work.

Setting status back to "active", since there are probably some more improvements possible.

Status:Active» Fixed

Thank you Pieter, it is very good to get this entire set of changes committed.

I think we should finally allow this issue to be 'fixed' given that we have both been over the entire codebase quite thoroughly and we are not aware of anything which still needs correcting. Yes I know that coding standards improvements are never totally 'fixed' and sure there might be a few little things to improve, but nothing like the huge number we had at the start of this issue. We can correct any as and when we spot them, either in with other patches if working on that piece of code, or if we find enough of them, as a separate patch. It would be good to close this issue, see it green, as it is one that we agreed should be finished before the next release.

Thank you so much for all your work on cleaning up the code, and thanks to Nancy and Rick for their input at the start.

Good work - and Scheduler is better for it.

Jonathan

Status:Fixed» Closed (fixed)

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