Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Double quote usage #18

Closed
wants to merge 3 commits into from
Closed

Double quote usage #18

wants to merge 3 commits into from

Conversation

kynx
Copy link

@kynx kynx commented Apr 30, 2019

Squiz.Strings.DoubleQuoteUsage.ContainsVar prevents strings containing variables. This doesn't match the examples in the documentation and, given that PHP7s interpolation uses less memory than concatenation, probably isn't intended.

This PR adds an exclude for that check and tries to make the docs a little more exact.

@geerteltink
Copy link
Member

geerteltink commented May 1, 2019

... given that PHP7s interpolation uses less memory than concatenation ...

I completely missed that part. Here's an in depth review: https://blog.blackfire.io/php-7-performance-improvements-encapsed-strings-optimization.html

$string = '\$var';
$query = "SELECT * FROM table WHERE name =''";
```

*Invalid: There are no variables inside double quote strings.*
*Invalid: There are no variables, control characters inside double quote strings*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to: "Invalid: There are no variables or control characters inside double quote strings."

```php
<?php
$string = "Hello There\r\n";
$string = "Hello $there";
$string = 'Hello There';
$string = 'Hello'.' There'."\n";
$string = 'Hello' . ' There' . "\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should remove this example as interpolation would be better. In this case it would end up the same as the first example: "Hello There\n";

@geerteltink
Copy link
Member

You need to update test/expected-report.txt as the report is different after changing the rules. Not sure how I did it, but I guess I ran the tests and just copy pasted phpcs.log into that file.

@kynx
Copy link
Author

kynx commented May 1, 2019

Thanks for the review @xtreamwayz . It'd be worth keeping an eye on squizlabs/PHP_CodeSniffer#2259 for a more comprehensive sniff.

@geerteltink
Copy link
Member

I saw that this morning. Looks like it's just an idea. No PR yet.

@Xerkus
Copy link
Member

Xerkus commented May 1, 2019

I believe our rule is that double quoted string can not contain vars or other variable interpolation and sprintf should be used instead for reasons related to QA and not performance.
This coding standard rule reflects the rule we've been following in our reviews, so I say it should stay.

@weierophinney
Copy link
Member

I believe our rule is that double quoted string can not contain vars or other variable interpolation and sprintf should be used instead for reasons related to QA and not performance.
This coding standard rule reflects the rule we've been following in our reviews, so I say it should stay.

The main reasons I've advocated usage of sprintf() are:

  • performance
  • readability

Both of these were informed by performance of previous PHP versions with regards to string interpolation. Interpolation was known to be slow, and escaping rules tend to get messy. Concatenation is predictable, but hard to read. sprintf() semantics are relatively easy to understand, and generally performant, making it a good solution to these problems. (For the record, str_replace() using named placeholders is also quite performant.)

However, if PHP 7 solves most of the performance issues, string interpolation would likely be a viable choice again. As such, I've got an open question to Julien Pauli asking what the performance differences are between the two approaches.

@geerteltink geerteltink added this to the 2.0.0 milestone Oct 17, 2019
@geerteltink geerteltink mentioned this pull request Oct 17, 2019
7 tasks
@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-coding-standard; a new issue has been opened at laminas/laminas-coding-standard#4.

@weierophinney
Copy link
Member

This repository has been moved to laminas/laminas-coding-standard. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

  • Squash all commits in your branch (git rebase -i origin/{branch})
  • Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
  • Run the laminas/laminas-migration tool on the code.
  • Clone laminas/laminas-coding-standard to another directory.
  • Copy the files from the second bullet point to the clone of laminas/laminas-coding-standard.
  • In your clone of laminas/laminas-coding-standard, commit the files, push to your fork, and open the new PR.
    We will be providing tooling via laminas/laminas-migration soon to help automate the process.

@kynx kynx closed this by deleting the head repository Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants