Replace intval(), strval(), .. *val() functions with their relevant type casting operators

Hi, Ayesh, very glad to meet an issue you created. I did do benchmarks by phpbench , with PHP 7.1, 7.2 and 7.3. The results show that 7.2 has improved performance. and no obvious diff between the two ways under the tested PHP versions.

So I doubt that casting is only faster under PHP 5. If so, I think it's optional to replace it nowadays. But it's good to unify the way in Core.

See the details below, and the script attached.

---

Benchmark under PHP 7.1

2 subjects, 16 iterations, 45,056 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 1.587 [1.726 1.726] 1.604 (μs)
⅀T: 27.622μs μSD/r 0.053μs μRSD/r: 3.028%
suite: 1343abadc450cf699e4e3f4a86a6c3827d3bc82f, date: 2020-01-22, stime: 17:35:55
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| benchmark | subject | set | revs | iter | mem_peak | time_rev | comp_z_value | comp_deviation |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| CastVsTypeHit | benchCast | 3x9 | 1024 | 0 | 435,904b | 1.969μs | +1.00σ | +4.29% |
| CastVsTypeHit | benchCast | 3x9 | 1024 | 1 | 435,904b | 1.807μs | -1σ | -4.29% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 0 | 435,904b | 1.668μs | -1σ | -0.53% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 1 | 435,904b | 1.686μs | +1.00σ | +0.53% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 0 | 435,904b | 1.738μs | +1.00σ | +3.61% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 1 | 435,904b | 1.617μs | -1σ | -3.61% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 0 | 435,904b | 1.954μs | +1.00σ | +9.62% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 1 | 435,904b | 1.611μs | -1σ | -9.62% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 0 | 435,904b | 1.764μs | +1.00σ | +0.42% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 1 | 435,904b | 1.749μs | -1σ | -0.42% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 0 | 435,904b | 1.676μs | -1σ | -2.36% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 1 | 435,904b | 1.757μs | +1.00σ | +2.36% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 0 | 435,904b | 1.587μs | -1σ | -0.52% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 1 | 435,904b | 1.604μs | +1.00σ | +0.52% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 0 | 435,904b | 1.767μs | +1.00σ | +2.88% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 1 | 435,904b | 1.668μs | -1σ | -2.88% |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+

Benchmark result under PHP 7.2

2 subjects, 16 iterations, 45,056 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.224 [0.250 0.250] 0.229 (μs)
⅀T: 4.006μs μSD/r 0.010μs μRSD/r: 3.633%
suite: 1343abaf04d23c21b9c9d7d5a0aa8e8d88101f71, date: 2020-01-22, stime: 17:37:04
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| benchmark | subject | set | revs | iter | mem_peak | time_rev | comp_z_value | comp_deviation |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| CastVsTypeHit | benchCast | 3x9 | 1024 | 0 | 458,512b | 0.278μs | -1σ | -2.06% |
| CastVsTypeHit | benchCast | 3x9 | 1024 | 1 | 458,512b | 0.290μs | +1.00σ | +2.06% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 0 | 458,512b | 0.224μs | -1σ | -3.49% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 1 | 458,512b | 0.240μs | +1.00σ | +3.49% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 0 | 458,512b | 0.241μs | -1σ | -1% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 1 | 458,512b | 0.246μs | +1.00σ | +1.00% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 0 | 458,512b | 0.230μs | -1σ | -3.42% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 1 | 458,512b | 0.246μs | +1.00σ | +3.42% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 0 | 458,512b | 0.249μs | -1σ | -15.14% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 1 | 458,512b | 0.338μs | +1.00σ | +15.14% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 0 | 458,512b | 0.225μs | -1σ | -3.29% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 1 | 458,512b | 0.241μs | +1.00σ | +3.29% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 0 | 458,512b | 0.251μs | +1.00σ | +0.59% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 1 | 458,512b | 0.248μs | -1σ | -0.59% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 0 | 458,512b | 0.229μs | +1.00σ | +0.06% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 1 | 458,512b | 0.228μs | -1σ | -0.06% |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+

Benchmark result under PHP 7.3

