Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
\Drupal\Component\Gettext\PoItem::$translation
is initialised to NULL and then used in \Drupal\Component\Gettext\PoItem::setFromArray()
with explode()
. There are code paths that never set the variable before passing it to explode. This will trigger a PHP deprecation error in 8.1.
Steps to reproduce
See test runs on #3220021: [meta] Ensure compatibility of Drupal 9 with PHP 8.1 (as it evolves)
Proposed resolution
TBD
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#21 | 3236769-2.patch | 695 bytes | andypost |
#2 | 3236769-2.patch | 697 bytes | alexpott |
Comments
Comment #2
alexpottThis fixes the deprecation notices... not sure it is the best fix. Dunno if we should initialise the variable to an empty string. The whole way PoItem works with setFromArray is not that great and liable to bugs. We should be using an immutable value object here but this is very old code.
Comment #3
larowlanStraight forward
Comment #4
larowlanAlso looks like translation can be a string, not just an array - if its empty should we just leave it uninitialized?
Comment #5
alexpottHmmm leaving it uninitialised might be a good option here. And then at some point in the future we can refactor this whole thing to be more strongly typed and less mutable. One effect of this would be that the value wouldn't be the same as it ends up now which is
.. see https://3v4l.org/N7Ems
Comment #6
daffie CreditAttribution: daffie commentedThe change looks good to me, only should we do the same with the previous line?
Comment #7
alexpott@daffie nope - $this->source can not be NULL at this point due to
Comment #8
daffie CreditAttribution: daffie commentedLooks good to me.
Comment #9
alexpott@daffie my concern with this fix is that we're still very inconsistent. \Drupal\Component\Gettext\PoItem::getTranslation() can return a NULL if setFromArray() is called with a source key that's a string and doesn't have the plural delimiter. And that's passed into a string function in \Drupal\Component\Gettext\PoStreamReader::readHeader(). All this code is very old and not very well typed.
The fix here is a minimal fix but I think we should take a bit longer to consider other approaches.
Comment #10
andypostThis is a case when source string has delimiter which means that
$this->translation
is array of plural elements, soexplode()
applies on arrayComment #11
andypostI think it should be like it, maybe to add assert to catch when NULL passed
Comment #12
alexpott@andypost that's a change in current behaviour. If you called getTranslation() after called setFromArray without 'translation' you'll now get NULL whereas before you'll get
['']
Comment #13
andypostYes, better patch
Basically we could set default value to translation to empty string but better to prevent explode when translation is not passed
Debugged it when exporting translation form
Comment #14
andypostprobably isset() should be faster
Comment #15
andypostbetter optimization
Comment #16
andypostSorry #15 is interdiff
Comment #17
alexpott@andypost #16 is still the same behaviour change as #11. The more I think about this the more I think we should do #2 and then open an issue to modernise the whole GetText component.
Comment #21
andypostThinking a bit more I'm ++ to RTBC patch in #2 - there's already set of follow-ups #1861442-20: [meta] Review of Drupal\Component\GetText files
Re-upload patch #2 (should be last patch in issue)
Comment #22
andypostComment #24
catchCommitted f824705 and pushed to 9.3.x. Thanks!