search

zendframework / zend-coding-standard

Zend Framework Coding Standard
BSD 3-Clause "New" or "Revised" License
35 stars 8 forks source link

Double quote usage #18

Closed kynx closed 1 year ago

kynx commented 5 years ago

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 commented 5 years ago

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

geerteltink commented 5 years ago

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 commented 5 years ago

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

geerteltink commented 5 years ago

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

Xerkus commented 5 years ago

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 commented 5 years ago

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:

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.

weierophinney commented 4 years ago

This repository has been closed and moved to laminas/laminas-coding-standard; a new issue has been opened at https://github.com/laminas/laminas-coding-standard/issues/4.

weierophinney commented 4 years ago

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: