Skip to content

Commit 8747d4b

Browse files
renatomarinhorenatomarinho
committed
fix: correctly remove JavaScript // comments without breaking code
- Fix RemoveComments middleware to preserve // inside strings - Implement character-by-character parser for single-line comments - Add comprehensive test suite for edge cases - Fix issue where comments after strings caused code to break - Preserve URLs with // (http://, https://) - Maintain line ending styles (\r\n, \n, \r) Tests added: - JavaScriptCommentsBugTest: Tests for reported bug scenarios - EdgeCaseCommentsTest: Edge cases (strings, regex, URLs) - Enhanced existing tests with new assertions All 41 tests passing with 183 assertions. Fixes issue where // comments were incorrectly removed from inside strings, causing JavaScript to break when combined with CollapseWhitespace middleware.
1 parent bc0a32c commit 8747d4b

File tree

9 files changed

+528
-40
lines changed

9 files changed

+528
-40
lines changed

‎.github/dependabot.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ updates:
1717
include: "scope"
1818
versioning-strategy: increase
1919
target-branch: "develop"
20-
20+
2121
# Enable version updates for GitHub Actions
2222
- package-ecosystem: "github-actions"
2323
directory: "/"

‎.github/workflows/auto-merge.yml‎

Lines changed: 31 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,38 @@
11
name: Auto Merge
22

33
on:
4-
pull_request:
5-
types: [opened, synchronize, reopened, labeled]
6-
pull_request_review:
7-
types: [submitted]
4+
pull_request:
5+
types: [opened, synchronize, reopened, labeled]
6+
pull_request_review:
7+
types: [submitted]
88

99
permissions:
10-
contents: write
11-
pull-requests: write
10+
contents: write
11+
pull-requests: write
1212

1313
jobs:
14-
auto-merge:
15-
name: Auto Merge Dependabot PRs
16-
runs-on: ubuntu-latest
17-
if: github.actor == 'dependabot[bot]'
18-
19-
steps:
20-
- name: Dependabot metadata
21-
id: metadata
22-
uses: dependabot/fetch-metadata@v2
23-
with:
24-
github-token: "${{ secrets.GITHUB_TOKEN }}"
25-
26-
- name: Enable auto-merge for Dependabot PRs
27-
if: steps.metadata.outputs.update-type == 'version-update:semver-patch' || steps.metadata.outputs.update-type == 'version-update:semver-minor'
28-
run: gh pr merge --auto --squash "$PR_URL"
29-
env:
30-
PR_URL: ${{ github.event.pull_request.html_url }}
31-
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
32-
33-
- name: Approve patch and minor updates
34-
if: steps.metadata.outputs.update-type == 'version-update:semver-patch' || steps.metadata.outputs.update-type == 'version-update:semver-minor'
35-
run: gh pr review --approve "$PR_URL"
36-
env:
37-
PR_URL: ${{ github.event.pull_request.html_url }}
38-
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
14+
auto-merge:
15+
name: Auto Merge Dependabot PRs
16+
runs-on: ubuntu-latest
17+
if: github.actor == 'dependabot[bot]'
18+
19+
steps:
20+
- name: Dependabot metadata
21+
id: metadata
22+
uses: dependabot/fetch-metadata@v2
23+
with:
24+
github-token: "${{ secrets.GITHUB_TOKEN }}"
25+
26+
- name: Enable auto-merge for Dependabot PRs
27+
if: steps.metadata.outputs.update-type == 'version-update:semver-patch' || steps.metadata.outputs.update-type == 'version-update:semver-minor'
28+
run: gh pr merge --auto --squash "$PR_URL"
29+
env:
30+
PR_URL: ${{ github.event.pull_request.html_url }}
31+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
32+
33+
- name: Approve patch and minor updates
34+
if: steps.metadata.outputs.update-type == 'version-update:semver-patch' || steps.metadata.outputs.update-type == 'version-update:semver-minor'
35+
run: gh pr review --approve "$PR_URL"
36+
env:
37+
PR_URL: ${{ github.event.pull_request.html_url }}
38+
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

