Coding standards of string concatenation have changed
JohnAlbin - April 14, 2008 - 18:13
| Project: | Coder |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
See #245115: Fix Drupal's awkward coding standards for the . operator
Not sure if coder warns on $var . 'string', but coder should now warn if using the “old” style $var .'string'
I think the module could search for /\ \.["']/ and /["']\.\ / to find those code style issues.
#1
Note that this should be for 7.x, not 6.x.
#2
I don't think the current coding style takes core version into account... could prove to be a challenge. :p
#3
Well you'd only change the Drupal 7.x version of coder (when there is one) to use this coding standard, and leave the 5.x and 6.x versions of coder as is. Shouldn't be a problem.
Cheers,
Stella
#4
Here's an attempt at a patch (against HEAD). (Not tested.)
I wasn't sure whether the
0-9in the regex should go, and nor was I sure about how to change the#warning. So, yeah, let me know what you think.#5
This one goes a bit further than the last one, in that it also fixes a capitalisation of the
#warningas well as some newly introduced coding style issues. ;) (Still only worked on one file though.)#6
And here's a full roll-out, fixing the coding style throughout the module! Stuff was found using the "code style" test in the module, and everything seems to work fine, so if someone else gives it the thumbs up, I'd say it's RTBC. :) (Note that it's still against HEAD, so you'll have to replace
6.xwith7.xin coder.info to test it.)#7
Please change this issue's Component to Coder Format when fixed.
#8
Just looking at the patch in #6, I can see:
- if ($file = file_check_upload($fieldname . '_upload')) { // not ok+ if ($file = file_check_upload($fieldname .'_upload')) { // not ok
}
$v .= 'bugger'; // ok
- $a = $v .'bugger'; // ok
- $a = $v ."bugger"; // ok, but will throw performance warning
+ $a = $v . 'bugger'; // ok
+ $a = $v . "bugger"; // ok, but will throw performance warning
$a = $v.'bugger'; // not ok
$a = $some_func().'bugger'; // not ok
Line 2 seems to be doing the opposite of what needs to be done. And lines 9 & 10 don't have any space around the . operator.
#9
Oops. nevermind. I was looking at one of the tests/ files. So, of course, the formatting was wrong!
#10
John, the mentioned lines belong to Coder's SimpleTests.
#11
So, Drupal 6 core won't get the new concat coding style since a coding style change is not a bug fix.
Is there a way to suppress those concat warnings for core in Coder 6.x?
#12
@JohnAlbin: This is why I keep saying that this is against HEAD, and not against the DRUPAL-6--1 branch. The check won't even exist in the D6 version, thus no need to suppress it (per comment #3). I'm not sure it should go in yet, but as soon as they start thinking about going D7 in HEAD, this patch will either apply cleanly or (hopefully) mean that they won't have to do do it "from scratch".
#13
I agree it should not go in yet, there seems to be some more debate/yelling going on over at: #245115: Fix Drupal's awkward coding standards for the . operator :-)
But, IMO, the change means that all new contrib code should follow the updated coding style now. So, if we can figure out a way to not have coder barf when testing Drupal 6 core code, the patch should go into coder DRUPAL-6--1.
#14
Hm. A possibility could perhaps be to loosen the check for Coder D6+ and have a check in coder_7x.inc? (Once work starts on that, that is...)
(I'd also like for coder's HEAD to chase core's HEAD, btw, so that it could be used for (helping with) reviewing stuff in core. But that's a discussion for another issue.)
#15
+1 for a looser check against 6.x then tighten up again for 7.x - makes sense to me.
#16
By the way, from what I gather from the discussion over there and elsewhere, Konstantin's comment pretty much sums it up:
#17
@Fresno: Actually Konstantin was saying the patch should not be applied to Drupal core before 7.x. He wasn’t saying anything about how the coding standard should be applied.
Many people, including me, will be using the new coding standard when writing modules and themes for 6.x.
Loosening the concat checking for Coder 6 would be an acceptable compromise to me. And would eliminate the need to 1.) piss people off who used the old coding standard when writing D6 contrib, and 2.) piss people off who are already using the new coding standard when writing D6 contrib. :-)
#18
Per #288771: Appending syntax.
Any update?
#19
Given that this is now a standard that isn't quite standard, I think that moving the string concatenation rules into their own standard makes sense. And this way, people can select either the 6.x or the 7.x standard by selecting which review to run.
#20
Hrm. Really? That seems like it'll just clutter the UI.
It seems like the logic of this would be very easy:
6.x:
If concatenation is done like this:
'string'. $varOR like this:'string' . $var, it's fine. Anything else is a bug.7.x:
If concatenation is done like this:
'string' . $var, it's fine. Anything else is a bug.#21
Yes, it does clutter the UI some in 6.x. Some people want to start writing their modules to the new standard, and are complaining about the coder warnings. I think that in 7.x, we'd remove the option, and move everything back into the style review.
#22
"Some people want to start writing their modules to the new standard, and are complaining about the coder warnings. "
Right. So why can't the logic change for 6.x as mentioned in #20?
#23
The proposal in #20 seems good to me. I know it'll mean we'll have different include files for D6 and D7 now, but that's a minor inconvenience. Also, for usability reasons, we may not wish to add the extra option - not necessarily because it clutters the UI, but because users unfamiliar with coder and coding standards may be confused by the choice.
#24
I also like the plan in #20.
#25
Looks like I'm outvoted here. I think that you're basically asking me to comment out one of the rules in 6.x. I'd like some tests to go with this. (Actually, I discovered that the simpletests are broken today, and I'm going to enforce a code-freeze until we fix all of those tests.)
#26
Agree with logic.
#27
If my understanding of this is correct, then in D6 there always has to be :
* a space between a $var and the dot
* no space between a "string" and a dot
Whereas in D7, there needs to be a space on either side of the dot.
So we're changing the D6 check in coder so it's flexible regarding the space between the "string" and the dot, but still enforcing that there has to be a space between a $var and the dot.
If so, then the above patch doesn't work because it doesn't warn about these lines:
$var = $str1. $str2; // Not ok$var = $str1. "foo"; // Not ok
All the other cases I tried work as expected.
Cheers,
Stella
#28
I also think this bit is wrong in the commented out review:
- '#value' => '(\.\s\'\'|\'\'\s\.|\.\s""|\.\s"")',+ '#value' => '(\.\'\'|\'\'\.|\.""|\."")',
Shouldn't it be:
- '#value' => '(\.\s\'\'|\'\'\s\.|\.\s""|""\s\.)',+ '#value' => '(\.\'\'|\'\'\.|\.""|""\.)',
#29
agree with Stella (missing some concatenation issues otherwise). I used the attached patch (uncommented) to port a module to D7 concatenation style. the patch also updates the warning message accordingly.
#30
What's the official status of this?
Any chance it will get in soon?
#31
Seems like the regex
(\.(?:|\s{2,})[\'\"]|[\'\"](?:|\s{2,})\.)would be more appropriate. Tests for either no space or more than one space between the dot and quotation character.#32
FYI: The coder_format script fully adheres to the new standard now. Committed attached patch.
#33
Can I ask what the status of this issue is for D6? I think the suggestion in #20 should be implemented; I never knew of the old D6 guideline (without space). I just started using Coder, but I get way too many errors for it to be useful...
#34
+1
I also get too many string concatenation errors for coder to be very useful. Is there a reason that this hasn't been fixed? I feel like a lot of people are going to be writing 6.x code to the new standard. Couldn't we just put a radio selector in the coder settings and allow people to choose what standard they want their code checked against? At least until 7.x?
#35
/me wonders why this still has not been delt with.
#36
Changes committed to both 6.x branches and 7.x.
Drupal 6 - rules have been relaxed, so they should allow both spaces and no spaces on either side of the dot with quotes. A space is still enforced for non-quote terms.
Drupal 7 - rule changed to enforce a space on both sides of the dot, and exactly one space (thanks Dave Reid!)
#37
Automatically closed -- issue fixed for 2 weeks with no activity.