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.

CommentFileSizeAuthor
#32 coder-HEAD.format-concatenation.patch2.89 KBsun
#29 246568_2.patch1.14 KBscor
#25 246568.patch1015 bytesdouggreen
#6 new_concatenation_style.d7.patch19.5 KBFreso
#5 new_concatenation_style.d7.patch1.84 KBFreso
#4 new_concatenation_style.d7.patch1.06 KBFreso

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Note that this should be for 7.x, not 6.x.

Freso’s picture

Freso commented

I don't think the current coding style takes core version into account... could prove to be a challenge. :p

stella’s picture

stella commented

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

Freso’s picture

Freso commented
Status: Active » Needs review
StatusFileSize
new new_concatenation_style.d7.patch1.06 KB

Here's an attempt at a patch (against HEAD). (Not tested.)

I wasn't sure whether the 0-9 in the regex should go, and nor was I sure about how to change the #warning. So, yeah, let me know what you think.

Freso’s picture

Freso commented
StatusFileSize
new new_concatenation_style.d7.patch1.84 KB

This one goes a bit further than the last one, in that it also fixes a capitalisation of the #warning as well as some newly introduced coding style issues. ;) (Still only worked on one file though.)

Freso’s picture

Freso commented
StatusFileSize
new new_concatenation_style.d7.patch19.5 KB

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.x with 7.x in coder.info to test it.)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Please change this issue's Component to Coder Format when fixed.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Needs review » Needs work

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.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented
Status: Needs work » Needs review

Oops. nevermind. I was looking at one of the tests/ files. So, of course, the formatting was wrong!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

John, the mentioned lines belong to Coder's SimpleTests.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

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?

Freso’s picture

Freso commented

@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".

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

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.

Freso’s picture

Freso commented

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.)

catch’s picture

catch
he/him
Primary language English
commented

+1 for a looser check against 6.x then tighten up again for 7.x - makes sense to me.

Freso’s picture

Freso commented

By the way, from what I gather from the discussion over there and elsewhere, Konstantin's comment pretty much sums it up: This patch was never intended to be applied to Drupal before 7.x. It should not be applied to any code that is related with Drupal before 7.x.

johnalbin’s picture

johnalbin
they/他
Primary language English
Location Taipei, Taiwan
commented

@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. :-)

boombatower’s picture

boombatower commented

Per #288771: Appending syntax.

Any update?

douggreen’s picture

douggreen commented
Status: Needs review » Needs work

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

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'. $var OR 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.

douggreen’s picture

douggreen commented

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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

"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?

stella’s picture

stella commented

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.

catch’s picture

catch
he/him
Primary language English
commented

I also like the plan in #20.

douggreen’s picture

douggreen commented
Status: Needs work » Needs review
StatusFileSize
new 246568.patch1015 bytes

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.)

boombatower’s picture

boombatower commented

Agree with logic.

stella’s picture

stella commented

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

stella’s picture

stella commented
Status: Needs review » Needs work

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' => '(\.\'\'|\'\'\.|\.""|""\.)',
scor’s picture

scor commented
Status: Needs work » Needs review
StatusFileSize
new 246568_2.patch1.14 KB

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.

boombatower’s picture

boombatower commented

What's the official status of this?

Any chance it will get in soon?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new coder-HEAD.format-concatenation.patch2.89 KB

FYI: The coder_format script fully adheres to the new standard now. Committed attached patch.

svdoord’s picture

svdoord commented

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...

jordanmagnuson’s picture

jordanmagnuson commented

6.x:
If concatenation is done like this: 'string'. $var OR 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.

+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?

boombatower’s picture

boombatower commented

/me wonders why this still has not been delt with.

stella’s picture

stella commented
Status: Needs review » Fixed

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!)

Status: Fixed » Closed (fixed)

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