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

CommentFileSizeAuthor
#21 3236769-2.patch695 bytesandypost
#16 3236769-16.patch803 bytesandypost
#15 3236769-15.patch706 bytesandypost
#13 3236769-13.patch806 bytesandypost
#11 3236769-11.patch744 bytesandypost
#2 3236769-2.patch697 bytesalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Critical
Status: Active » Needs review
StatusFileSize
new 3236769-2.patch697 bytes

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

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Straight forward

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Also looks like translation can be a string, not just an array - if its empty should we just leave it uninitialized?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Hmmm 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 

array(1) {
  [0]=>
  string(0) ""
}

.. see https://3v4l.org/N7Ems

daffie’s picture

daffie commented
Status: Needs review » Needs work

The change looks good to me, only should we do the same with the previous line?

+++ b/core/lib/Drupal/Component/Gettext/PoItem.php
@@ -197,7 +197,7 @@ public function setFromArray(array $values = []) {
       $this->setSource(explode(self::DELIMITER, $this->source));
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@daffie nope - $this->source can not be NULL at this point due to

    if (isset($this->source) &&
        strpos($this->source, self::DELIMITER) !== FALSE) {
daffie’s picture

daffie commented
Status: Needs work » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs review

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

andypost’s picture

andypost
he/him
Primary language Russian
commented

This is a case when source string has delimiter which means that $this->translation is array of plural elements, so explode() applies on array

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 3236769-11.patch744 bytes

I think it should be like it, maybe to add assert to catch when NULL passed

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@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
['']

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 3236769-13.patch806 bytes

Yes, 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

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/lib/Drupal/Component/Gettext/PoItem.php
@@ -197,7 +197,10 @@ public function setFromArray(array $values = []) {
+      if (!is_null($this->translation)) {

probably isset() should be faster

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 3236769-15.patch706 bytes

better optimization

andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 3236769-16.patch803 bytes

Sorry #15 is interdiff

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

The last submitted patch, 11: 3236769-11.patch, failed testing. View results

The last submitted patch, 13: 3236769-13.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 16: 3236769-16.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 3236769-2.patch695 bytes

Thinking 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)

andypost’s picture

andypost
he/him
Primary language Russian
commented
Related issues: +#1861442: [meta] Review of Drupal\Component\GetText files

  • catch committed f824705 on 9.3.x 
    Issue #3236769 by andypost, alexpott, larowlan: PoItem causes...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed f824705 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.