Call PHP native functions with a preceding backslash

Adding performance tag.

There must be a phpcs rule for this somewhere.

I think SlevomatCodingStandard.Namespaces.FullyQualifiedGlobalFunctions will enforce this.

This setting seems like an ideal starting point for performance purposes:

includeSpecialFunctions: include complete list of PHP internal functions that could be optimized when referenced via FQN.

Trying to improve the issue title.

I don't have a strong preference between everything at once vs. optimized first.

Can we treat this and handle this as a coding-standards job, making it a component of 🌱 [meta] Fix PHP coding standards in core Active ?

Created a patch for the issue mentioned, please review.
Thank You.

@Ranjit1032002 this is about prefixing builtins with \, not specifically isset or array_key_exists.

For example is_array -> \is_array(), str_contains -> \str_contains

This issue is likely more of an overarching issue, per function/contsruct, as I'm sure diffs will eventually get to many megabytes.

Setting back to 'Active'.

The patch in #8 was also changing instances of array_key_exists() to isset() which is very much incorrect (and certain to be introducing bugs), as those two things are not equivalent. The correct change for array_key_exists() is simply to change it to \array_key_exists()

At a glance I also see some changes in that patch which have already been committed for the predecessor issues 🐛 drupal_static() edge case bug and inconsistent comments Fixed and 📌 Eliminate anti-pattern isset($arr[$key]) || array_key_exists($key, $arr) Fixed , which will be one reason why it didn't apply.

Per #9 there is a more general issue here. The specific case of array_key_exists() might be worth patching in the short term, as there is a known performance enhancement to be had from doing that; however the performance-critical cases originally established for 📌 array_key_exists() micro-optimization Fixed have now been addressed in 📌 Eliminate anti-pattern isset($arr[$key]) || array_key_exists($key, $arr) Fixed , and without profiling I couldn't guess whether there would be a noticeable benefit from updating the remaining cases.

@Ranjit1032002, for clarity about why you cannot do what you were doing in that patch:

$a = ['foo' => NULL]
array_key_exists('foo', $a) => true
isset($a['foo']) => false

Since the list of functions that have opcode implementations is subject to change, we should do this for all PHP native functions.

For the record, Symfony does it selectively. Only some specific functions are prefixed. That makes code style inconsistent.
For the purpose of consistency, I propose we apply the rule unconditionally. Namespaced code, procedural code, native PHP functions, custom functions, whatever. Everything should be fully qualified. This needs to be applied to constants as well.

I enabled the sniff and ran phpcbf. There was one error

FILE: /builds/project/drupal/core/modules/views/src/ViewsConfigUpdater.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 258 | ERROR | The trigger_error message '\ sprintf ( The update to convert
     |       | "numeric" arguments to "entity_target_id" for entity reference
     |       | fields for view "%s" is deprecated in drupal:10.3.0 and is
     |       | removed from drupal:12.0.0. Profile, module and theme provided
     |       | configuration should be updated. See
     |       | https://www.drupal.org/node/3441945 $view -> id ( ) )' does not
     |       | match the relaxed standard format: %thing% is deprecated in
     |       | %deprecation-version% any free text %removal-version%.
     |       | %extra-info%. See %cr-link%
     |       | (Drupal.Semantics.FunctionTriggerError.TriggerErrorTextLayoutRelaxed)

For now, let's ignore that sniff on this line.

Everything but what is explained below are fully qualified and tests are passing. 3,808 files are changed.

To get tests to pass a 'phpcs:ignore ' line was added to the following

• \Drupal\Core\Theme\Registry::getPrefixGroupedUserFunctions
• \Drupal\Component\Datetime\Time::getCurrentTime
• \Drupal\Component\Datetime\Time::getCurrentMicroTime
• \Drupal\views\ViewsConfigUpdater::processEntityArgumentUpdate

and these functions are ignored by the sniff
base_path,history_read,views_add_contextual_links

I was curious about this topic in general after seeing the pattern appear in some code I was reviewing. A search turned up this article, which has a very different opinion on the matter:

https://medium.com/@steven_3682/the-definitive-case-against-a-backslash-before-a-php-native-function-9f1f51c4415

Has anyone done benchmark tests to confirm that a change this big in Drupal is worth the predicted performance gains?

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.