Problem/Motivation

\Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() can produce a regex that fails on certain PHP builds.

Steps to reproduce

Set pcre.jit to 1 and run \Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationFiles()

We've tried testing this on drupalci - see #2 - but it does not fail in the way it does locally. https://3v4l.org/m091V also shows that its PHP builds are not affected - interesting PHP 8 alpha 2 had a bug here...

Proposed resolution

Fix the regex returned by \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() to work regardless of compiled pcre version.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#16 3181644-16.patch4.36 KBalexpott
#16 15-16-interdiff.txt801 bytesalexpott
#15 3181644-15.patch4.35 KBalexpott
#15 8-15-interdiff.txt4.35 KBalexpott
#8 3181644-8.patch855 bytesalexpott
#2 3181644-2.patch2.07 KBalexpott
#2 3181644-2-will-fail.patch1.23 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3181644-2-will-fail.patch1.23 KB
3181644-2.patch2.07 KB

Here's a will-fail patch to show that the regex in \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() is broken by pcre.jit. And the other patch attached fixes this.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Which exact version of PHP you are using? I faced that PHP did changes in 7.4.12 by committing unreleased lib-pcre jit changes

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
CreditAttribution: chr.fritsch at Thunder commented

I found this issue together with @alexpott and I ran into it with PHP 7.4.9. I didn't tested any other version.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

I bet it caused build option --without-pcre-jit or --with-pcre-jit (so probably could be caught)

@chr.fritsch please check which pcre version used? I'm waiting for 10.36 to try because right now (10.35 pcre) php can catch this bug with own tests https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/14061#note...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented

I'm on 7.4.10 - I guess DrupalCI is built with -without-pcre-jit so setting the flag is meaningless.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented

Well we're not building it with --with-pcre-jit so I guess that's why we can't make the test fail.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Title: PHP setting pcre.jit to 1 makes tests fail » PHP setting pcre.jit to 1 makes \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() regex misbehave
Issue summary: View changes
Priority: Major » Normal
Issue tags: +Needs manual testing
FileSize
3181644-8.patch855 bytes

Hmmm this seems to be caused by other system differences. I think DrupalCI has pcre.jit set to 1... it's the default and you have to do work to make PHP compile without it.

I think the thing to do is to fix \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() here and be done. We have test coverage in \Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationFiles() so we know the regex change is not breaking anything.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

The 2-fail patch fails for me using --with-external-pcre option and following library

$ php --ri pcre

pcre

PCRE (Perl Compatible Regular Expressions) Support => enabled
PCRE Library Version => 10.35 2020-05-09
PCRE Unicode Version => 13.0.0
PCRE JIT Support => enabled
PCRE JIT Target => x86 64bit (little endian + unaligned)

Directive => Local Value => Master Value
pcre.backtrack_limit => 1000000 => 1000000
pcre.recursion_limit => 100000 => 100000
pcre.jit => 1 => 1
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: -Needs manual testing

I tested it manually on the setup from #10 using patch #8 and 3181644-2-will-fail.patch

before 

There was 1 failure:

1) Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationFiles
2 installer languages found.
Failed asserting that 0 is identical to 2.

/var/www/html/web/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:94
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/Assert.php:2344
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/Assert.php:1330
/var/www/html/web/core/tests/Drupal/KernelTests/Core/Installer/InstallerLanguageTest.php:34
/var/www/html/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:730

FAILURES!
Tests: 2, Assertions: 3, Failures: 1.

after

Testing Drupal\KernelTests\Core\Installer\InstallerLanguageTest
Test 'Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationFiles' started
Test 'Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationFiles' ended
Test 'Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationCache' started
Test 'Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationCache' ended


Time: 00:01.593, Memory: 4.00 MB

OK (2 tests, 10 assertions)
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Drupal CI using

pcre

PCRE (Perl Compatible Regular Expressions) Support => enabled
PCRE Library Version => 10.34 2019-11-21
PCRE Unicode Version => 12.1.0
PCRE JIT Support => enabled
PCRE JIT Target => x86 64bit (little endian + unaligned)

