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

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...

Added a CS rule forbid said casting functions, but emit a warning and not an error. I don't know PHPCS enough to see if it can add a warning message instead of a function suggestion. The two instance of intval calls are explicitly allowed with a PHPCS toggle.

The reason why I thought to use a warning instead of an error is because none of these functions are deprecated, and especially in intval case, there is a parameter for base conversion. If you think we are better off with an error, I'll happily change it too.

Thank you.

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: 📌 Replace Updater.php intval() functions with their relevant type casting operators Fixed and 📌 Replace the use of intval() and strval() as callbacks with a foreach and typecast or such Postponed: needs info .

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.