Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Shouldn't the spelling of "URI", excluding variable names, be in uppercase? There are lots of cases where it is spelled in lowercase in comments.
Comment | File | Size | Author |
---|---|---|---|
#30 | drupal-n1742958-30-d6.patch | 11.98 KB | DamienMcKenna |
#24 | drupal-n1742958-24-d6.patch | 164.35 KB | DamienMcKenna |
#18 | drupal-n1742958-18-d7.patch | 32.71 KB | DamienMcKenna |
#16 | drupal-n1742958-16-d7.patch | 45.71 KB | DamienMcKenna |
#13 | drupal-n1742958-13-d7.patch | 47.8 KB | DamienMcKenna |
Comments
Comment #1
DamienMcKennaThis replaces all uses of "URI" in comments with the uppercase spelling, except where the comment is specifically discussing a variable name.
Comment #2
DamienMcKennaThis patch makes the same fixes for D7.
Comment #3
jhodgdonWait, 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:
among others...
Comment #4
DamienMcKenna@jhodgdon: good call, I'll work on these.
Comment #5
DamienMcKennaThis patch resolves spelling of URI, URL, HTTP and HTTPs.
Comment #6
jhodgdonIsn'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!
Comment #7
DamienMcKennaThanks 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? :)
Comment #8
jhodgdonRegarding 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.
Comment #9
DamienMcKennaThis patch removes the changes to ArchiveTar.php and the txt files.
Comment #10
jhodgdonThanks! The patch looks good now.
Comment #11
webchickOk, 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!
Comment #12
DamienMcKennaThanks @webchick.
I'll work on a backport on Monday.
Comment #13
DamienMcKennaI did a fresh review of the D7 codebase for occurrences of 'url', 'uri', 'http' and 'https' and made them all uppercase when appropriate.
Comment #14
xjmEr. This change seems unrelated?
Comment #15
DamienMcKennaOops, 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 :)
Comment #16
DamienMcKennaThis should be better. *cough* I reviewed the patch line by line to ensure it *only* contains the intended typo fixes this time.
Comment #17
jhodgdonAs 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:
(etc.)
Other that that, the patch looks good!
Comment #18
DamienMcKennaI've updated the D7 patch per jhodgdon's comments.
Comment #19
jhodgdonThanks! I'll get this one committed.
Comment #20
jhodgdonThanks! Committed to 7.x.
Comment #21
DamienMcKennaAny interest in my backporting this to D6?
Comment #22
jhodgdonIf 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.)
Comment #23
DamienMcKennaI'll put it together this evening :)
Comment #24
DamienMcKennaD6 version, with a few extra changes:
Let me know if you'd prefer I provide a patch that *only* handles the typos.
Comment #25
DamienMcKennaComment #27
jhodgdonThe actual error from that test run was:
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=...
Comment #28
DamienMcKennaFair enough, I'll focus effort on the docs cleanup instead.
Comment #29
jhodgdonSorry 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!
Comment #30
DamienMcKennaA patch for D6 that just fixes spellings of url, ftp, http and https.
Comment #31
jhodgdonThis patch looks good to commit - thanks!
Comment #32
DamienMcKennaThank you Jennifer.
Comment #33
jhodgdonThanks again! This has been (finally) committed to 6.x.