Directive => Local Value => Master Value
pcre.backtrack_limit => 1000000 => 1000000
pcre.recursion_limit => 100000 => 100000
pcre.jit => 1 => 1
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Used to build pcre 10.36-RC1 and it fixed the test (test pass without patch #8), so the problem caused by pcre version

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Needs review » Needs work

One question is worth asking: is changing the regex from drupal-[0-9a-z\.-]+\.po$ to drupal-[0-9a-z\.-]+\.[^\.]+\.po$ ok?

So the regex for validating langcodes is ^[a-zA-Z]{1,8}(-[a-zA-Z0-9]{1,8})*$ so new regex won't work for langcodes with capital letters. See \Drupal\language\Form\LanguageFormBase::validateCommon() and https://www.w3.org/International/articles/language-tags/

We need to:

  • Fix regex so that this works
  • And test coverage of this
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
8-15-interdiff.txt4.35 KB
3181644-15.patch4.35 KB

This changes the regex to match version strings better (see _install_get_version_info()) and langcodes - see \Drupal\language\Form\LanguageFormBase::validateCommon()

It also doesn't have problems with PCRE 10.35.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented
FileSize
15-16-interdiff.txt801 bytes
3181644-16.patch4.36 KB

Let's make the test .po file not so underlined in red in PHPStorm...

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Related issues: +#3158071: Redirect loop during installation with non-English
APolitsin’s picture

APolitsin CreditAttribution: APolitsin commented
Priority: Normal » Critical

I think if users can not install Drupal on default PHP installation - it's a critical problem

#3158071

Confirm Loop Drupal 9.1.0, Ru lang
Ubuntu 20.04, php-fpm 7.4
(upd) PCRE Library Version 10.35 2020-05-09

* patch helps me
* drush install also helps

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Can we close #3158071: Redirect loop during installation with non-English as a duplicate of this?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott as a volunteer and at Acro Commerce, Thunder commented

@cilefen I postponed that one on this just in-case there are other causes. I'm pretty certain it is a duplicate but it's hard to be 100% sure with the installer.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Locally the original test fails for me with PCRE 10.35:

$ php -i|grep "PCRE Library"
PCRE Library Version => 10.35 2020-05-09

$ vendor/bin/phpunit -c core ./core/tests/Drupal/KernelTests/Core/Installer/InstallerLanguageTest.php
PHPUnit 8.5.13 by Sebastian Bergmann and contributors.

Testing Drupal\KernelTests\Core\Installer\InstallerLanguageTest
F.                                                                  2 / 2 (100%)

Time: 689 ms, Memory: 6.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\Installer\InstallerLanguageTest::testInstallerTranslationFiles
2 installer languages found.
Failed asserting that 0 is identical to 2.

After applying the patch the test passes. The change looks good to me so this is RTBC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: PHP setting pcre.jit to 1 makes \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() regex misbehave » PCRE library version 10.35 makes \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() regex misbehave

Retitling, as I don't think this is JIT related. I can only reproduce on PCRE 10.35. The original test passes for me on PCRE 10.32 and @andypost's earlier comments indicate that 10.34 and 10.36 are OK too.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: PCRE library version 10.35 makes \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() regex misbehave » PCRE library version 10.35 with pcre.jit=1 makes \Drupal\Core\StringTranslation\Translator\FileTranslation::getTranslationFilesPattern() regex misbehave

Confirmed that this still needs pcre.jit=1, with JIT disabled on 10.35 the original test passes.

  • catch committed 7f828c9 on 9.2.x 
    Issue #3181644 by alexpott, andypost, longwave, chr.fritsch: PCRE...

  • catch committed 2be68f9 on 9.1.x 
    Issue #3181644 by alexpott, andypost, longwave, chr.fritsch: PCRE...

  • catch committed 234d024 on 8.9.x 
    Issue #3181644 by alexpott, andypost, longwave, chr.fritsch: PCRE...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.2.x, cherry-picked to 9.1.x and 8.9.x, thanks!

Status: Fixed » Closed (fixed)

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

quietone credited AppLEaDaY.

quietone credited bbu23.

quietone credited lukass.

quietone credited sargath.

quietone credited thijsv.

quietone credited tstoeckler.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I have closed #3158071: Redirect loop during installation with non-English as a duplicate and am moving credit.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented