Page MenuHomePhorge

Fix int fields for now
ClosedPublic

Authored by avivey on Jul 3 2023, 18:06.

Details

Diff Detail

Repository
rP Phorge
Branch
master
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 624
Build 624: arc lint + arc unit

Event Timeline

avivey requested review of this revision.Jul 3 2023, 18:06

(looks like most other custom fields already override this method with something useful).

Note that after phutil_string_cast(), NULL is never returned, but just a string

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

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

I think I prefer the explicit "nonempty" call, for clearity.

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.

Yeah but I don't see that readability bonus point here since we have a "not non-empty" condition.

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.

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.

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.

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;

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 !== ''

sgtm

This revision is now accepted and ready to land.Jul 4 2023, 09:07
This revision was automatically updated to reflect the committed changes.