JsOptimizer preg_replace gives null as return

Yes, this is an issue. I can confirm it. In my case, I had several minified vendor js files that all had source maps. This fixed it. Still needs tests. So back to NW.

Did some sandboxing over various regexes, and the best way to address this is to do away with any backtracking whatsoever.

Source: https://stackoverflow.com/a/36629670

And sandbox for various regexes: https://3v4l.org/JagL4

Attached a patch and test for the new non-backtracking regex.

Use an appropriately named test-only-fail patch so as not to trigger the "Needs work" tag.

Edit: This was a misconception on my part, Drupal apparently doesn't have a convention for skipping the "Needs work" tag trigger.

The proposed patch breaks the page if the javascript happens to contain:

// ...
(t.evaluate||Ht).source+"|$","g"),p="//# sourceURL="+(bl.call(t,"sourceURL")?(t.sourceURL+"").replace(/\s/g," "):"lodash.templateSources["+ ++Zr+"]")+"\n";
// ...

e.g. https://cdn.jsdelivr.net/npm/lodash@4.17.21/lodash.min.js

The regex needs to only match against the last line rather than attempt to match potential sourceMaps located in the middle of the code.

See #2400287-50: Remove all occurences of sourceMappingURL and sourceURL when JS files are aggregated

I can confirm the issue and the observation in #16 when applying the patch.

Edit: In my case I was able to fix it, by running webpack in production mode, so that sourcemaps have not been created. But that only works as long you have problem in your own scope. The general problem still exists. In my case it was also the lodash library.

I confirm the observation of szeidler, after running webpack in production mode the problem is gone.

straight reroll

RTBC:

has tests, passes tests

tests only fails

Related issues are linked

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