JsOptimizer preg_replace gives null as return

RTBC:

has tests, passes tests

tests only fails

Related issues are linked

The last submitted patch, 24: 2990907-24.patch, failed testing. View results →

+++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
@@ -75,6 +75,35 @@ public function testClean($js_asset, $expected) {
+  /**
+   * Tests that the javascript may be cleaned without backtracking.
+   */
+  public function testCleanWithRecursionLimit() {
+    // Specify a hard backtracking limit.
+    ini_set('pcre.backtrack_limit', 100);
+    $backtrack_limit = (int) ini_get('pcre.backtrack_limit');
+    $this->assertEquals(100, $backtrack_limit);
+    $script = <<<JS
+(function($) { "use strict"; })
+//# sourceMappingURL=data:application/json;charset=utf-8;base64,
+JS;
+    // Generate a source map URL that would exceed the backtrack limit.
+    $script .= str_repeat('x', $backtrack_limit);
+    // Add an empty space after the sourcemap, followed by other
+    // miscellaneous code.
+    $script .= <<<JS
+ // I appear after the sourcemap URL.
+(function($) { "use strict"; console.log('Hello'); })
+JS;
+    $expected_script = <<<JS
+(function($) { "use strict"; })
+// I appear after the sourcemap URL.
+(function($) { "use strict"; console.log('Hello'); })
+JS;
+
+    $this->assertEquals($expected_script, $this->optimizer->clean($script));
+  }
+

Overall this looks good but the regexp is trying to find both sourceURL and sourceMappingURL so I think we should probably have coverage for both?

It could be an extra method with nearly identical content or maybe one method with a data provider.

I think we'd ideally also have test coverage for the issue raised in #16 when the source code itself contains //# sourceURL= in a string, but should not be stripped.

@codebymikey yes I think that's needed too.

Actually I wonder if this code is needed at all with Peast - if it strips this already, then we don't need the regexp?

I rebased the last patch and added it to a MR.