FixtureManipulator should only use Composer commands, rather than manipulating JSON directly

Bumping to critical because this is blocking a green core MR.

We also have a pcre.inifile in our repo, that we use in our own test suite. Let’s remove that in this issue. And let’s do the archeological research to find out why it was originally added! That should help 🤓

The custom pcre.ini was introduced in #3239466: Refactor build tests to use core-supported project templates → due to PCRE_JIT_STACKLIMIT_ERROR as being the cause of random failure.
The comment left in the file ---

# This file is used on Drupal CI because Composer occasionally dies with
# PCRE_JIT_STACKLIMIT_ERROR during certain test cases, and there is no insight
# into where the failure is happening, or why. This has only been observed on
# Drupal CI. Since the PCRE engine's JIT is not strictly necessary to the
# functioning of the Automatic Updates module, this file disables the PCRE JIT
# as a workaround.

Looks like @phenaproxima did that, without providing additional context beyond what's in that comment you quoted. Looks like it's a different error though? I'm curious if this particular issue rings bells for @phenaproxima though :)

Searching the composer/composer repo finds a single match: https://github.com/composer/composer/issues/10037.

But that was a weakness in Composer's ClassMapGenerator. This is different — this is happening just by doing composer config! 🤯

For sure another weakness in the upstream regexp, but we should be able to create a debug build of composer where we add our own additional logging to figure out the root cause — I just paired with @Omkar.Podey to figure out how to modify composer to add debug output:

1. git clone git@github.com:composer/composer.git
2. cd composer
3. composer install
4. MAKE MODIFICATIONS in src
5. composer compile
6. sudo cp composer.phar /opt/homebrew/bin/composer
7. sudo chmod +x /opt/homebrew/bin/composer

That’s assuming you have this:

$ dirname $(which composer)
/opt/homebrew/bin

So the failure is caused when we try to execute Preg::match('#^{ (?P<leadingspace>\s*?) (?P<content>\S+.*?)? (?P<trailingspace>\s*) }$#sx', $children, $match); and the value being passed as $children can be seen as a dump which is a valid json.

string(1224) "DUMP ------>>>>>>> {"composer/plugin-b":{
"type":"path",
"url":"/tmp/base-repo-1678282141.13118550/composer--plugin-b",
"options":{
"symlink":false
}
},
"composer/plugin-a":{
"type":"path",
"url":"/tmp/base-repo-1678282141.13118550/composer--plugin-a",
"options":{
"symlink":false
}
},
"drupal/core-vendor-hardening":{
"type":"path",
"url":"/tmp/base-repo-1678282141.13118550/drupal--core-vendor-hardening",
"options":{
"symlink":false
}
},
"drupal/semver_test":{
"type":"path",
"url":"/tmp/base-repo-1678282141.13118550/drupal--semver_test",
"options":{
"symlink":false
}
},
"packagist.org":false,"drupal\/core-recommended":{"type":"path","version":"9.8.0","url":"\/tmp\/base-repo-1678282141.13118550\/drupal--core-recommended","options":{"symlink":false}},"drupal\/core-dev":{"type":"path","version":"9.8.0","url":"\/tmp\/base-repo-1678282141.13118550\/drupal--core-dev","options":{"symlink":false}},"drupal\/core":{"type":"path","version":"9.8.0","url":"\/tmp\/base-repo-1678282141.13118550\/drupal--core","options":{"symlink":false}},"cweagans\/composer-patches":{"type":"path","version":"24.12.1999","url":"\/tmp\/base-repo-1678282141.13118550\/cweagans--composer-patches","options":{"symlink":false}}} <<<<<<<<--------"

The interesting thing is the part which is slightly different from the rest

1. It has an extra key version.
2. The formatting has no spaces or new lines.

"packagist.org":false,"drupal\/core-recommended":{"type":"path","version":"9.8.0","url":"\/tmp\/base-repo-1678282141.13118550\/drupal--core-recommended","options":{"symlink":false}},"drupal\/core-dev":{"type":"path","version":"9.8.0","url":"\/tmp\/base-repo-1678282141.13118550\/drupal--core-dev","options":{"symlink":false}},"drupal\/core":{"type":"path","version":"9.8.0","url":"\/tmp\/base-repo-1678282141.13118550\/drupal--core","options":{"symlink":false}},"cweagans\/composer-patches":{"type":"path","version":"24.12.1999","url":"\/tmp\/base-repo-1678282141.13118550\/cweagans--composer-patches","options":{"symlink":false}}}

Looks like I'm running into this same problem over at 📌 Remove FixtureManipulator::modifyPackage() last usage Fixed :

Testing Drupal\Tests\package_manager\Kernel\ComposerPluginsValidatorTest
........E......E..E..E                                            22 / 22 (100%)

Time: 03:14.838, Memory: 6.00 MB

There were 4 errors:

1) Drupal\Tests\package_manager\Kernel\ComposerPluginsValidatorTest::testValidationDuringPreCreate with data set "complex combination" (array(true, false, false, true, true), array(array('drupal/semver_test', '8.1.0', 'drupal-module'), array('drupal/core-vendor-hardening', '9.8.0', 'composer-plugin', array('*'), array('AnyClass')), array('composer/plugin-a', '6.1', 'composer-plugin', array('*'), array('AnyClass')), array('composer/plugin-b', '20.1', 'composer-plugin', array('*'), array('AnyClass')), array('not-cweagans/not-composer-patches', array('*'), array('AnyClass'), '6.1', 'composer-plugin'), array('also-not-cweagans/also-not-co...atches', '20.1', 'composer-plugin', array('*'), array('AnyClass'))), array(Drupal\package_manager\ValidationResult Object (...)))
PhpTuf\ComposerStager\Domain\Exception\RuntimeException: The command "'/usr/local/bin/composer' 'config' 'repo.also-not-cweagans\/also-not-composer-patches' '{"type":"path","url":"/tmp/base-repo-1678128972.67055174/also-not-cweagans--also-not-composer-patches","version":"20.1","options":{"symlink":false},"canonical":false}' '--working-dir=/tmp/package_manager_testing_roottest13405578/active'" failed.

Exit Code: 2(Misuse of shell builtins)

Working directory: /var/www/html

Output:
================


Error Output:
================

In PcreException.php line 29:

  preg_match(): failed executing "#^{ (?P\s*?) (?P\S+.
  *?)? (?P\s*) }$#sx": Backtrack limit exhausted


config [-g|--global] [-e|--editor] [-a|--auth] [--unset] [-l|--list] [-f|--file FILE] [--absolute] [-j|--json] [-m|--merge] [--append] [--source] [--] [ [...]]

— https://www.drupal.org/pift-ci-job/2610721 →

What's really fascinating is that:

• HEAD of 3.0.x passes just fine on this contrib module's Drupal CI
• HEAD of 3.0.x converted to a core merge request fails like @tedbow reported on core's Drupal CI
• 👆 This is only possible thanks to the use of pcre.jit = 0, which the failing MR here (which stops doing that on this module's DrupalCI) proves
• With 📌 Remove FixtureManipulator::modifyPackage() last usage Fixed , it fails also on this contrib module's Drupal CI, even with pcre.jit = 0!

How is this possible?! 🤯

#3345633 is not even touching ComposerPluginsValidatorTest, only FixtureManipulator is being modified. So there must be something about the combination of circumstances that causes this hard failure. But what?

Note that it can be reproduced even with this subset — it's literally this one single test case (that's aptly named "complex") that causes composer to crash. I'm testing with composer 2.5.4, @omkar.podey is testing with 2.4.2.

Checking #10.

😬

Now let's disable the pcre.jit=0 work-around on DrupalCI. Now it should fail.

The last submitted patch, 14: 3346628-14-subset.patch, failed testing. View results →

Let's see if the package/name vs package\/name differences in @omkar.podey's output at #8 make a difference… this adopts the consistent use of JSON_UNESCAPED_SLASHES.

According to

<?php

$a = ['drupal/core' => TRUE];
var_dump(json_encode($a));
var_dump(json_encode($a, JSON_UNESCAPED_SLASHES));

var_dump(json_decode(json_encode($a)));
var_dump(json_decode(json_encode($a, JSON_UNESCAPED_SLASHES)));

⇒

string(21) "{"drupal\/core":true}"
string(20) "{"drupal/core":true}"
object(stdClass)#1 (1) {
  ["drupal/core"]=>
  bool(true)
}
object(stdClass)#1 (1) {
  ["drupal/core"]=>
  bool(true)
}

this should work: https://3v4l.org/492Qn

I've narrowed it down to the \Drupal\fixture_manipulator\FixtureManipulator::addRepository() calls, which matches what @omkar.podey reported in #8 (that's JSON for the repositories).