2 subjects, 16 iterations, 45,056 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 0.228 [0.246 0.246] 0.233 (μs)
⅀T: 3.929μs μSD/r 0.004μs μRSD/r: 1.453%
suite: 1343aba58483a93833b8b6f36649ea59e46d26b2, date: 2020-01-22, stime: 17:37:42
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| benchmark | subject | set | revs | iter | mem_peak | time_rev | comp_z_value | comp_deviation |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+
| CastVsTypeHit | benchCast | 3x9 | 1024 | 0 | 462,432b | 0.247μs | -1σ | -0.78% |
| CastVsTypeHit | benchCast | 3x9 | 1024 | 1 | 462,432b | 0.251μs | +1.00σ | +0.78% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 0 | 462,432b | 0.228μs | -1σ | -5.99% |
| CastVsTypeHit | benchCast | 3x9 | 10240 | 1 | 462,432b | 0.257μs | +1.00σ | +5.99% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 0 | 462,432b | 0.250μs | +1.00σ | +0.39% |
| CastVsTypeHit | benchCast | -3x9 | 1024 | 1 | 462,432b | 0.248μs | -1σ | -0.39% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 0 | 462,432b | 0.233μs | +1.00σ | +0.19% |
| CastVsTypeHit | benchCast | -3x9 | 10240 | 1 | 462,432b | 0.232μs | -1σ | -0.19% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 0 | 462,432b | 0.246μs | -1σ | -0.79% |
| CastVsTypeHit | benchTypeHit | 3x9 | 1024 | 1 | 462,432b | 0.250μs | +1.00σ | +0.79% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 0 | 462,432b | 0.245μs | -1σ | -1.8% |
| CastVsTypeHit | benchTypeHit | 3x9 | 10240 | 1 | 462,432b | 0.254μs | +1.00σ | +1.80% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 0 | 462,432b | 0.248μs | +1.00σ | +1.20% |
| CastVsTypeHit | benchTypeHit | -3x9 | 1024 | 1 | 462,432b | 0.242μs | -1σ | -1.2% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 0 | 462,432b | 0.250μs | +1.00σ | +0.49% |
| CastVsTypeHit | benchTypeHit | -3x9 | 10240 | 1 | 462,432b | 0.248μs | -1σ | -0.49% |
+---------------+--------------+------+-------+------+----------+----------+--------------+----------------+

        yield '3x9' => [ 'value' => '999' ];

BTW, 3x9 what I meant is three bit of number 9. originally, I tested 999,999,999, in short 9x9

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

At this time we will need a D10 of this patch and also a recheck of the repo if there are other instances.

Please do not just reroll without checking.

Thank you. IF you open a MR you don't need to upload a patch fyi. Hiding the files to not confuse people

After applying patch #17 see 11 instances of intval still in the repo.

Confirmed I'm only seeing the 2 intvals() in the updater.php file now.

I think that if we're going to adopt this, we need a phpcs rule to keep these from creeping back in:
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Pro...

Thanks for adding the warning.

Confirmed it's working by editing a random test file and using the functions that I shouldn't and I did get the warning.

But will the warning cause commit checks to fail on DrupalCI, to prevent these from creeping back in?

Forking the MR to check.

Edit: BTW naming an MR branch 10.1.x is not a good idea; you should always prefix your feature branches with the issue ID. :)

Is it because they are warnings and not errors?

@smustgrave Right, that's what I was trying to confirm. And it works:

https://www.drupal.org/pift-ci-job/2576189

Has:

ILE: /var/www/html/core/modules/media/src/OEmbed/Resource.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 498 | WARNING | The use of function intval() is discouraged
     |         | (Generic.PHP.ForbiddenFunctions.Discouraged)
----------------------------------------------------------------------

I grepped for remaining usages:

[ayrton:drupal | Wed 04:38:58] $ grep -r "intval" * | grep -v "vendor" | grep -v "node_modules"
core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:      $ids = array_map('intval', $ids);
core/lib/Drupal/Core/Updater/Updater.php:            $filetransfer->chmod($parent_dir, intval($old_perms, 8));
core/lib/Drupal/Core/Updater/Updater.php:      $filetransfer->chmod($path, intval($new_perms, 8), $recursive);
core/phpcs.xml.dist:        <element key="intval" value="null"/>
core/modules/language/src/LanguageNegotiator.php:    $enabled_methods = array_map('intval', $enabled_methods);
core/modules/views/src/Plugin/views/HandlerBase.php:      $value = array_map('intval', $value);

For the usages in the updater, I think we should file a followup to consider converting those and remove the PHPCS ignore, because of this little bit of fun:

echo intval(42, 8);                   // 42
echo intval('42', 8);                 // 34

However, that still leaves these three places where intval is used as a callback, which presumably would still be faster if replaced with a foreach and typecast or such:

core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:      $ids = array_map('intval', $ids);
core/modules/language/src/LanguageNegotiator.php:    $enabled_methods = array_map('intval', $enabled_methods);
core/modules/views/src/Plugin/views/HandlerBase.php:      $value = array_map('intval', $value);

There are similarly any number of places where strval() is used as a callback:

[ayrton:drupal | Wed 04:53:21] $ grep -r "strval" * | grep -v "vendor" | grep -v "node_modules"
core/lib/Drupal/Core/Form/FormState.php:          if (array_slice(explode('][', $name), 0, count($section)) === array_map('strval', $section)) {
core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php:    return array_map('strval', array_keys($this->entityTypeBundleInfo->getBundleInfo($entity_type_id)));
core/modules/views_ui/admin.inc:  if ($process_input && !$form_state->getTriggeringElement() && !empty($element['#is_button']) && isset($user_input[$element['#name']]) && isset($element['#values']) && in_array($user_input[$element['#name']], array_map('strval', $element['#values']), TRUE)) {
core/modules/toolbar/src/Ajax/SetSubtreesCommand.php:      'subtrees' => array_map('strval', $this->subtrees),
core/modules/migrate/src/Plugin/migrate/id_map/Sql.php:    return hash('sha256', serialize(array_map('strval', $source_id_values)));
core/modules/ckeditor5/src/HTMLRestrictions.php:            $value = array_map('strval', array_keys($value));
core/modules/views/src/Tests/ViewResultAssertionTrait.php:          $row[$expected_column] = is_array($field_value) ? array_map('strval', $field_value) : (string) $field_value;

Since those callbacks are apparently not identified by the PHPCS rule, and since reviewing changes to rewrite an array_mao() is more complex than simply scanning the 1:1 replacements here, I think those usages should be scoped to a followup issue.

So NW for two followup issues: One for the octal, and one for the use of intval() and strval() as callbacks.

I also left one small point of feedback on the MR, but this mostly looks good.

Thanks!

Unnecessary parentheses removed. Please, review.

Changes look good

Moving to NW for the 2 followups requested by @xjm in #28

Thanks.

Follow-up issues created: #3337254: Replace Updater.php intval() functions with their relevant type casting operators and #3337257: Replace the use of intval() and strval() as callbacks with a foreach and typecast or such.

I guided myself with this doc, as it's the first time I've done it. I hope that's correct.

I believe the next step is to update the issue summary? Tagging for this.

Added a number of code review comments.

Hi, @longwave! Thanks for the code review.

I removed the unnecessary casts and followed your suggestions rewriting some lines in a cleaner way.

Just not sure about this change in core/lib/Drupal/Core/Updater/Updater.php:

$old_perms = fileperms($parent_dir) & 0777;
...
$filetransfer->chmod($old_perms);

Should we keep with '0755' as the 2nd argument?
$filetransfer->chmod($old_perms, 0755);

Following back up on this I believe the change should be fine? Will send it up to see if the committers have any thoughts too.

A couple of clarifications and nitpicks in the MR but otherwise this looks almost ready to go.

Applied the @longwave's suggestions, please review.

Still seem some open threads

Which one? Sorry, I can't see anything :-(

I think I was wrong here - previously this also protected against uid 0 being cancelled. We could still write something like

if ($uid == 1) {
$root = $account;
}
if ($uid <= 1) {
continue;
}

That code is already in the MR here.

There are two child issues which are fixing special cases of *val. They need to be committed before this one because this is enabling the rule.

I am postponing this.