[#132] Add Highlight.js syntax highlighter module.#138
Conversation
📝 WalkthroughWalkthroughAdds Highlight.js integration: dependency declaration, module enablement, editor toolbar and filter updates, default highlight_js settings, a new do_base library and a LibraryInfoAlter hook that appends a Gherkin language dependency when highlight_js is present. Changes
Sequence Diagram(s)sequenceDiagram
participant Core as Drupal Core (library system)
participant Libraries as Libraries array
participant Hook as do_base\LibraryInfoAlterHook
participant Highlight as highlight_js module
Core->>Libraries: gather libraries (including highlight_js.custom)
Core->>Hook: invoke library_info_alter(libraries, extension)
Hook->>Libraries: check for extension == 'highlight_js' and 'highlight_js.custom' lib
alt highlight_js present and custom lib found
Hook->>Libraries: append dependency 'do_base/highlight_js.gherkin' to highlight_js.custom.dependencies
end
Libraries->>Core: return modified libraries
Core->>Highlight: load highlight_js assets (now with gherkin dependency)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #138 +/- ##
=======================================
Coverage 0.00% 0.00%
=======================================
Files 1 2 +1
Lines 8 10 +2
=======================================
- Misses 8 10 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/default/filter.format.civictheme_rich_text.yml`:
- Around line 51-56: The YAML has extra spaces inside the empty mapping for the
highlight_js settings; update the highlight_js block (the settings key in the
highlight_js entry) to use an empty mapping without interior spaces (e.g.,
change settings: { } to settings: {}) to satisfy YAML lint rules.
In `@config/default/highlight_js.settings.yml`:
- Around line 8-9: YAML lint error comes from extra spaces inside the empty-map
braces for the keys role_copy_access and languages; fix by replacing the current
values "{ }" with compact empty-map "{}" for both role_copy_access and
languages so they are valid YAML empty mappings.
| highlight_js: | ||
| id: highlight_js | ||
| provider: highlight_js | ||
| status: true | ||
| weight: -40 | ||
| settings: { } |
There was a problem hiding this comment.
Fix YAML lint: too many spaces inside empty braces.
YAMLlint reports a formatting error on line 56.
🔧 Proposed fix
- settings: { }
+ settings: {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| highlight_js: | |
| id: highlight_js | |
| provider: highlight_js | |
| status: true | |
| weight: -40 | |
| settings: { } | |
| highlight_js: | |
| id: highlight_js | |
| provider: highlight_js | |
| status: true | |
| weight: -40 | |
| settings: {} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 56-56: too many spaces inside empty braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/default/filter.format.civictheme_rich_text.yml` around lines 51 - 56,
The YAML has extra spaces inside the empty mapping for the highlight_js
settings; update the highlight_js block (the settings key in the highlight_js
entry) to use an empty mapping without interior spaces (e.g., change settings: {
} to settings: {}) to satisfy YAML lint rules.
| role_copy_access: { } | ||
| languages: { } |
There was a problem hiding this comment.
Fix YAML lint: too many spaces inside empty braces on both lines.
YAMLlint flags both role_copy_access and languages values.
🔧 Proposed fix
-role_copy_access: { }
-languages: { }
+role_copy_access: {}
+languages: {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| role_copy_access: { } | |
| languages: { } | |
| role_copy_access: {} | |
| languages: {} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 8-8: too many spaces inside empty braces
(braces)
[error] 9-9: too many spaces inside empty braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/default/highlight_js.settings.yml` around lines 8 - 9, YAML lint error
comes from extra spaces inside the empty-map braces for the keys
role_copy_access and languages; fix by replacing the current values "{ }" with
compact empty-map "{}" for both role_copy_access and languages so they are valid
YAML empty mappings.
This comment has been minimized.
This comment has been minimized.
596666a to
58930b2
Compare
This comment has been minimized.
This comment has been minimized.
58930b2 to
e227ca1
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composer.json`:
- Line 31: You added "drupal/highlight_js": "^1.2" to composer.json but did not
update and commit composer.lock; run composer require drupal/highlight_js:"^1.2"
(or composer update drupal/highlight_js) to regenerate composer.lock, verify the
resolved versions and tests, then commit and push the updated composer.lock
alongside the composer.json change so the transitive dependency tree is pinned
for CI and reproducible builds.
In `@config/default/seckit.settings.yml`:
- Line 11: The CSP style-src currently permits the broad prefix
"https://cdnjs.cloudflare.com/ajax/libs/highlight.js/" which allows any
highlight.js stylesheet; update the style-src value in seckit.settings.yml to
reference the exact versioned theme file used (e.g., the github theme such as
"https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/styles/github.min.css")
or, if highlight_js.theme is dynamic, change the code that generates this CSP to
compute and inject the exact versioned URL from highlight_js.settings.yml (or
the module config) rather than the directory prefix so only the specific theme
file is allowed.
In `@tests/behat/features/seckit.feature`:
- Line 20: The current Behat assertion in tests/behat/features/seckit.feature
does an order-sensitive substring match for the Content-Security-Policy header
(the step that checks "style-src 'self' ... 'unsafe-inline' ..."), which will
false-fail if CSP sources are reordered; modify the test to be order-insensitive
by extracting the CSP header value, isolating the style-src directive (e.g., via
regex for "style-src[^;]*"), splitting it into individual sources/tokens, and
asserting that the expected set of sources (include "'self'", "'unsafe-inline'"
and each CDN URL from the original string) are present (and no unexpected
sources if desired) rather than asserting the entire directive as a single
substring. Ensure the change updates the Behat step implementation used by that
feature so future reordering of sources does not break the test.
In `@web/modules/custom/do_base/do_base.libraries.yml`:
- Line 3: The YAML lint error comes from extra spaces inside the inline mapping
braces on the external asset line; edit the mapping for the URL
"https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/languages/gherkin.min.js"
and remove the spaces immediately after '{' and before '}' so the options read
"{type: external, minified: true}" instead of "{ type: external, minified: true
}".
---
Duplicate comments:
In `@config/default/filter.format.civictheme_rich_text.yml`:
- Line 81: The YAML linter flags extra spaces in the empty map for the settings
key; replace the current `settings: { }` entry with a compact empty mapping
`settings: {}` to remove the extra spaces and satisfy the linter (look for the
`settings` key in filter.format.civictheme_rich_text.yml).
In `@config/default/highlight_js.settings.yml`:
- Around line 8-9: The YAML linter flags extra spaces inside empty braces for
the keys role_copy_access and languages; update their values from "{ }" to "{}"
to remove the extra spaces so the file conforms to YAMLlint rules and no longer
fails validation.
| "drupal/gin": "^4.0.6", | ||
| "drupal/gin_toolbar": "^2", | ||
| "drupal/google_tag": "^2.0.9", | ||
| "drupal/highlight_js": "^1.2", |
There was a problem hiding this comment.
composer.lock must be committed alongside this change.
The PR description acknowledges this is deferred, but shipping without an updated composer.lock means the resolved transitive dependency tree is undefined for all environments. CI may resolve a different constraint than intended, and reproducible builds are broken until the lock file is committed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composer.json` at line 31, You added "drupal/highlight_js": "^1.2" to
composer.json but did not update and commit composer.lock; run composer require
drupal/highlight_js:"^1.2" (or composer update drupal/highlight_js) to
regenerate composer.lock, verify the resolved versions and tests, then commit
and push the updated composer.lock alongside the composer.json change so the
transitive dependency tree is pinned for CI and reproducible builds.
| And the response header "Content-Security-Policy" should contain the value "report-uri /report-csp-violation" | ||
| And the response header "Content-Security-Policy" should contain the value "script-src 'self' https://www.googletagmanager.com https://www.gstatic.com https://www.recaptcha.net https://www.google.com https://cdnjs.cloudflare.com https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/js/tabby.min.js https://unpkg.com/@popperjs/core@2.11.6/dist/umd/popper.js https://unpkg.com/tippy.js@6.3.7/dist/tippy.umd.js;" | ||
| And the response header "Content-Security-Policy" should contain the value "style-src 'self' 'unsafe-inline' https://fonts.googleapis.com/ https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/css/tabby-ui.min.css https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.65.12/codemirror.css https://unpkg.com/tippy.js@6.3.7/dist/tippy.css https://cdnjs.cloudflare.com/ajax/libs/select2/4.0.13/css/select2.min.css;" | ||
| And the response header "Content-Security-Policy" should contain the value "style-src 'self' https://cdnjs.cloudflare.com/ajax/libs/highlight.js/ 'unsafe-inline' https://fonts.googleapis.com/ https://cdn.jsdelivr.net/gh/cferdinandi/tabby@12.0.3/dist/css/tabby-ui.min.css https://cdnjs.cloudflare.com/ajax/libs/codemirror/5.65.12/codemirror.css https://unpkg.com/tippy.js@6.3.7/dist/tippy.css https://cdnjs.cloudflare.com/ajax/libs/select2/4.0.13/css/select2.min.css;" |
There was a problem hiding this comment.
Order-sensitive substring match may produce false failures on CSP source reordering.
The Behat step should contain the value "style-src 'self' https://cdnjs... 'unsafe-inline'..." performs an exact substring match against the full CSP header string. If the seckit module emits the same sources in a different order (e.g., after an upgrade or config re-sort), the test fails even though the security posture is identical.
Consider verifying that seckit's source ordering is stable across upgrades, or note this as a known maintenance point whenever seckit or highlight_js module versions change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/behat/features/seckit.feature` at line 20, The current Behat assertion
in tests/behat/features/seckit.feature does an order-sensitive substring match
for the Content-Security-Policy header (the step that checks "style-src 'self'
... 'unsafe-inline' ..."), which will false-fail if CSP sources are reordered;
modify the test to be order-insensitive by extracting the CSP header value,
isolating the style-src directive (e.g., via regex for "style-src[^;]*"),
splitting it into individual sources/tokens, and asserting that the expected set
of sources (include "'self'", "'unsafe-inline'" and each CDN URL from the
original string) are present (and no unexpected sources if desired) rather than
asserting the entire directive as a single substring. Ensure the change updates
the Behat step implementation used by that feature so future reordering of
sources does not break the test.
| @@ -0,0 +1,5 @@ | |||
| highlight_js.gherkin: | |||
| js: | |||
| https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/languages/gherkin.min.js: { type: external, minified: true } | |||
There was a problem hiding this comment.
Fix YAML lint: remove extra spaces inside braces.
YAMLlint reports too many spaces inside braces on this line.
🔧 Proposed fix
- https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/languages/gherkin.min.js: { type: external, minified: true }
+ https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/languages/gherkin.min.js: {type: external, minified: true}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/languages/gherkin.min.js: { type: external, minified: true } | |
| https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/languages/gherkin.min.js: {type: external, minified: true} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 3-3: too many spaces inside braces
(braces)
[error] 3-3: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/modules/custom/do_base/do_base.libraries.yml` at line 3, The YAML lint
error comes from extra spaces inside the inline mapping braces on the external
asset line; edit the mapping for the URL
"https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.9.0/languages/gherkin.min.js"
and remove the spaces immediately after '{' and before '}' so the options read
"{type: external, minified: true}" instead of "{ type: external, minified: true
}".
Checklist before requesting a review
[#132] Verb in past tense.#132added to descriptionChangedsectionChanged
drupal/highlight_jsmodule for syntax highlighting in rich text contentstyle-srcto allow highlight.js styles from cdnjs.cloudflare.comdo_baseOOP hook (#[Hook("library_info_alter")]) — the highlight.js CDN common bundle only includes ~40 languages; Gherkin needs to be loaded separatelydrush config:exportScreenshots
Summary by CodeRabbit
New Features
Configuration
Tests