As part of doing this, I learned the painful way just now that composer has a class called \Composer\Json\JsonManipulator that modifies JSON using regular expressions rather than a proper JSON parser 🤯😱😱😱😱😱😱😱

https://github.com/composer/composer/blob/main/src/Composer/Json/JsonMan...

There are quite a few bug reports about composer failing with big composer.json files — and the suggested solutions such as https://github.com/composer/composer/issues/9595#issuecomment-757911140, https://github.com/composer/composer/issues/5104 and https://github.com/composer/composer/issues/6830 are simply not being worked on … 😳

Signing off for today, @phenaproxima will continue this!

Re-roll of #17.

I stared at this helplessly for a while, then decided I might as well update my computer to macOS Ventura.

While that was going on, I paced and pondered what's going on here. I thought about that failing regex, and the fact that Composer manipulates JSON with regexes (which is truly its own special category of insanity).

I read about what regex backtracking is. I paced some more. Then I thought more about Omkar's observation in #8:

The formatting has no spaces or new lines.

That makes me wonder, just a bit...is it possible that Composer is expecting the JSON it's dealing with to be pretty-printed? Some of the opinions in the failing regex -- specifically, the presence of spaces -- makes me think it might not ever be expecting to deal with one big ol' unformatted line of JSON, and beyond a certain length, it hits the backtracking limit.

What, I wonder, would happen if we always ensured everything was pretty-printed?

I doubt this will make a single dent in the problem, but just for funsies, let's try using JSON_PRETTY_PRINT when calling file_put_contents() in FixtureManipulator.

the fact that Composer manipulates JSON with regexes (which is truly its own special category of inexplicable)

As I wrote that, I thought of a possible explanation for this choice: maybe they're trying to support sites that don't have JSON support built into PHP. According to https://www.php.net/manual/en/json.installation.php, the JSON extension only became part of PHP core as of PHP 8.

...no fucking way: https://www.drupal.org/pift-ci-job/2612513 →

My jaw is on the floor.

My work here is done.

The last submitted patch, 10: 3346628-10-subset.patch, failed testing. View results →

Re-titling to clarify the proposed solution implemented in the MR.

😂😂😂😂😂😂😂😂😂
    👇
😭😭😭😭😭😭😭😭😭
    👇
😂😂😂😂😂😂😂😂😂

Nope, you thought of that too! 🤩🥳

JsonFile is safe to use in this context. It's part of Composer, but Composer is a dev dependency of core, so we can rely on this class being available to us in tests.

Even though the JSON_PRETTY_PRINT flag for json_encode() clearly also works (see #23), I do agree this is preferable: it stresses explicitly in code how absurd and brittle Composer's JSON handling is.

I'm marking this RTBC for the shape of the result, but I want to add explicit documentation prior to commit to document this incredibly scary rabbit hole 🤓

Thanks to @phenaproxima, I woke up to a world of semi-sanity. So I just documented that semi-sanity in b1f205ee. But in doing so, I noticed that documenting it felt rather unsatisfactory: FixtureManipulator uses "only" composer commands … except when it doesn't.

That felt confusing to write, a statement with weak clarity.

Wouldn't it be great if we could actually say that FixtureManipulator uses solely composer commands?!

Turns out … we CAN! That means we no longer need to hardcode any knowledge about composer.json structure, only knowledge about how to use Composer commands. IOW: we're constructing and manipulating fixtures solely using composer itself (even the committed fixture: see \Drupal\automatic_updates\Development\ComposerFixtureCreator introduced by 🐛 The 'fake_site' fixture cannot be using with `composer show` because the packages are not installed Fixed ). That means we've now arrived at a point where we truly use the "public API" of Composer — both for making Automatic Updates work and for testing it.

This was a logical next step that would not have come to me if @phenaproxima had not already pushed it this far! 😊🚀

Back to Wim for test failures; I guess there's an errant install_path being passed in somewhere, most likely from within Automatic Updates Extensions. Shouldn't be too hard to locate. But it's early o'clock here and I gotta get my day started properly.

Was actually pretty easy, because it was caused by dead code!

Given that @phenaproxima is starting his day, this is blocking a lot of alpha blockers and his review didn't surface any other concerns, I'll go ahead and merge this as soon as it comes back green!

Oh wow the GitLab "merge" button does not actually respect the commit authors that are listed here on d.o?! 😱😬

You gotta use the merge button at the bottom of the d.o issue, not the one in the GitLab UI.

😳 Why is that not disabled then?!

Reverted and re-committed with proper title and credit.

🙏

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