Ref T7409. Create a linter rule to prefer PHP_SAPI_NAME in favor of php_sapi_name(). Based on Generic_Sniffs_PHP_SAPIUsageSniff.
Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T7409: Implement PHPCS lints
Added unit tests.
Diff Detail
- Repository
- rARC Arcanist
- Branch
- master
- Lint
Lint Passed - Unit
Test Failures - Build Status
Buildable 5196 Build 5214: [Placeholder Plan] Wait for 30 Seconds
Time | Test | |
---|---|---|
1,161 ms | ArcanistPuppetLintLinterTestCase::testLinter | |
725 ms | ArcanistCSSLintLinterTestCase::testLinter | |
113 ms | ArcanistCSSLintLinterTestCase::testVersion | |
187 ms | ArcanistChmodLinterTestCase::testLinter | |
317 ms | ArcanistClosureLinterTestCase::testLinter | |
View Full Test Results (1 Failed · 48 Passed · 4 Skipped) |
Event Timeline
I think we should go the other way on this? PHP_SAPI_NAME feels more magical/surprising to me, and we consistently use php_sapi_name() in the code. Here are some reasons why functions are preferable to constants in the general case (although not necessarily terribly relevant in this case):
- If you typo, you get a nice hard failure on a bad function call; you may get a soft wrong behavior with a constant.
- You can pass a function call as a callback.
- Functions can be checked by static analyzers like PHPCompatInfo, constants can not.
- Functions show up on profiles.
- Future versions of things may introduce parameters, meaning the function is the only way to accomplish something.
- Functions generally have better integration with ctags, documentation, IDEs, etc. For example php.net/php_sapi_name gives you function documentation; php.net/SAPI_NAME does not give you constant documentation.
They're slightly better if you do a SAPI check 100,000 times or something, since they have lower overhead, but your code is wrong no matter which form you're using, and the correct fix is to identify the issue with a profiler and memoize the result of the computation (or whatever), not decrease the constant factor very slightly by removing a minuscule amount of call overhead. Because functions show up on profiles, it's arguably better to have a function: although it will make the problem worse, it will also make the problem easier to identify and fix.
This is also wrong; the constant is PHP_SAPI not PHP_SAPI_NAME.
Even from the point of view of consistency, this feels like one of the least useful lint rules we could possibly write; we have 5 total callsites in 500K lines of code.