Explicitly cast int to string when expected by PHP functions
ClosedPublic
Actions

Authored by aklapper on Mon, Jun 9, 20:55.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Commits

rP6bcc10fbf1db: Explicitly cast int to string when expected by PHP functions

Summary

Avoid Parameter $foo of function bar expects string, int[|string] given output in static code analysis by explicitly casting to string when a return value is an int, or by passing a string by encapsulating an int with apostrophes.

Test Plan

Run static code analysis.

Diff Detail

Repository

rP Phorge

Branch

castInt2String (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 2087
Build 2087: arc lint + arc unit

Event Timeline

aklapper created this revision.Mon, Jun 9, 20:55

Owners added a reviewer: O1: Blessed Committers.Mon, Jun 9, 20:55

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptMon, Jun 9, 20:55

aklapper requested review of this revision.Mon, Jun 9, 20:55

Harbormaster completed remote builds in B2084: Diff 3197.Mon, Jun 9, 20:55

valerio.bozzolan mentioned this in T16101: Differential+Diffusion+Diviner: add support for bcpow, bcadd, bcpow, etc..Mon, Jun 9, 22:08

Note that Phorge has phutil_string_cast() that does the (string) cast, probably useful somewhere, especially from points where alien objects could arrive from the ski (e.g. database entities).

Documentation we are proud about:

https://we.phorge.it/book/flavor/article/php_pitfalls/#check-for-non-empty-strings

I've flagged as ✅ the places where I'm 100% sure that your patch is 100% correct and phutil_string_cast() would have no sense. Unsure about the rest, need more time,

src/applications/aphlict/management/PhabricatorAphlictManagementWorkflow.php
395	✅ native cast has sense
src/applications/config/check/PhabricatorWebServerSetupCheck.php
46	✅ native string has sense
59	✅ native string has sense
src/applications/differential/controller/DifferentialRevisionAffectedPathsController.php
90	✅ native string has sense
src/applications/feed/PhabricatorFeedStoryPublisher.php
324	✅ native string has sense
src/applications/feed/storage/PhabricatorFeedStoryData.php
48	✅ native string has sense
src/applications/files/engine/PhabricatorLocalDiskFileStorageEngine.php
50	✅ native cast has sense
src/applications/metamta/controller/PhabricatorMetaMTAMailViewController.php
197	✅ native string has sense
src/applications/phrequent/storage/PhrequentTimeBlock.php
262	✅ native string has sense
src/applications/search/engine/PhabricatorSearchNgramEngine.php
39–55	Maybe early cast? Still unclear which cast action but maybe just once
src/applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php
123	✅ native string has sense
src/applications/xhprof/view/PhabricatorXHProfProfileSymbolView.php
96	✅ native string has sense

valerio.bozzolan added inline comments.Tue, Jun 10, 07:00

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
406	✅ Seems super good
src/applications/search/engine/PhabricatorSearchNgramEngine.php
33	↑ Here, the `$token` is a string, but PHP is silly and it can become an integer https://we.phorge.it/book/flavor/article/php_pitfalls/#php-casts-all-digit-array-keys
39–55	I can confirm: yes for the early cast, yes it's the correct cast.

Thanks for the review!

In D26074#28501, @valerio.bozzolan wrote:

Note that Phorge has phutil_string_cast() that does the (string) cast, probably useful somewhere, especially from points where alien objects could arrive from the ski (e.g. database entities).

So there is phutil_string_cast() and PHP's (string) cast and PHP's strval() method...sigh. :)

cast earlier in PhabricatorSearchNgramEngine

Harbormaster completed remote builds in B2087: Diff 3200.Tue, Jun 10, 08:59

Check if you like the bonus point

src/aphront/sink/AphrontHTTPSink.php
36–38	This check is funny. We can get rid of this regular expression, early cast to int (moving up old line 40) and just do `if ($code < 100 \|\| $code > 999)`, that is, checking if the integer has not 3 digits. Incidentally faster. Let's be sure to run this unit test, in case ./src/aphront/sink/__tests__/AphrontHTTPSinkTestCase.php

This revision is now accepted and ready to land.Wed, Jun 11, 18:53

aklapper added inline comments.Wed, Jun 11, 20:32

src/aphront/sink/AphrontHTTPSink.php
36–38	@valerio.bozzolan Yes but... :) Your proposed change also changes semantics - if the incoming `$code` parameter was something like `372bleh` it would not fail anymore as before but pass instead due to casting to the int 372, I think. (Yes, test passes with your proposed change.)