‎.github/workflows/pull-request.yml‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ jobs:
1515
name: Validate PR
1616
runs-on: ubuntu-latest
1717
if: github.event.pull_request.draft == false
18-
18+
1919
steps:
2020
- name: Checkout code
2121
uses: actions/checkout@v4
@@ -83,16 +83,16 @@ jobs:
8383
} catch (e) {
8484
console.log('Could not read coverage');
8585
}
86-
86+
8787
const body = `## 🔍 Pull Request Validation Results
88-
88+
8989
- ✅ **Code Style**: PSR-2 compliant
9090
- ✅ **Tests**: All passing
9191
- 📊 **Coverage**: ${coverage}
9292
- 🚀 **Ready to merge**
93-
93+
9494
*Automated validation by GitHub Actions*`;
95-
95+
9696
github.rest.issues.createComment({
9797
issue_number: context.issue.number,
9898
owner: context.repo.owner,

‎src/Middleware/RemoveComments.php‎

Lines changed: 131 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,146 @@
44

55
class RemoveComments extends PageSpeed
66
{
7-
const REGEX_MATCH_JS_AND_CSS_COMMENTS = '/(?:(?:\/\*(?:[^*]|(?:\*+[^*\/]))*\*+\/)|(?:(?<!\:|\\\|\'|\")\/\/.*))/';
7+
const REGEX_MATCH_MULTILINE_COMMENTS = '/\/\*(?:[^*]|(?:\*+[^*\/]))*\*+\//';
88
const REGEX_MATCH_HTML_COMMENTS = '/<!--[^]><!\[](.*?)[^\]]-->/s';
99

1010
public function apply($buffer)
1111
{
12-
$buffer = $this->replaceInsideHtmlTags(['script', 'style'], self::REGEX_MATCH_JS_AND_CSS_COMMENTS, '', $buffer);
12+
// First, remove multi-line comments (/* ... */)
13+
$buffer = $this->replaceInsideHtmlTags(['script', 'style'], self::REGEX_MATCH_MULTILINE_COMMENTS, '', $buffer);
14+
15+
// Then, remove single-line comments (//) more carefully
16+
$buffer = $this->removeSingleLineComments($buffer);
1317

1418
$replaceHtmlRules = [
1519
self::REGEX_MATCH_HTML_COMMENTS => '',
1620
];
1721

1822
return $this->replace($replaceHtmlRules, $buffer);
1923
}
24+
25+
/**
26+
* Remove single-line comments (//) from script tags while preserving them inside strings
27+
*
28+
* @param string $buffer
29+
* @return string
30+
*/
31+
protected function removeSingleLineComments($buffer)
32+
{
33+
foreach ($this->matchAllHtmlTag(['script', 'style'], $buffer) as $tagMatched) {
34+
$tagAfterReplace = $this->removeCommentsFromTag($tagMatched);
35+
$buffer = str_replace($tagMatched, $tagAfterReplace, $buffer);
36+
}
37+
38+
return $buffer;
39+
}
40+
41+
/**
42+
* Remove // comments from a script/style tag content
43+
*
44+
* @param string $tag
45+
* @return string
46+
*/
47+
protected function removeCommentsFromTag($tag)
48+
{
49+
// Detect line ending style
50+
$lineEnding = "\n";
51+
if (strpos($tag, "\r\n") !== false) {
52+
$lineEnding = "\r\n";
53+
} elseif (strpos($tag, "\r") !== false) {
54+
$lineEnding = "\r";
55+
}
56+
57+
// Split by lines to process each line
58+
$lines = preg_split('/\r\n|\r|\n/', $tag);
59+
$processedLines = [];
60+
61+
foreach ($lines as $line) {
62+
$processedLines[] = $this->removeSingleLineCommentFromLine($line);
63+
}
64+
65+
return implode($lineEnding, $processedLines);
66+
}
67+
68+
/**
69+
* Remove // comment from a single line while preserving // inside strings
70+
*
71+
* @param string $line
72+
* @return string
73+
*/
74+
protected function removeSingleLineCommentFromLine($line)
75+
{
76+
$result = '';
77+
$length = strlen($line);
78+
$inSingleQuote = false;
79+
$inDoubleQuote = false;
80+
$inRegex = false;
81+
$escaped = false;
82+
83+
for ($i = 0; $i < $length; $i++) {
84+
$char = $line[$i];
85+
$nextChar = $i + 1 < $length ? $line[$i + 1] : '';
86+
$prevChar = $i > 0 ? $line[$i - 1] : '';
87+
88+
// Handle escape sequences
89+
if ($escaped) {
90+
$result .= $char;
91+
$escaped = false;
92+
continue;
93+
}
94+
95+
if ($char === '\\' && ($inSingleQuote || $inDoubleQuote || $inRegex)) {
96+
$result .= $char;
97+
$escaped = true;
98+
continue;
99+
}
100+
101+
// Toggle quote states
102+
if ($char === '"' && !$inSingleQuote && !$inRegex) {
103+
$inDoubleQuote = !$inDoubleQuote;
104+
$result .= $char;
105+
continue;
106+
}
107+
108+
if ($char === "'" && !$inDoubleQuote && !$inRegex) {
109+
$inSingleQuote = !$inSingleQuote;
110+
$result .= $char;
111+
continue;
112+
}
113+
114+
// Handle regex literals (basic detection)
115+
if ($char === '/' && !$inSingleQuote && !$inDoubleQuote) {
116+
// Check if this might be a regex literal
117+
// Simple heuristic: regex usually comes after =, (, [, ,, return, or at start
118+
if ($prevChar === '=' || $prevChar === '(' || $prevChar === '[' || $prevChar === ',' || $prevChar === ' ') {
119+
// Look ahead to see if this looks like a regex (not a comment)
120+
if ($nextChar !== '/' && $nextChar !== '*') {
121+
$inRegex = true;
122+
$result .= $char;
123+
continue;
124+
}
125+
}
126+
127+
// End of regex literal
128+
if ($inRegex) {
129+
$inRegex = false;
130+
$result .= $char;
131+
continue;
132+
}
133+
}
134+
135+
// Check for // comment outside of strings
136+
if (!$inSingleQuote && !$inDoubleQuote && !$inRegex && $char === '/' && $nextChar === '/') {
137+
// Check if this is not part of a URL (preceded by :)
138+
if ($prevChar !== ':') {
139+
// Found a comment, remove everything from here to end of line
140+
break;
141+
}
142+
}
143+
144+
$result .= $char;
145+
}
146+
147+
return $result;
148+
}
20149
}

‎tests/Boilerplate/index.html‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,9 @@ <h1>Test Background Image</h1>
8787
*/
8888
console.log('Page');
8989
/* before - inline comment*/console.log('Speed!');// after - inline comment
90+
var url = "http://example.com"; // This comment should be removed
91+
var text = "Some text"; // This comment should also be removed
92+
console.log('Important code'); // Don't break this
9093
</script>
9194

9295
<script src="https://github.com/vinkius-labs/laravel-page-speed/test/Boilerplate/js/vendor/modernizr-3.5.0.min.js"></script>

‎tests/Middleware/CollapseWhitespaceTest.php‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,29 @@ public function test_collapse_whitespace(): void
3939
$this->assertSame($compress, trim($partial[0]));
4040

4141
$this->assertStringContainsString(
42-
"<script> console.log('Laravel'); console.log('Page'); console.log('Speed!'); </script>",
42+
"<script> console.log('Laravel'); console.log('Page'); console.log('Speed!'); var url = \"http://example.com\"; var text = \"Some text\"; console.log('Important code'); </script>",
4343
$this->response->getContent()
4444
);
4545
}
46+
47+
public function test_javascript_not_broken_by_comment_removal_and_whitespace_collapse(): void
48+
{
49+
// This test ensures that when comments are removed and whitespace is collapsed,
50+
// the JavaScript code remains functional and nothing is accidentally commented out
51+
$content = $this->response->getContent();
52+
53+
// Ensure all expected JavaScript statements are present
54+
$this->assertStringContainsString("console.log('Laravel');", $content);
55+
$this->assertStringContainsString("console.log('Page');", $content);
56+
$this->assertStringContainsString("console.log('Speed!');", $content);
57+
$this->assertStringContainsString('var url = "http://example.com";', $content);
58+
$this->assertStringContainsString('var text = "Some text";', $content);
59+
$this->assertStringContainsString("console.log('Important code');", $content);
60+
61+
// Ensure comments are removed
62+
$this->assertStringNotContainsString("// This comment should be removed", $content);
63+
$this->assertStringNotContainsString("// This comment should also be removed", $content);
64+
$this->assertStringNotContainsString("// Don't break this", $content);
65+
$this->assertStringNotContainsString("// Single Line Comment", $content);
66+
}
4667
}

0 commit comments

Comments
 (0)