Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix attributes option ignoring values passed from the safe filter #4938

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Apr 18, 2024

Updates the govukAttributes macro to, when passed a set of key-value pairs, check if the value is an object that matches the shape of those returned by Nunjucks' safe filter (i.e. an object with val and length keys).

If found, that object's val is used in place of the object. Previously, this would just be lost and the macro would return an empty string.

If the value is an object that doesn't have those specific keys, it is instead treated as one of our attribute configuration objects (which use the keys value and optional instead).

Resolves #4937.

Changes

  • Adds a check at the start of the govukAttributes attribute loop to look for objects resembling the output of the safe filter. If matched, the object's val value is mapped to the macro's working value.
  • Adds a test to ensure that values passed via the safe filter are output by the macro as expected.

Thoughts

Checking for the existence of two keys is definitely not a foolproof way of ensuring that the object is actually one returned by the safe filter, however I'm not sure there is a more obvious way of detecting its use.

This does not create output that perfectly matches the expected output in #4937, as HTML is still returned escaped even if safe is used by the author. This is because the macro indiscriminately escapes all of the attributes and values it outputs.

Updates the `govukAttributes` macro to look for an object that matches the shape of those returned by Nunjucks' `safe` filter. If found, the value of that object is mapped to the value that should be used by the macro.
Copy link

github-actions bot commented Apr 18, 2024

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 113.24 KiB
dist/govuk-frontend-development.min.js 42.21 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 87.21 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 81.95 KiB
packages/govuk-frontend/dist/govuk/all.mjs 4.17 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.23 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 42.2 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.55 KiB

Modules

File Size (bundled) Size (minified)
all.mjs 77.67 KiB 40.19 KiB
accordion.mjs 22.71 KiB 12.85 KiB
button.mjs 5.98 KiB 2.69 KiB
character-count.mjs 22.4 KiB 9.92 KiB
checkboxes.mjs 5.83 KiB 2.83 KiB
error-summary.mjs 7.89 KiB 3.46 KiB
exit-this-page.mjs 17.1 KiB 9.26 KiB
header.mjs 4.46 KiB 2.6 KiB
notification-banner.mjs 6.26 KiB 2.62 KiB
password-input.mjs 15.15 KiB 7.25 KiB
radios.mjs 4.83 KiB 2.38 KiB
skip-link.mjs 4.39 KiB 2.18 KiB
tabs.mjs 10.13 KiB 6.11 KiB

View stats and visualisations on the review app


Action run for fe7a019

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4938 April 18, 2024 16:35 Inactive
@querkmachine querkmachine requested a review from a team April 18, 2024 17:13
@querkmachine querkmachine marked this pull request as ready for review April 18, 2024 17:13
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the speedy fix. The solution looks very fine to me, I'm not sure if we'd have access to the SafeString class to check types anyway (and trying to check the type could lead to subtle issues, which would complicate our testing).

Thinking we can explore the forced escaping in another PR if we get feedback it causes issues.

@querkmachine querkmachine merged commit e1b5611 into main Apr 19, 2024
48 checks passed
@querkmachine querkmachine deleted the fix-safe-attributes branch April 19, 2024 09:00
@querkmachine
Copy link
Member Author

I'll see about raising another issue specifically about using safe to return unescaped values, but that's definitely a much lower priority than this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

govukAttributes macro doesn't output values that use Nunjucks' safe filter
3 participants