WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 2 weeks ago

Last modified 2 weeks ago

#41562 closed task (blessed) (fixed)

PHP 7.2: create_function deprecation in \Gettext_Translations class

Reported by: ayeshrajans Owned by: pento
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: I18N Keywords: needs-patch
Focuses: Cc:

Description

Please see #37082 for the complement issue, and #40109 for the parent issue.

Please see Sara's (who is one of the leading contributors to PHP 7.2) ticket #37082 for information. Her patches fixed majority of the erros for create_function().

I tried a new approach, similar to how Closure class works in modern PHP versions.

\Gettext_Translations::make_plural_form_function function returns a callable, which can also be in the form of [$object, 'method']. Please review this new approach with the \TranslationMultipleFormCallback class. It still calls eval() because there is no other way to evaluate nplural formulas without a comprehensive parser.

create_function() is no better than eval() because it is merely a thin wrapper around eval().

With the patch in #41457 applied, there is only 1 PHP error in Travis CI PHP 7.2 builds! : https://travis-ci.org/Ayesh/wordpress-develop/jobs/261036177#L1257

Attachments (1)

41562.patch (1.9 KB) - added by ayeshrajans 3 months ago.

Download all attachments as: .zip

Change History (11)

@ayeshrajans
3 months ago

#1 @ayeshrajans
3 months ago

  • Component changed from General to I18N
  • Keywords has-patch added

#2 @johnbillion
3 months ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to rmccue
  • Status changed from new to assigned
  • Type changed from defect (bug) to task (blessed)

Thanks for the patch, @ayeshrajans.

As you noted, eval() is no better than create_function(), although it does get rid of the deprecated notice that's shown in PHP 7.2.

@rmccue has been working on a comprehensive solution for this for a while, which avoids using create_function() or eval(). I'll ask him to upload his patch here along with all the information, research, and testing that he's done.

#3 @ayeshrajans
3 months ago

Thanks @johnbillion.
I tried to find a way to evaluate the n-plural formulas too (no eval()), without much success. I have, however, worked on a little parser to evaluate them. I'll try to see if I can put together.

Last edited 3 months ago by ayeshrajans (previous) (diff)

#5 @pento
2 weeks ago

  • Owner changed from rmccue to pento

#6 @pento
2 weeks ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41722:

I18N: Introduce the Plural_Forms class.

Historically, we've evaluated the plural forms for each language using create_function(). This is being deprecated in PHP 7.2, so needs to be replaced.

The Plural_Forms class parses the Plural-Forms header from the PO file, and internally caches the result of all subsequent plural form tests, allowing it to match the performance of the existing code.

Props rmccue.
Fixes #41562.

#7 @johnbillion
2 weeks ago

  • Keywords needs-patch added; has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

This breaks the internet on PHP 5.2 and 5.3. https://travis-ci.org/WordPress/wordpress-develop/builds/283011249

#8 @pento
2 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 41723:

I18N: Fix a PHP error introduced in [41722].

PHP 5.2 and 5.3 don't support short array syntax, Ryan.

Fixes #41562.

#9 @pento
2 weeks ago

In 41725:

Tests: Some tests in [41722] were using newer PHPUnit features.

test_cache used PHPUnit's object mocking to test some internal behaviour in Plural_Forms, but made use of the willReturn() method, which was introduced in PHPUnit 4.0 as shorthand for will($this->returnValue()). Fixed by switching to the longer form.

Several tests used the @expectedException directive to catch generic Exception exceptions, which was added in PHPUnit 3.7. Fixed by changing to an explicit try / catch test.

See #41562.

#10 @johnbillion
2 weeks ago

In 41730:

I18N: Improvements to the tests for plural forms.

  • Move the create_function() code into a file that's only loaded, and into a test that's only run, on PHP <= 7.2 to avoid deprecated warnings in 7.2+.
  • Convert the test skipping into a failure if the GlotPress locale file cannot be downloaded.
  • Ensure test_exceptions fails if an exception is not thrown.
  • Docs improvements

See #41562, #40109

Note: See TracTickets for help on using tickets.