Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double quote usage #4

Open
weierophinney opened this issue Dec 31, 2019 · 9 comments
Open

Double quote usage #4

weierophinney opened this issue Dec 31, 2019 · 9 comments

Comments

@weierophinney
Copy link
Member

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.


Originally posted by @kynx at zendframework/zend-coding-standard#18

@weierophinney
Copy link
Member Author

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


Originally posted by @geerteltink at zendframework/zend-coding-standard#18 (comment)

@weierophinney
Copy link
Member Author

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.


Originally posted by @geerteltink at zendframework/zend-coding-standard#18 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @kynx at zendframework/zend-coding-standard#18 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @geerteltink at zendframework/zend-coding-standard#18 (comment)

@weierophinney
Copy link
Member Author

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.


Originally posted by @Xerkus at zendframework/zend-coding-standard#18 (comment)

@weierophinney
Copy link
Member Author

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.


Originally posted by @weierophinney at zendframework/zend-coding-standard#18 (comment)

@geerteltink
Copy link
Member

@weierophinney Did you get a reply on your question? Maybe we could run a benchmark and see for ourselves.

@weierophinney
Copy link
Member Author

I never did get a reply, unfortunately. I'd love to have some benchmarks.

I definitely prefer using sprintf() or str_replace(), as both allow memoizing the string template as a constant or variable, and then passing variables to it. String interpolation has always felt a bit fraught with issues, but I'm willing to be in the minority here.

@kynx
Copy link

kynx commented Mar 17, 2020

@weierophinney tbh, the main place I use interpolation is in exception messages and one-off scripts. I raised the issue after an in-house argument over my exception messages. It blew over long ago :)

@geerteltink geerteltink removed this from the 2.0.0 milestone May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants