Need to catch Peast exceptions

Whoops, meant to create this for 11.x, but no matter, they seem to be identical.

Pushed up a simple proof of concept which catches the exception, creates an informative log entry if it can, and proceeds to use the original source to not cause a complete crash.

Other than it also needing a test, Is this a good approach?

THANK YOU for this. I was running down a big rabbit hole when JS aggregation was on trying to find out what was causing my errors and this saved me.

Good idea - can you open an MR for the branch?

To test this - do we need js with a syntax exception that Peast will definitely throw an error for?

I opened 📌 Update Peast to 1.15.4 Fixed to update Peast too.

This seems to also resolve the case where you mistakenly don't mark a library as minified: https://www.drupal.org/project/drupal/issues/3371010 🐛 Aggregation changes in 10.1 break xdebug Postponed: needs info

Rebased and Fixed issues with tests failing. Needs Review

Couple of comments on the MR but overall looking good.

Went with the default system logger as there isn't any other instances of asset logging to really warrant it's own channel.

Looks great. Will mean that actually broken JavaScript won't cause any more problems than it would have done in 10.x, and if Peast has its own parsing errors on unusual/new syntax that we still get a perfectly usable aggregate, just without that particular file minimized.

Lol, was just about to check in on this and it's already RTBC.

Do we need an actual test to ensure broken scripts keep making it through to the aggregated files (or at least go through this part unmodified)?

Good point yes we do.

Here's a test only patch that should fail.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased/retargeted onto 11.x

Looks good!

Here's the interdiff of the test coverage I added:

diff --git a/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
index 15f21a22cf0062f0e16a860257e069ab8c6b0e10..3c4623ab228fdb1c3df9573a041dce6b8a046296 100644
--- a/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
+++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
@@ -119,6 +119,16 @@ public function providerTestOptimize() {
         ],
         file_get_contents($path . 'to_be_minified.js.optimized.js'),
       ],
+      4 => [
+        [
+          'type' => 'file',
+          'preprocess' => TRUE,
+          'data' => $path . 'syntax_error.js',
+        ],
+        // When there is a syntax error, the 'optimized' contents are the
+        // contents of the original file.
+        file_get_contents($path . 'syntax_error.js'),
+      ],
     ];
   }
 
diff --git a/core/tests/Drupal/Tests/Core/Asset/js_test_files/syntax_error.js b/core/tests/Drupal/Tests/Core/Asset/js_test_files/syntax_error.js
new file mode 100644
index 0000000000000000000000000000000000000000..3794c41c8451c98cfcbbaba6b6839263f3aeebbc
--- /dev/null
+++ b/core/tests/Drupal/Tests/Core/Asset/js_test_files/syntax_error.js
@@ -0,0 +1,13 @@
+/**
+ * Some comments.
+ */
+
+(function foo() {
+  // Missing closing parenthesis.
+  if (true {
+    'print 1';
+  }
+  else {
+    'print 2';
+  }
+})

Given that was the only code I touched here, I'm going to go ahead and commit it. Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

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