Problem/Motivation

We need to remove all occurrences of sourceMappingURL and sourceURL including the old //@ and new //# syntax. Otherwise these source map comments are causing 404 errors after the JS files are aggregated. After JS aggregation the source maps can no longer used.

This can be done with a regex in:

  • D7: drupal_build_js_cache().
  • D8: JsCollectionOptimizer::optimize()

Proposed resolution

Remove sourceMappingURL and sourceURL from JS files.

Remaining tasks

Review and commit.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#71 drupal-2400287-quill-with-agg.png15.8 KBcyb_tachyon
#71 drupal-2400287-quill-without-agg.png19.29 KBcyb_tachyon
#62 interdiff-2400287-48-61.txt751 bytesaerozeppelin
#62 2400287-61.patch627 bytesaerozeppelin
#48 2400287-remove--js-source-maps-48.patch701 bytesyuriy.babenko
#36 interdiff.txt2.68 KBrteijeiro
#36 remove_all_occurences-2400287-36.patch9.95 KBrteijeiro
#32 interdiff-2400287-30-32.txt1.73 KBcutesquirrel
#32 remove_all_occurences-2400287-32.patch9.77 KBcutesquirrel
#30 remove_all_occurences-2400287-30.patch10.76 KBcutesquirrel
#30 interdiff-2400287-23-30.txt2.08 KBcutesquirrel
#23 Issue-2400287-Remove-all-occurences-of-sourceMapping.patch8.88 KBhass
#18 interdiff-13-18.txt2.37 KBborisson_
#18 remove_all_occurences-2400287-18.patch6.84 KBborisson_
#13 Issue-2400287-by-hass-Remove-all-occurences-of-source-D8b.patch7.55 KBhass
#11 Issue-2400287-by-hass-Remove-all-occurences-of-source-D8.patch7.59 KBhass
#6 Issue-2400287-by-hass-Remove-JS-source-and-source-map-D8.patch1.04 KBhass
#5 Issue-2400287-by-hass-Remove-JS-source-and-source-map-D7.patch860 byteshass
#3 Issue-2400287-by-hass-Remove-JS-source-and-source-map-D7.patch859 byteshass
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hass’s picture

Issue summary: View changes
hass’s picture

hass’s picture

Status: Needs review » Needs work
hass’s picture

Status: Needs work » Needs review
FileSize
860 bytes

*Damn* EGit bug. Reattaching.

hass’s picture

Version: 7.x-dev » 8.0.x-dev
Component: base system » asset library system
FileSize
1.04 KB

And here the D8 patch.

hass’s picture

Issue summary: View changes

Status: Needs review » Needs work

Status: Needs work » Needs review
hass’s picture

Assigned: Unassigned »
Status: Needs review » Needs work

Needs test

hass’s picture

Status: Needs work » Needs review
FileSize
7.59 KB

Status: Needs review » Needs work
hass’s picture

Status: Needs work » Needs review
FileSize
7.55 KB

Cannot run unit tests locally on windows. Try to find the root cause with d.o

Status: Needs review » Needs work
hass’s picture

Assigned: » Unassigned

I have no idea what's wrong here and cannot test locally. If someone can jump in an fix the failing test it would be great. The clean code itself works well in core, it's only the tests that fail.

moshe weitzman’s picture

Priority: Normal » Major

This is causing a 404 on a vanilla install when Dev tools are enabled.

borisson_’s picture

Assigned: Unassigned » borisson_
Issue tags: +SprintWeekend2015

Going to try to get test working again

borisson_’s picture

Status: Needs work » Needs review
FileSize
6.84 KB
2.37 KB

Test is now succesful locally

borisson_’s picture

