ci(js): lint the backend JavaScript, which nothing did - #855
Conversation
A file whose methods had landed outside their class passed the whole CI matrix — 71 green checks — and the rating buttons it shipped would not have worked. PHPStan does not read JavaScript, the functional suite renders the template without executing the module, and no Playwright spec covers that view. A person reading the diff found it. node --check was the cheaper candidate and lost on evidence, not on taste: it parses a .js file as a CommonJS script and ACCEPTED the broken file, while ESLint parses it as an ES module — which is how the browser loads it — and reports the parse error on the right line. That also settles a contradiction left open in #842's review: the file was a syntax error all along; node --check simply read it in the wrong mode. My stated reason for not deciding this earlier was wrong, and checking it took one command: "eslint would bring a JS toolchain into a PHP extension". package.json, package-lock.json and three devDependencies were already there, and CI already runs npm install for the e2e job. No sibling extension lints JavaScript — rte_ckeditor_image has a package.json for commitlint only, nr-vault one for Playwright, nr-repurpose none. There was no house standard to follow, which is a reason to set one rather than to wait. The rule set is small on purpose. no-undef is the error, because that is the rule that catches the defect above. no-unsanitized is loaded but set to warn: as an error it reports 12 assignments and NONE is a hole — the code escapes through escapeHtml(), visibly and with a comment saying so, and the rule cannot recognise a helper of ours as a sanitizer. Dropping the plugin was not an option either: two files already carry eslint-disable-line no-unsanitized/property, suppressions written for a rule that never ran, which become "rule not found" errors the moment linting is switched on. Those twelve are triaged in #854 with the evidence, because a security question belongs where it can be argued. The clean tree lints at 0 errors. Reintroducing the #825 shape — a method after the class's closing brace — fails with a parse error and exit 1. Thin caller of the shared script-check reusable, so no job is defined here; pre-command installs the devDependencies the runner does not ship. Closes #825 Signed-off-by: Sebastian Mendel <sebastian.mendel@netresearch.de>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
Dependency ReviewThe following issues were found:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #855 +/- ##
============================================
- Coverage 86.71% 86.56% -0.15%
- Complexity 9916 10009 +93
============================================
Files 574 574
Lines 32049 32215 +166
============================================
+ Hits 27792 27888 +96
- Misses 4257 4327 +70
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Review record — no Copilot review exists (monthly, account-wide quota), so the green gate is not a read of the change. The check I would not have thought to make, and did: The rule choice is measured, not preferred. The control was run in both directions. Clean tree: 0 errors, exit 0. #825's shape reintroduced — a method after the class's closing brace — parse error, exit 1. And Twelve sanitizer findings are deliberately warnings, and the reason is in #854, not here. None of them is a hole: the code escapes through One correction to my own earlier reasoning, stated in the PR body too: I had declined to decide this on the grounds that eslint would introduce a JS toolchain into a PHP extension. Repo rules: no job defined here (thin caller of the shared reusable), Merging on that basis. Assisted by claude-code:claude-opus-5 — Session |
|
Self-review: 633798d The review this pull request demands is unsatisfiable (Copilot quota wall or repeated bot failures on this head). Per the documented fallback, the diff on this head was reviewed by the PR author; this comment is the on-the-record attestation the merge gate reads back. It stops matching on the next push. |
Closes #825. A file whose methods had landed outside their class passed the whole CI matrix — 71 green checks — and the rating buttons it shipped would not have worked. PHPStan does not read JavaScript, the functional suite renders the template without executing the module, and no Playwright spec covers that view. A person reading the diff found it.
node --checklost on evidence, not on tasteIt was the cheap candidate: no dependency, no config. Tested against the actual broken file, it accepted it — because it parses a
.jsfile as a CommonJS script. ESLint parses it as an ES module, which is how the browser loads it, and reports the parse error on the right line.That also settles something left open in #842's review. I had written there that I could not reconcile
node --checkpassing with a minimal reproduction failing, and would not dress it up. The answer is the parsing mode: the file was a syntax error all along, andnode --checkread it in the wrong one.My reason for not deciding this earlier was wrong
I had said eslint "would bring a JS toolchain into a PHP extension that has none". One command disproves it:
package.json,package-lock.json, three devDependencies including Playwright, and CI already runsnpm installfor the e2e job. ESLint is one more line in a file that exists.No sibling extension lints JavaScript —
rte_ckeditor_imagehas apackage.jsonfor commitlint and husky only,nr-vaultone for Playwright,nr-repurposenone at all. There was no house standard to copy, which is a reason to set one rather than to wait for it.The rule set is small on purpose
no-undefis an error: it is the rule that catches the defect above.no-unsanitizedis loaded but set to warn, and the reason is measured rather than cautious. As an error it reports 12 assignments, and none of them is a hole — the code escapes throughescapeHtml(), visibly, with a comment saying so:The rule recognises a fixed set of sanitizers and a helper of ours is not among them. Dropping the plugin was not an option either: two files already carry
eslint-disable-line no-unsanitized/property— suppressions written for a rule that never ran, which become "rule not found" errors the moment linting is switched on.Those twelve are triaged in #854 with the evidence and three options. A security question belongs where it can be argued and closed, not in a config comment.
Verification
npm cifrom the lockfile, then lintRepo rules: no job is defined here — thin caller of the shared
script-checkreusable,check-workflow-ownership.phppasses, anddrift_compare.pyagainst the org template reports no drift.pre-commandinstalls the devDependencies the runner does not ship.One thing worth knowing beyond this repo
This makes nr_llm the first Netresearch TYPO3 extension with a JavaScript gate. If it holds up, it belongs in the shared template rather than being rebuilt per repository — otherwise the next extension gets the same 71-green-checks surprise.
Assisted by claude-code:claude-opus-5 — Session