Fix 'Drupal.Strings.UnnecessaryStringConcat' coding standard [#2902412]

Part of meta-issue #2571965: [meta] Fix PHP coding standards in core

Step 1: Preparation

Open the file core/phpcs.xml.dist and add a line for the sniff of this ticket. The sniff name is in the issue title. Make sure your patch will include the addition of this line.

Step 2: Install & configure PHPCS

Install PHP CodeSniffer and the ruleset from the Coder module:

$ composer install
$ ./vendor/bin/phpcs --config-set installed_paths ../../drupal/coder/coder_sniffer

Once you have installed the phpcs package, you can list all the sniffs available to you like this:

$ ./vendor/bin/phpcs --standard=Drupal -e

This will give you a big list of sniffs, and the Drupal-based ones should be present.

Step 3: Prepare the phpcs.xml file

To speed up the testing you should make a copy of the file phpcs.xml.dist (in the core/ folder) and save it as phpcs.xml. This is the configuration file for PHP CodeSniffer.

We only want this phpcs.xml file to specify the sniff we're interested in. So we need to remove all the rule items, and add only our own sniff's rule. Rule items look like this:

<rule ref="Drupal.Strings.UnnecessaryStringConcat"/>

Remove all of them, and add only the sniff from this issue title. This will make sure that our tests run quickly, and are not going to contain any output from unrelated sniffs.

Step 4: Run the test

Now you are ready to run the test! From within the core/ folder, run the following command to launch the test:

$ cd core/
$ ../vendor/bin/phpcs -p

This takes a couple of minutes. The -p flag shows the progress, so you have a bunch of nice dots to look at while it is running.

Step 5: Fix the failures

When the test is complete it will present you a list of all the files that contain violations of your sniff, and the line numbers where the violations occur. You could fix all of these manually, but thankfully phpcbf can fix many of them. You can call phpcbf like this:

$ ../vendor/bin/phpcbf

This will fix the errors in place. You can then make a diff of the changes using git. You can also re-run the test with phpcs and determine if that fixed all of them.

Comment	File	Size	Author
#13	2902412-13.patch	12.25 KB	andypost
#13	8.7.x: PHP 7.2 & MySQL 5.5 24,426 pass
#13	interdiff.txt	6.26 KB	andypost
#5	2902412-5.patch	10.54 KB	jofitz
#5	8.5.x: PHP 7 & MySQL 5.5 Patch Failed to Apply 8.7.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply
#5	interdiff-4-5.txt	4.16 KB	jofitz
#4	2902412-4.patch	6.37 KB	jofitz
#4	8.5.x: PHP 7 & MySQL 5.5 22,175 pass
#2	drupal-coding-standards-2902412-2.patch	6.44 KB	mfernea
#2	8.5.x: PHP 7 & MySQL 5.5 22,150 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

15 August 2017 at 22:04

mfernea created an issue. See original summary.

Comment #2

mfernea CreditAttribution: mfernea at AmeXio commented 16 August 2017 at 11:54

Status:	Active	» Needs review
Issue tags:	-Novice

File	Size
drupal-coding-standards-2902412-2.patch	6.44 KB
8.5.x: PHP 7 & MySQL 5.5 22,150 pass

Here is a partial patch. I'm not sure how to approach the errors in DbDumpCommand.php and HtmlToTextTest.php.

Comment #3

andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented 11 September 2017 at 10:38

Status:

Needs review

» Needs work

Need small reroll for phpcs.xml.dist file
To cover the errors in DbDumpCommand.php and HtmlToTextTest.php files let's use following construction

$combined_string = implode('', [
  'part1',
  'part2',
  ...
]);

Comment #4

jofitz CreditAttribution: jofitz at ComputerMinds commented 11 September 2017 at 11:56

Status:

Needs work

» Needs review

File	Size
2902412-4.patch	6.37 KB
8.5.x: PHP 7 & MySQL 5.5 22,175 pass

Simple re-roll before addressing @andriyun's suggestion in #3.

Comment #5

jofitz CreditAttribution: jofitz at ComputerMinds commented 11 September 2017 at 13:12

File	Size
interdiff-4-5.txt	4.16 KB
2902412-5.patch	10.54 KB
8.5.x: PHP 7 & MySQL 5.5 Patch Failed to Apply 8.7.x: PHP 7.2 & MySQL 5.5 Patch Failed to Apply

File

Size

interdiff-4-5.txt

4.16 KB

2902412-5.patch

10.54 KB

8.5.x:
PHP 7 & MySQL 5.5 Patch Failed to Apply

8.7.x:
PHP 7.2 & MySQL 5.5 Patch Failed to Apply

Resolve coding standards errors in DbDumpCommand.php and HtmlToTextTest.php.

Comment #6

andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented 11 September 2017 at 13:34

Status:

Needs review

» Reviewed & tested by the community

LGTM

Comment #7

andriyun CreditAttribution: andriyun at Skilld, Drupal Ukraine Community commented 11 September 2017 at 13:34

3 files were hidden/shown/deleted

File	Size
drupal-coding-standards-2902412-2.patch	6.44 KB
8.5.x: PHP 7 & MySQL 5.5 22,150 pass
2902412-4.patch	6.37 KB
8.5.x: PHP 7 & MySQL 5.5 22,175 pass
interdiff-4-5.txt	4.16 KB

Comment #8

mfernea CreditAttribution: mfernea at AmeXio commented 11 September 2017 at 14:49

Good idea @andriyun! Looks good to me too. Thanks Jo! :)

Comment #9

xjm

she/her

English

CreditAttribution: xjm at Acquia commented 12 September 2017 at 22:33

Status:

Reviewed & tested by the community

» Needs review

Hmm, I'm not sure about using implode('',...) with a static list of strings. It adds a function call to get around a coding standard. We don't do that anywhere else in core; all the others are with a variable:

grep -r "implode(''," *
core/lib/Drupal/Core/Asset/CssCollectionOptimizer.php:              $data = implode('', $matches[0]) . $data;
core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php:    return implode('', $args);
core/lib/Drupal/Core/Mail/MailFormatHelper.php:            $output .= static::wrapMail('', implode('', $indent)) . "\n";
core/lib/Drupal/Core/Mail/MailFormatHelper.php:        $output .= static::wrapMail($chunk, implode('', $indent)) . $line_endings;
core/modules/big_pipe/src/Render/BigPipe.php:    $pre_body = implode('', $parts);
core/modules/locale/locale.module:  return implode('', preg_split('~(?<!\\\\)[\'"]\s*\+\s*[\'"]~s', substr($string, 1, -1)));
core/modules/locale/src/Form/TranslateEditForm.php:      $new_translation_string = implode('', $new_translation['translations']);
core/modules/search/search.module:      $tokens .= implode('', $chars) . ' ';
core/modules/search/tests/src/Functional/SearchTokenizerTest.php:    $string = implode('', $chars);
core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php:    $expected = implode('', $expected);
core/themes/engines/twig/twig.engine:  return Markup::create(implode('', $output));

It looks like it's just two places that this is being done for readability, and one is a fixture for a test, whereas the other one is generating a command for a fixture for a test.

I think HtmlToTextTest can probably just inline them. Less sure about the best change for the DB dump command one since it also includes actual linebreaks; maybe it could use EOF format or whatnot. (Do we have any coding standards that prefer not using EOF format/multi-line strings)?

Setting NR for further discussion.

Comment #10

12 September 2017 at 22:34

Version:

8.5.x-dev

» 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #11

borisson_

Dutch

Mechelen, 🇧🇪

CreditAttribution: borisson_ as a volunteer commented 1 June 2018 at 08:42

I think the EOF format is a good idea here, I don't know of any coding standard preventing that.

Comment #12

1 June 2018 at 08:42

Version:

8.6.x-dev

» 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Comment #13

andypost

he/him

Russian

CreditAttribution: andypost as a volunteer and at Skilld commented 8 October 2018 at 14:21

Issue summary:

View changes

File	Size
interdiff.txt	6.26 KB
2902412-13.patch	12.25 KB
8.7.x: PHP 7.2 & MySQL 5.5 24,426 pass

Re-roll & address #9 with http://php.net/manual/en/language.types.string.php#language.types.string...

2 places in \Drupal\Tests\system\Functional\Mail\HtmlToTextTest require str_replace(["\r", "\n"], '', $input) because the case ensures that new lines actually added by text processing

Comment #14

borisson_

Dutch

Mechelen, 🇧🇪

CreditAttribution: borisson_ as a volunteer commented 9 October 2018 at 06:51

Status:

Needs review

» Reviewed & tested by the community

This looks very good now, great work y'all!

Comment #15

9 October 2018 at 07:08

The last submitted patch, 5: 2902412-5.patch, failed testing. View results

Comment #16

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 8 November 2018 at 15:51

Status:

Reviewed & tested by the community

» Fixed

Committed e6a0d58 and pushed to 8.7.x. Thanks!

Comment #17

8 November 2018 at 15:51

catch committed 8ea1c48 on 8.7.x

Issue #2902412 by Jo Fitzgerald, andypost, mfernea, andriyun: Fix '...

Comment #18

22 November 2018 at 15:54

Status:

Fixed

» Closed (fixed)

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

Comment #19

quietone CreditAttribution: quietone at Acro Commerce commented 26 November 2018 at 02:51

It has taken a while but finally tracked all the unexpected changes to the database dumps to this patch. If an existing dump (currently using one in Commerce Migrate) is imported and then immediately exported the files no longer match and they should.

It probably isn't worth modifying the code that this patch change, unless someone else thinks so, but the migrate dumps need to be rerolled because of this. And I will be doing the same for all the dumps in Commerce Migrate as well.