No functional code was changed in the creation of this patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Senpai’s picture

Assigned: Unassigned » Senpai
Status: Active » Needs review
floretan’s picture

I agree that in most cases we want to put comments on a separate line above the code in question. However, for the case of adding implementation details (which you would usually find in a footnote) it makes more sense to put them on the same line after the code.

In this specific case I think that

<?php
$timestamp = strtotime($date); // As of PHP 5.1.0, strtotime returns FALSE on failure instead of -1.

if ($timestamp <= 0) {
  $timestamp = aggregator_parse_w3cdtf($date); // Aggregator_parse_w3cdtf() returns FALSE on failure.
?>

makes more sense than

<?php
// As of PHP 5.1.0, strtotime returns FALSE on failure instead of -1.
$timestamp = strtotime($date);

if ($timestamp <= 0) {
  // Aggregator_parse_w3cdtf() returns FALSE on failure.
  $timestamp = aggregator_parse_w3cdtf($date);
?>

The attached patch simply reverts the changes affecting these two comments.

Otherwise this looks good. Great job Senpai!

catch’s picture

Status: Needs review » Reviewed & tested by the community

Good stuff.

Senpai’s picture

@Flobruit, Yeah, I agree. After reviewing the coding standards, I realize that you are correct, and the two comments in question are bylines, and not comments that explain what's going on here. They're more of a warning to be careful with this function during certain circumstances.

RTBC #2!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Anonymous’s picture

Status: Fixed » Closed (fixed)

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