Fix T15516
Details
Details
- Reviewers
speck valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15516: Regression in PhabricatorStandardCustomField.php:304 - field can be an integer
- Commits
- rP603cf474ee1d: Fix int fields for now
Viewed an "int" custom field, no boom.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 627 Build 627: arc lint + arc unit
Event Timeline
Comment Actions
(looks like most other custom fields already override this method with something useful).
Comment Actions
I mean, so the goal of the cast is to be able to operate strictly with strings, so finally we can if($v === '') without any pitfall
Comment Actions
Yeah but I don't see that readability bonus point here since we have a "not non-empty" condition.
I also think that we have the great advantage of having Evan who created phutil_string_cast() to make it super-stable to only return strings. Also thanks to your unit tests, we can surely rely on if($value_str === '') and I somehow think in this way we can reach a level of clarity and readability OH MY GOSH WITHOUT PRECEDENT. BUY PHORGE, FROM TODAY WITH 0.01% MORE STRICT CONTROLS, FREE.
Comment Actions
How about:
if (nonempty($value)) {
return $value;
}
return null;
}I also think that we have the great advantage of having Evan who created phutil_string_cast() to make it super-stable to only return strings. Also thanks to your unit tests, we can surely rely on if($value_str === '') and I somehow think in this way we can reach a level of clarity and readability OH MY GOSH WITHOUT PRECEDENT. BUY PHORGE, FROM TODAY WITH 0.01% MORE STRICT CONTROLS, FREE.
I literally can't understand this paragraph. I don't know what you're trying to say here.
Using $value === '' would be only place in the code that does that, and it would be surprising/unclear to readers why we only care about explicitly empty string in this particular case. They'll have to know that string_cast never returns null, and then understand this is a micro-optimization.
Comment Actions
Using $value === '' would be only place in the code that does that
Hoping to be useful I just want to highlight that strict checks were already in use by Phabricator, and Phorge. Some examples:
$ grep -R "=== ''" --include="*.php" . ./src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php: if (last($parts) === '') { ./src/applications/harbormaster/controller/HarbormasterBuildViewController.php: if (count($lines) === 1 && trim($lines[0]) === '') { ./src/applications/phriction/editor/PhrictionTransactionEditor.php: if ($content === '') { ./src/applications/metamta/stamp/PhabricatorStringMailStamp.php: if ($value === null || $value === '') { ./src/applications/auth/provider/PhabricatorLDAPAuthProvider.php: if ($old === null || $old === '') { ./src/applications/maniphest/xaction/ManiphestTaskPointsTransaction.php: $is_empty = phutil_string_cast($value) === ''; // <---------- ./src/applications/transactions/storage/PhabricatorModularTransactionType.php: return $value_clean === ''; ./src/applications/transactions/storage/PhabricatorApplicationTransaction.php: if ($old === '' || $old === null) { ./src/applications/transactions/editengine/PhabricatorEditEngine.php: if ($first_name === null || $first_name === '') { ./src/applications/transactions/editfield/PhabricatorEditField.php: if ($value === '') { ./src/applications/search/engine/PhabricatorApplicationSearchEngine.php: $is_empty_query_key = phutil_string_cast($query_key) === ''; // <---------- ./src/applications/project/storage/PhabricatorProject.php: if ($phid === null || $phid === '') { ./src/applications/project/storage/PhabricatorProject.php: if ($path_key === null || $path_key === '') { ./src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php: if ($name === null || $name === '') { ./src/applications/diviner/markup/DivinerSymbolRemarkupRule.php: if ($value === '') { ./src/applications/config/view/PhabricatorSetupIssueView.php: } else if ($value === '') { ./src/infrastructure/markup/blockrule/PhutilRemarkupInterpreterBlockRule.php: while (trim($lines[$last_key]) === '') { ./src/infrastructure/markup/blockrule/PhutilRemarkupInterpreterBlockRule.php: if (trim($lines[$first_key]) === '') { ./src/infrastructure/markup/blockrule/PhutilRemarkupInterpreterBlockRule.php: if (trim($lines[$last_key]) === '') { ./src/infrastructure/markup/blockrule/PhutilRemarkupTableBlockRule.php: if ($content === null || trim($content) === '') { $ grep -R "!== ''" --include="*.php" . ./resources/sql/autopatches/20160531.pref.07.phidval.php: if ($row->getPHID() !== '') { ./src/applications/auth/factor/PhabricatorAuthFactor.php: if ($sync_key !== '') { ./src/applications/config/option/PhabricatorCoreConfigOptions.php: if ($path !== '' && $path !== '/') { ./src/applications/config/option/PhabricatorSecurityConfigOptions.php: if ($path !== '' && $path !== '/') { ./src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php: $is_nonempty = phutil_string_cast($value) !== ''; // <---------- ./src/view/form/control/AphrontFormControl.php: $has_caption = phutil_string_cast($this->getCaption()) !== ''; // <----------
Also I would like to highlight that, when talking about PHP, and talking about casting to string - (string) $v - the result always is a string, never NULL. And fortunately, so it does phutil_string_cast().
Last word to you but it would be strange to have to check NULL after a string cast - so using phutil_nonempty_string(phutil_string_cast($v)) is probably somehow too intensive use of our utilities.
Comment Actions
Explicit is better then implicit.
You might know that "in php, casting to string is never null", but I wouldn't know that, and many other readers that aren't experts in php wouldn't know that.
The goal in the code is to minimize the cognitive load on the reader, and having a check explicitly called "non-empty-string" is minimizing that.
Comment Actions
Yeah no problem, I just wanted to understand and share each other points.
As compromise I think that inverting the cases could be an extra readability point. So, as you suggested:
... if (phutil_nonempty_string($value_str)) { return $value_str; } return null;
Comment Actions
Thanks for the fix! <3
For the records, yes, we are aware that phutil_nonempty_string($v) is just a syntax sugar and that here it is equivalent to $v !== ''
