Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

Status: Active » Needs review
FileSize
6.62 KB

This replaces all uses of "URI" in comments with the uppercase spelling, except where the comment is specifically discussing a variable name.

DamienMcKenna’s picture

Version: 8.x-dev » 7.x-dev
FileSize
6.72 KB

This patch makes the same fixes for D7.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

Wait, has the 8.x patch been committed? If not, let's start there, and please don't change to 7.x until it's committed.

And if we're fixing URI, how about also URL? A quick grep found these:

core/includes/form.inc: * Returns HTML for a url form element.
core/includes/theme.inc: *   - url: The url for the link.
core/includes/theme.inc: *   - url: The url of the main page.

among others...

DamienMcKenna’s picture

@jhodgdon: good call, I'll work on these.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
47.07 KB

This patch resolves spelling of URI, URL, HTTP and HTTPs.

jhodgdon’s picture

Isn't ArchiverTar one of the files we don't touch, since it comes from a different project? I also don't think we should change test .txt files, even if they are lame, and we definitely should not change anything in Symfony.

Other than that, the patch looks good!

DamienMcKenna’s picture

FileSize
46.05 KB

Thanks jhodgdon.

This patch has been updated to remove the modification to the Symfony component, sorry for overlooking that one (I'll work on upstream fixes for that).

I think modifications to ArchiveTar.php is fair game as its license allows it.

As for the text files, why not? Shouldn't *all* text follow coding standards? :)

jhodgdon’s picture

Regarding ArchiveTar, it's not the license, it's that the whole file doesn't follow our coding/docs standards and I think we try to not modify it as much as possible.

Regarding the text files, they are used in tests and I would just as soon not modify them. And no they are not subject to our coding standards.

DamienMcKenna’s picture

FileSize
35.6 KB

This patch removes the changes to ArchiveTar.php and the txt files.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! The patch looks good now.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, wanted to quickly get the major queue cleaned up before looking at this. Miraculously, this still seems to apply with patch -p1, so w00t!

Committed and pushed to 8.x. Thanks!

DamienMcKenna’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Active
Issue tags: +Needs backport to D7

Thanks @webchick.

I'll work on a backport on Monday.

DamienMcKenna’s picture

Status: Active » Needs review
FileSize
47.8 KB

I did a fresh review of the D7 codebase for occurrences of 'url', 'uri', 'http' and 'https' and made them all uppercase when appropriate.

xjm’s picture

Status: Needs review » Needs work
+++ b/modules/dblog/dblog.testundefined
--- a/modules/field/field.module
+++ b/modules/field/field.moduleundefined

+++ b/modules/field/field.moduleundefined
@@ -1167,9 +1167,21 @@ function theme_field($variables) {
   // Render the items.
   $output .= '<div class="field-items"' . $variables['content_attributes'] . '>';
...
   foreach ($variables['items'] as $delta => $item) {
-    $classes = 'field-item ' . ($delta % 2 ? 'odd' : 'even');
-    $output .= '<div class="' . $classes . '"' . $variables['item_attributes'][$delta] . '>' . drupal_render($item) . '</div>';
+    // Set the default css classes (e.g. zebra, delta, first/last). This
+    // isn't 0 indexed because users expect the first item to be 1 and odd.
+    $classes = array();
+    $classes[] = 'field-item';
+    $classes[] = (($delta + 1) % 2 ? 'odd' : 'even');
+    $classes[] = 'field-item-' . ($delta + 1);
+    if ($delta == 0) {
+      $classes[] = 'field-item-first';
+    }
+    elseif ($delta == $item_count - 1) {
+      $classes[] = 'field-item-last';
+    }
+    $output .= '<div class="' . implode(' ', $classes) . '"' . $variables['item_attributes'][$delta] . '>' . drupal_render($item) . '</div>';

+++ b/modules/field/theme/field.tpl.phpundefined
@@ -56,7 +56,7 @@ HTML comment.
-      <div class="field-item <?php print $delta % 2 ? 'odd' : 'even'; ?>"<?php print $item_attributes[$delta]; ?>><?php print render($item); ?></div>
+      <div class="field-item <?php print $delta % 2 ? 'odd' : 'even'; ?> field-item-<?php print $delta; ?><?php if ($delta == 0): print ' field-item-first'; endif ?><?php if ($delta == count($items) - 1): print ' field-item-last'; endif ?>"<?php print $item_attributes[$delta]; ?>><?php print render($item); ?></div>

Er. This change seems unrelated?

DamienMcKenna’s picture

Oops, that snook in from #964288: Field output lacks 'first' and 'last' css classes for field-items X-) I'll re-roll it, without the powerups :)

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
45.71 KB

This should be better. *cough* I reviewed the patch line by line to ensure it *only* contains the intended typo fixes this time.

jhodgdon’s picture

Status: Needs review » Needs work

As in the 8.x patch, I don't think we should change any of the test .txt data files. See #6/#8 above.

For Drupal 7.x, we probably also don't want to change UI text that will need to have translations updated:

-      $this->assertTrue($url == $correct_link, t('The url() function returns the right url (@url) in accordance with the chosen language', array('@url' => $url . " == " . $correct_link)));
+      $this->assertTrue($url == $correct_link, t('The url() function returns the right URL (@url) in accordance with the chosen language', array('@url' => $url . " == " . $correct_link)));

(etc.)

Other that that, the patch looks good!

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
32.71 KB

I've updated the D7 patch per jhodgdon's comments.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I'll get this one committed.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Committed to 7.x.

DamienMcKenna’s picture

Any interest in my backporting this to D6?

jhodgdon’s picture

If someone wants to backport it, I will happily review the patch, and probably Gabor will commit it. Just set the version to 6.x and the status to "to be ported" and port away, if you would like to do it. Thanks! (Same constraints as d7: no text changes and no test file changes please.)

DamienMcKenna’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

I'll put it together this evening :)

DamienMcKenna’s picture

FileSize
164.35 KB

D6 version, with a few extra changes:

  • There were several variations of "an urlencoded" vs "a urlencoded" vs "urlencoded()" vs "URL-encoded", etc; I've standardized these on "urlencoded" or "a urlencoded" as appropriate.
  • Several comment lines were missing periods at the end of their sentences. This is not an exhaustive list, but it resolves several lines.
  • Several comment lines did not wrap at 80 characters. This is not an exhaustive list.

Let me know if you'd prefer I provide a patch that *only* handles the typos.

DamienMcKenna’s picture

Status: Patch (to be ported) » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-n1742958-24-d6.patch, failed testing.

jhodgdon’s picture

The actual error from that test run was:

Output: [error: patch failed: includes/common.inc:531
error: includes/common.inc: patch does not apply].

So it looks like there's a problem in the patch.

As far as the questions in #24 are concerned... yes we normally prefer patches to stick to the reported issue -- there were spots in the D7/8 patches that I noticed that could have benefited from some cleanup, but even though they were in the same lines as the capitalizations, we didn't clean them up.

Anyway... we're not really going back and cleaning up D6 docs at this point. There are a lot of places in the D6 docs that are not up to standards; less so in D7/8. If you'd like to help clean up d7/8 and bring it up to standards, there is a meta-issue:
#1310084: [meta] API documentation cleanup sprint
and here is a search for the issues -- quite a lot of them have patches in need of review, so that would be a VERY WONDERFUL way to help out!
http://drupal.org/project/issues/search/drupal?status[]=Open&issue_tags=...

DamienMcKenna’s picture

Version: 6.x-dev » 7.x-dev
Status: Needs work » Fixed

Fair enough, I'll focus effort on the docs cleanup instead.

jhodgdon’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Needs work

Sorry for the confusion -- it is fine to make a patch that fixes up the capitalization of acronyms (this specific issue) for D6. What I don't want to see is a patch that makes other clean-up fixes for D6. Thanks!

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
11.98 KB

A patch for D6 that just fixes spellings of url, ftp, http and https.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This patch looks good to commit - thanks!

DamienMcKenna’s picture

Thank you Jennifer.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again! This has been (finally) committed to 6.x.

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