Assigned: borisson_ » Unassigned
pfrenssen’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
    @@ -123,6 +123,8 @@ public function optimize(array $js_assets) {
    +              // Remove unwanted JS code that cause issues.
    +              $data = $this->optimizer->clean($data);
    

    I would be a bit more specific in the documentation. What kind of code is removed and why?

  2. +++ b/core/lib/Drupal/Core/Asset/JsOptimizer.php
    @@ -28,4 +28,20 @@ public function optimize(array $js_asset) {
    +  /**
    +   * Processes the contents of a javascript for cleanup.
    +   *
    +   * @param $contents
    +   *   The contents of the javascript.
    +   *
    +   * @return
    +   *   Contents of the javascript.
    +   */
    

    "A javascript" and "the javascript" reads a bit weird. I would say "a javascript file" or "javascript source code".

  3. +++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
    @@ -0,0 +1,75 @@
    + * @file
    + * Contains \Drupal\system\Tests\Asset\JsOptimizerUnitTest.
    

    This matches the path of the test, but it should match the namespace.

  4. +++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
    @@ -0,0 +1,75 @@
    +  protected function setUp() {
    +    parent::setUp();
    +
    +    $this->optimizer = new JsOptimizer();
    +  }
    

    {@inheritdoc} documentation is missing.

  5. +++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
    @@ -0,0 +1,75 @@
    +  function providerTestClean() {
    +    $path = dirname(__FILE__)  . '/js_test_files/';
    +    return array(
    +      // File. Tests:
    +      // - Stripped sourceMappingURL with comment # syntax.
    +      0 => array(
    +        file_get_contents($path . 'source_mapping_url.min.js'),
    +        file_get_contents($path . 'source_mapping_url.min.js.optimized.js'),
    +      ),
    +      // File. Tests:
    +      // - Stripped sourceMappingURL with comment @ syntax.
    +      1 => array(
    +        file_get_contents($path . 'source_mapping_url_old.min.js'),
    +        file_get_contents($path . 'source_mapping_url_old.min.js.optimized.js'),
    +      ),
    +      // File. Tests:
    +      // - Stripped sourceURL with comment # syntax.
    +      2 => array(
    +        file_get_contents($path . 'source_url.min.js'),
    +        file_get_contents($path . 'source_url.min.js.optimized.js'),
    +      ),
    +      // File. Tests:
    +      // - Stripped sourceURL with comment @ syntax.
    +      3 => array(
    +        file_get_contents($path . 'source_url_old.min.js'),
    +        file_get_contents($path . 'source_url_old.min.js.optimized.js'),
    +      ),
    +    );
    +  }
    

    Aren't we just comparing files that are already present in core? $this->optimizer seems to be unused in the test. Edit: I was mistaken, it is used in the test itself.

  6. +++ b/core/tests/Drupal/Tests/Core/Asset/js_test_files/source_mapping_url.min.js
    @@ -0,0 +1,2 @@
    +(function($) { "use strict"; })
    +//# sourceMappingURL=source_mapping_url.min.map
    \ No newline at end of file
    

    Coding standards: some of the test files do not end in a newline.

nod_’s picture

For the newline comment it's usual that minified files for vendor libraries do not end with a newline. Our tests should include at least a minified file without newline to make sure it works.

hass’s picture

Thanks for fixing the tests.

We need to extend the case I think. I heard that the same comments could be inside CSS. Additionally to this we could move the charset removal from CSS files to the clean function and add the clean function to the interface.

EDIT: /*# sourceMappingURL=<url> */ will be removed by our CSS compressor. However it would be cleaner to add the charset at least to the CSS cleanup

hass’s picture

Status: Needs work » Needs review
FileSize
8.88 KB

That patch is what I was thinking about.

The review from #20 seems mostly invalid.

  1. At this line we cannot say what it cleans. This is up to the clean function that describe what will be removed.
  2. Fixed
  3. Looks like an invalid comment or is already incorrect in CSS code.
  4. Looks like an invalid comment or is already incorrect in CSS code.
  5. As core has no JS compression, we cannot test anything. This is out of scope of this issue.
  6. Invalid report. This is how minified files look like.

Status: Needs review » Needs work

The last submitted patch, 23: Issue-2400287-Remove-all-occurences-of-sourceMapping.patch, failed testing.

pfrenssen’s picture

@Hass, if you copy incomplete or incorrect code from another test that doesn't mean that it is acceptable in a new patch. Regarding #3 - the namespace mentioned in the @file docblock is different from the actual namespace used, so it is clearly incorrect. #4 has missing documentation which is against standards.

I agree on #5 and #6 these can be disregarded. I have no strong feelings about #1 but this documentation is so vague that it raises more questions than answers in my mind.

cilefen’s picture

The 404's are not a production error because they only occur in the context of browser devtools, as I have demonstrated in #1341792-77: [meta] Ship minified versions of external JavaScript libraries.

hass’s picture

Nevertheless we should remove these on aggregation as map files will never fit for the aggregated code.

cutesquirrel’s picture

Assigned: Unassigned » cutesquirrel
cutesquirrel’s picture

Status: Needs work » Needs review
FileSize
2.08 KB
10.76 KB

Hi,
the last failed test seems only due to a "@charset" : since the patch, the charset are not anymore removed (normal).
So i've added it to optimized css version, and replace '' by "".
Hope this helps

hass’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
    @@ -179,7 +179,11 @@ function providerTestOptimize() {
    +  //  $this->verbose($t);
    

    Debug?

  2. +++ b/core/tests/Drupal/Tests/Core/Asset/css_test_files/charset.css.optimized.css
    @@ -1 +1 @@
    +@charset "UTF-8";html{font-family:"sans-serif";}
    

    This is wrong. All charsets need to be removed on CSS optimization.

cutesquirrel’s picture

Status: Needs work » Needs review
FileSize
9.77 KB
1.73 KB

Oups, it was a bit too late in the night... my mind wasn't effective !
Now it should be ok :)
Thanks @hass !

hass’s picture

Great. :-) What about the commented verbose()?

rteijeiro’s picture

Assigned: cutesquirrel » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: +drupaldevdays

@hass, I can't find the commented verbose you mention here. The patch looks good to me. Still applies and tests are ok in local environment. It's RTBC?

pfrenssen’s picture

Status: Reviewed & tested by the community » Needs work

This needs a bit of clean-up.

  1. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
    @@ -40,11 +40,29 @@ public function optimize(array $css_asset) {
    +   * @param $contents
    +   *   The contents of the CSS asset.
    +   *
    +   * @return
    +   *   Contents of the CSS asset.
    

    Data types are missing for $contents and the return value.

  2. +++ b/core/lib/Drupal/Core/Asset/CssOptimizer.php
    @@ -40,11 +40,29 @@ public function optimize(array $css_asset) {
    +    // Remove multiple charset declarations for standards compliance (and fixing Safari problems).
    

    Comment exceeds 80 characters.

  3. +++ b/core/lib/Drupal/Core/Asset/JsOptimizer.php
    @@ -28,4 +28,20 @@ public function optimize(array $js_asset) {
    +   * @param $contents
    +   *   The contents of the javascript asset.
    +   *
    +   * @return
    +   *   Contents of the javascript asset.
    

    Missing data types.

  4. +++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
    @@ -0,0 +1,75 @@
    +/**
    + * @file
    + * Contains \Drupal\system\Tests\Asset\JsOptimizerUnitTest.
    + */
    +
    +namespace Drupal\Tests\Core\Asset;
    

    The namespace in the docblock doesn't match the namespace in the class.

  5. +++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
    @@ -0,0 +1,75 @@
    +  protected function setUp() {
    +    parent::setUp();
    +
    +    $this->optimizer = new JsOptimizer();
    +  }
    

    This method doesn't have documentation, you can simply add {@inheritdoc} here.

  6. +++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
    @@ -0,0 +1,75 @@
    +  /**
    +   * Provides data for the JS asset cleaning test.
    +   */
    

    It would be nice if you could add an @see that refers to the method that uses this data.

    This should also document the return value. Usually people just say that it returns "an array of test data".

  7. +++ b/core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
    @@ -0,0 +1,75 @@
    +  /**
    +   * Tests clean a JS asset group containing 'type' => 'file'.
    +   *
    

    This sentence could use some love. What about "Tests cleaning of a JS asset group containing 'type' => 'file'"?

  8. +++ b/core/tests/Drupal/Tests/Core/Asset/js_test_files/source_mapping_url.min.js
    @@ -0,0 +1,2 @@
    +(function($) { "use strict"; })
    +//# sourceMappingURL=source_mapping_url.min.map
    

    Anonymous functions should have a space between "function" and the opening parenthesis. Or is this space stripped as part of the minifying process?

    This is in all the minified files.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
9.95 KB
2.68 KB

Fixed points 1,2,3,4,5,6 and 7 in #35. Not sure about point 8. Those JS files are manually created for the tests so no minified.

pfrenssen’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks Ruben! RTBC if the bot comes back green.

hass’s picture

Between #30 and #32 you removed the CssOptimizerUnitTest and therefore the verbose!? But now we have no unit test for this any longer.

hass’s picture

Status: Reviewed & tested by the community » Needs work
borisson_’s picture

Status: Needs work » Reviewed & tested by the community

The CssOptimizerUnitTest is still in source code, it got removed from the patch because the code that was used for testing was removed.

    static $i=0;
    $t =  $this->optimizer->optimize($css_asset);
    file_put_contents('/tmp/test_'.$i++.'.html',$t.PHP_EOL.'######'.PHP_EOL.$expected);
    //  $this->verbose($t);
    $this->assertEquals($expected, $t, 'Group of file CSS assets optimized correctly.');

All this code does is dump the css assets to the /tmp folder to manually test if the files are correctly optimized. The verbose method in there is a reference to the phpunit verbose method. (https://phpunit.de/manual/current/en/textui.html).

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Can someone double-check this patch is still valid, despite #2473837: Use minified jQuery once just got committed, which added a classmap?

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Rerun the tests and they all passed, they didn't rely on our JS libraries itself.
So we are safe.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great.

That foo vs. foo_old stuff was a little funny, but from a bit of Googling I see that this is basically an external standard outside of our control.

Committed and pushed to 8.0.x. Thanks!

aspilicious’s picture

Status: Fixed » Reviewed & tested by the community

Can you triple check? We don't see anything in the logs.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

AHEM! Sorry about that. :)

  • webchick committed e139632 on 8.0.x
    Issue #2400287 by hass, cutesquirrel, borisson_, rteijeiro, pfrenssen,...
hass’s picture

yuriy.babenko’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
701 bytes

I re-rolled patch in #5 (as-is) against 7.37. Works well on this end.

star-szr’s picture

Status: Reviewed & tested by the community » Needs review

Thanks @yuriy.babenko, patches need peer review though.

netbek’s picture

The patch in #48 unfortunately breaks lodash because the regex matches //# sourceURL= which isn't at the end of the minified file. It's part of the lodash template function: https://github.com/lodash/lodash/blob/b9d1a938c3770ed09589ce93d75e29afa1...

Is it common for source maps URLs to start on a new line? If so, then adding a start-of-line anchor to the pattern might work. Or should it check that matches are at the end of the file?

netbek’s picture

Thinking this through some more, a simple start-of-line check isn't enough. It should check if the source map comment is at the end of each file that's aggregated. I'll try to post a patch when time allows.

  • webchick committed e139632 on 8.1.x
    Issue #2400287 by hass, cutesquirrel, borisson_, rteijeiro, pfrenssen,...
deanflory’s picture

This was still an issue for D7 right? Just came across it.

hass’s picture

Yes

IRuslan’s picture

D7 version from #5 works good for me.
But I wonder why we are removing this info, instead of setting up a proper path to .map files to make them work?

  • webchick committed e139632 on 8.3.x
    Issue #2400287 by hass, cutesquirrel, borisson_, rteijeiro, pfrenssen,...

  • webchick committed e139632 on 8.3.x
    Issue #2400287 by hass, cutesquirrel, borisson_, rteijeiro, pfrenssen,...
deanflory’s picture

#5 still applies and appears to fix the error in D7.50

hass’s picture

Status: Needs review » Reviewed & tested by the community
stefan.r’s picture

The regex is identical to the one in D8 so this seems fine.

In the reroll an extra "// Prefix filename to prevent blocking by firewalls which reject files" slipped in though, can anyone post a proper reroll?

stefan.r’s picture

Status: Reviewed & tested by the community » Needs work
aerozeppelin’s picture

Status: Needs review » Needs work

The last submitted patch, 62: 2400287-61.patch, failed testing.

  • webchick committed e139632 on 8.4.x
    Issue #2400287 by hass, cutesquirrel, borisson_, rteijeiro, pfrenssen,...

  • webchick committed e139632 on 8.4.x
    Issue #2400287 by hass, cutesquirrel, borisson_, rteijeiro, pfrenssen,...
deanflory’s picture

So did #62 pass or fail? What is the appropriate patch for D7.54? #5? #62?

David_Rothstein’s picture

Status: Needs work » Needs review
David_Rothstein’s picture

I moved it back to needs review since the tests were passing. However it looks like #50 and #51 still need discussion?

pandaski’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/includes/common.inc
@@ -5091,6 +5091,8 @@ function drupal_build_js_cache($files) {
+    $contents = preg_replace('/\/\/(#|@)\s(sourceURL|sourceMappingURL)=\s*(\S*?)\s*$/m', '', $contents);

This line is matching the D8 snippet.

This is RTBC to D7



  /**
   * Processes the contents of a javascript asset for cleanup.
   *
   * @param string $contents
   *   The contents of the javascript asset.
   *
   * @return string
   *   Contents of the javascript asset.
   */
  public function clean($contents) {
    // Remove JS source and source mapping urls or these may cause 404 errors.
    $contents = preg_replace('/\/\/(#|@)\s(sourceURL|sourceMappingURL)=\s*(\S*?)\s*$/m', '', $contents);

    return $contents;
  }
minakshiPh’s picture

Hi,

The patch in #62 resolves my error coming for another module https://www.drupal.org/project/janrain_capture/issues/3145691#comment-13730221 so marking it RTBC.

+RTBC

Thanks,
minakshiPh

cyb_tachyon’s picture

Noting here that this doesn't seem to properly move webpack URI's and can break aggregated third party JS.

Example: //# sourceURL=webpack://Quill/ is stripped from //# sourceURL=webpack://Quill/./assets/icons/align-left.svg? - it seems to stop parsing at the ./ character when a '.' is a valid part of a webpack URI.

Without agg:

With agg:

mcdruid’s picture

Issue tags: +Pending Drupal 7 commit

Confirmed (per #69) that the line added in #62 is exactly the same as what's in D9:

https://git.drupalcode.org/project/drupal/-/blob/9.1.5/core/lib/Drupal/C...

  • mcdruid committed fbb5903 on 7.x
    Issue #2400287 by hass, cutesquirrel, borisson_, rteijeiro, aerozeppelin...
mcdruid’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Per #68, if #50 / #51 is still a problem that's the case in D8/9 too and would require a follow-up for D7/8/9.

Thanks!

Status: Fixed » Closed (fixed)

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

filipetakanap’s picture

ye, i still have this error for a long time even after updates... D9