Contrib.social

Fatal error when modules are declaring global string constants using t() function

Open on Drupal.org →
Created on 4 February 2016, over 9 years ago
Updated 10 February 2025, 5 months ago

Although a bad pattern, I recently saw this in a pre-release for a contrib module.

If a module is defining a global constant like this (for example on the top of its module file):

define('MY_GLOBAL', t('Test this'));

...and the site in question is installed in something other than english, then locale will try to save this string for translation, and try to save some metadata about it, among them, the request URI where the string was first found. Long story short, it ends up here (core/modules/locale/src/LocaleLookup.php line 152:

     $this->stringStorage->createString(array(
        'source' => $offset,
        'context' => $this->context,
        'version' => \Drupal::VERSION,
      ))->addLocation('path', $this->requestStack->getCurrentRequest()->getRequestUri())->save();

Now, looking at the documentation for getCurrentRequest, it can return "Request|null". In this case, it returns null, and so getRequestUri causes "Fatal error: Call to a member function getRequestUri() on null".

Now, one might argue that the fault here is entirely at the module author, because they should not create a global constant like that. Fair enough. But one might also argue that Drupal should maybe not fail so loudly. In my opinion, we could add a check for the return value of getCurrentRequest, and if "null" is returned, we set a default value of "/" (for example). We might also show a warning in watchdog for example, so that module authors can be notified in some way.

Open to suggestions, and will provide the first patch suggestion in next post.

Steps to reproduce:

- Install Drupal in another language than English
- Enable a new module.
- Add a global constant with t() (for example with define('MY_GLOBAL', t('Test this'));) to that module.
- Try to refresh any page on the site.

This does not occur if you try to enable a module with that constant already defined, but it will happen if you import config that enables a module that has this constant defined.

Anyway, discussion started - patch coming up :)

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component

language system

Created by

🇳🇴Norway eiriksm Norway

Live updates comments and jobs are added and updated live.
  • D8MI

    (Drupal 8 Multilingual Initiative) is the tag used by the multilingual initiative to mark core issues (and some contributed module issues). For versions other than Drupal 8, use the i18n (Internationalization) tag on issues which involve or affect multilingual / multinational support. That is preferred over Translation.

  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

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.

  • Comment 5 months ago →
    🇺🇸United States smustgrave

    This came up as a daily bugsmash target.

    When I triaged it in 2022 I probably should of tagged it. But will need a clear issue summary using the standard template.

    Fixes are in MRs now and test coverage will still be needed.

  • Comment 5 months ago →
    🇦🇺Australia acbramley

    I'm not really sure this is something we should support. It's also been 9 years since there was work done here that wasn't a reroll. The fix seems to apply to a much broader scope than defining constants. Generally we would discourage global constants now anyway as they can be placed in various OO contexts (or Enums). I think this should be closed as won't fix.

  • Comment 5 months ago →
    🇳🇴Norway eiriksm Norway

    That being said, it's still possible to trigger in other ways than with a constant. It was just randomly how I discovered it back in the day (can't even remember reporting it hehe time flies)

  • Status changed to Postponed: needs info 2 months ago
  • Comment 2 months ago →
    🇺🇸United States smustgrave

    So in agreement to close?

  • Comment 2 months ago →
    🇳🇿New Zealand quietone

    In light on the comments in #32, which I agree with, this should be closed.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024