Page MenuHomePhabricator

Add a linter rule for `php_sapi_name()`
AbandonedPublic

Authored by joshuaspence on Apr 8 2015, 1:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 5:11 PM
Unknown Object (File)
Fri, Dec 6, 8:26 AM
Unknown Object (File)
Sun, Dec 1, 5:49 PM
Unknown Object (File)
Sun, Nov 24, 4:48 PM
Unknown Object (File)
Nov 22 2024, 11:29 AM
Unknown Object (File)
Nov 21 2024, 3:39 AM
Unknown Object (File)
Nov 16 2024, 7:02 PM
Unknown Object (File)
Nov 10 2024, 12:06 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T7409: Implement PHPCS lints
Summary

Ref T7409. Create a linter rule to prefer PHP_SAPI_NAME in favor of php_sapi_name(). Based on Generic_Sniffs_PHP_SAPIUsageSniff.

Test Plan

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

Unit TestsFailed

TimeTest
1,161 msArcanistPuppetLintLinterTestCase::testLinter
725 msArcanistCSSLintLinterTestCase::testLinter
113 msArcanistCSSLintLinterTestCase::testVersion
187 msArcanistChmodLinterTestCase::testLinter
317 msArcanistClosureLinterTestCase::testLinter
View Full Test Results (1 Failed · 48 Passed · 4 Skipped)

Event Timeline

joshuaspence retitled this revision from to Add a linter rule for `php_sapi_name()`.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley edited edge metadata.

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.

This revision now requires changes to proceed.Apr 8 2015, 2:13 PM

Yeah, I pretty much agree with you on this.