Require backslash prefix for global functions and constants

Created on 20 January 2022, over 3 years ago
Updated 12 March 2024, about 1 year ago

Problem/Motivation

This is something that I first learned thanks to the EA Extended / EA Ultimate plugin for PhpStorm.
When calling/referencing global-namespace functions or constants from within a namespace, there is some ambiguity:

namespace N;

use function strpos as strpos_alias;
use const T_STRING as T_STRING_ALIAS;

strpos(..);  // Calls \N\foo() if exists, or \foo() otherwise.
\strpos(..);  // Calls \foo();
strpos_alias(..);  // Calls \foo();

print T_STRING;  // Prints constant \N\T_STRING if exists, or \T_STRING otherwise.
print \T_STRING;  // Prints constant \T_STRING.
print T_STRING_ALIAS;  // Prints constant \T_STRING.

The non-qualified reference does have a minor performance impact, which can become relevant for repeated low-level operations.
See https://github.com/Roave/FunctionFQNReplacer for benchmarks and further reading.

In Drupal, I mostly see function calls without the clarifying leading backslash.
Outside of Drupal, I see a mix of "use function" and FQN (leading backslash) for functions, and mostly imports for constants.

I don't see a recommendation about this in our coding standards.
See https://www.drupal.org/docs/develop/standards/coding-standards#functcall β†’

I personally like backslash for functions and constants when writing new code - this way I don't have to keep the imports up to date, and I can easily distinguish top-namespace functions from imported namespaced functions. Plus I don't need to create aliases for namespaced constants or functions used in the same file.
For existing code, it might be less disruptive to add the imports. Although I think the auto merge resolution in PhpStorm handles the added backslash more smoothly than added imports.

Proposed resolution

Decide whether we should
- recommend or prescribe backslash.
- recommend or prescribe imports.
- recommend or prescribe to use the ambiguous reference without backslash or import.
- leave it up to the developer.

Then decide whether this should be updated in core, or just be applied to new code.

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Active

Component

Coding Standards

Created by

πŸ‡©πŸ‡ͺGermany donquixote

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Went searching for something like this. I think this could be better titlted: "Require backslash prefix for global functions and constants" so renamed.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Article on the performance improvement of using FQNS'd global functions https://veewee.github.io/blog/optimizing-php-performance-by-fq-function-...

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    I think it would be easier if this was only required for new code.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Agreed we should do this. There are also compiler-level optimisations for \array_key_exists() that only work when it's explicitly namespaced.

    I think we could 'require' it for all code, but not enable the rule until core is actually compliant?

  • πŸ‡©πŸ‡ͺGermany donquixote

    Is there any argument for "use" vs in-place backslash?
    Personally I think I prefer in-place backslash, but "use" seems more common in composer packages.

    Clearly in-place backslash is more git-friendly, that is, it reduces merge conflicts from changes in unrelated places.

    (In my day to day work I currently don't use either of the two methods, because I would have to convince my colleagues first.)

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    phpcbf can fix this so we can turn the rule on and make the decision at the same time

  • πŸ‡¬πŸ‡§United Kingdom catch

    I think we should only use use for functions where they're namespaced functions, which we currently don't have any/many of but could in the future.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    #8 yeah that makes sense to me too.

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    I think this is just SlevomatCodingStandard.Namespaces.FullyQualifiedGlobalFunctions?

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia
  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia

    Added the template to the issue summary.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Added myself and @kim.pepper as two supporters

  • πŸ‡¦πŸ‡ΊAustralia kim.pepper πŸ„β€β™‚οΈπŸ‡¦πŸ‡ΊSydney, Australia
  • πŸ‡¬πŸ‡§United Kingdom catch

    Added myself as a supporter.

  • πŸ‡¦πŸ‡ΊAustralia dpi Perth, Australia

    + support

    Discussion for core: 🌱 Call PHP native functions fully qualified (like \array_key_exists()) Active

    External discussion in derivative standard: https://github.com/previousnext/coding-standard/pull/69

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Added proposed text

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Current practice is to use 'use' statements for constants. So, I think we can reduce scope here to global functions.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    This was discussed at the last coding standards meeting. There was some performance testing done, the results are in #3458726: Coding Standards Meeting Tuesday 2024-07-16 2100 UTC β†’ .

  • πŸ‡ΊπŸ‡ΈUnited States daletrexel Minnesota, USA

    (As noted over in [#])

    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 other than the ones in the original article to confirm that a change this big in Drupal is worth the predicted performance gains? It looks like the ones run in #3458726: Coding Standards Meeting Tuesday 2024-07-16 2100 UTC β†’ were only the ones from the original article, and even then found to be pretty insignificant.

  • πŸ‡©πŸ‡ͺGermany donquixote

    I would not put too much emphasis on the performance argument.
    The medium article from #20 looks interesting, but I have not looked into it enough to assess it.
    Certainly it's good to understand what we are optimizing for.

    Currently we live in a world where most functions are in the global namespace.
    This applies to native php functions and to functions from Drupal modules.
    Many packages don't want to add functions, they only want to add classes.
    (there have been interesting discussions in the php internals list about static methods vs functions)

    If more packages and modules embrace and provide namespaced functions, the potential for ambiguity becomes bigger.

    One thing here is technical ambiguity, where actually the wrong function is called.
    The other thing is confusion when reading code, because you don't immediately see if a function is in the global namespace or not.

    I was long convinced that backslash is the way to go, but maybe we should also consider imports, and check how projects outside of Drupal do it.

  • πŸ‡§πŸ‡ͺBelgium BramDriesen Belgium πŸ‡§πŸ‡ͺ

    This has been a discussion point in the MR's of our team for our projects. Not so much about what is in the IS but more about the global \Drupal:: calls in none name-spaced files (e.g. .module).

    I would usually write my code with the leading backslash.

    $facet_manager = \Drupal::service('facets.manager');
    

    But then a team member would come in saying the leading backslash is not required as we're not in a namespace. Looking at Drupal core, I can see a mix of both being used, so I think it's good if we could standardise this as a standard.

    Core example where it is done with a \ : https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/autom...

    Core example where it is done withouth a \ : https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/toolb...

    Happy to say thought that there are not that many cases without the \

    PS: I find coding standards difficult English to read πŸ™ƒ so if this is not the same thing we're discussing let me know and I'll create a new issue.

  • πŸ‡§πŸ‡ͺBelgium borisson_ Mechelen, πŸ‡§πŸ‡ͺ

    #22, this is about calls to functions specifically, because of the performance part. Yours is different enough to warrant it's own issue. Especially since I think it is a lot less controversial.

  • πŸ‡§πŸ‡ͺBelgium BramDriesen Belgium πŸ‡§πŸ‡ͺ
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Update link

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    One item I don't see mentioned is mockability for Unit Testing.

    Inside a namespaced class a Global functions preceded by a backslash can not be mocked while global functions called without a preceding backslash can be.

    How widely this trick is used I am not sure, and ideally we are moving away from global functions, however eventually they do get called (especially when you start working with more 'low level' code), If you are going to simulate every code flow execution sometimes you just have to mock the response.

    IIRC I've tried this once and it worked decently to abstract away having to include 3rd party code into the test case.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Another issue that needs a change record.

Production build 0.71.5 2024