Skip to content

safecss_filter_attr: Add exceptions for min(), max(), etc. functions#3212

Closed
noisysocks wants to merge 9 commits intoWordPress:trunkfrom
noisysocks:fix/min-in-safecss_filter_attr
Closed

safecss_filter_attr: Add exceptions for min(), max(), etc. functions#3212
noisysocks wants to merge 9 commits intoWordPress:trunkfrom
noisysocks:fix/min-in-safecss_filter_attr

Conversation

@noisysocks
Copy link
Copy Markdown
Member

@noisysocks noisysocks commented Sep 7, 2022

Trac ticket: https://core.trac.wordpress.org/ticket/55966

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

noisysocks
noisysocks commented Sep 8, 2022
View reviewed changes
@noisysocks
Copy link
Copy Markdown
Member Author

noisysocks commented Sep 8, 2022 •
edited
Loading

I added a few more test cases to this, combined the two regexes that handle CSS functions, and rewrote said regex so that it checks for balanced parentheses.

I'm not sure what this CI failure is about:

Run git diff --exit-code
diff --git a/tests/phpunit/data/images/canola-100x75-jpg.webp b/tests/phpunit/data/images/canola-100x75-jpg.webp
index ef484c7..24bc477 100644
Binary files a/tests/phpunit/data/images/canola-100x75-jpg.webp and b/tests/phpunit/data/images/canola-100x75-jpg.webp differ

Do you know @hellofromtonya @peterwilsoncc @SergeyBiryukov?

edit: lol I accidentally git added a test fixture.

noisysocks
noisysocks commented Sep 8, 2022
View reviewed changes
tests/phpunit/tests/theme/wpThemeJson.php
),
'core/cover' => array(
'filter' => array(
'duotone' => 'var(--wp--preset--duotone--blue-red, var(--fallback-unsafe))',
Copy link
Copy Markdown
Member Author

@noisysocks noisysocks Sep 8, 2022

Choose a reason for hiding this comment

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

These changes to the WP_Theme_JSON tests are similar to WordPress/gutenberg#31982.

We were relying on the fact safecss_filter_attr doesn't support nested var() functions in order to test that attributes were being passed through safecss_filter_attr. Since this PR makes it so that safecss_filter_attr supports nested var() functions, we need to change the test cases to a different invalid CSS string.

cc. @oandregal to make sure I'm not mistaken 😀

Copy link
Copy Markdown
Member

@SergeyBiryukov SergeyBiryukov Sep 8, 2022

Choose a reason for hiding this comment

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

That is my understanding too 👍

Copy link
Copy Markdown
Member

@oandregal oandregal Sep 8, 2022

Choose a reason for hiding this comment

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

Yeah, changes make sense. Thanks!

@noisysocks
Copy link
Copy Markdown
Member Author

noisysocks commented Sep 8, 2022

Tests passing! I’m pretty happy with this. What do you think @SergeyBiryukov?

@ockham ockham mentioned this pull request Sep 8, 2022
Closed
37 tasks
@SergeyBiryukov
Copy link
Copy Markdown
Member

SergeyBiryukov commented Sep 8, 2022

Thanks for the PR! Merged in r54100.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oandregal oandregal oandregal left review comments

@SergeyBiryukov SergeyBiryukov SergeyBiryukov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

No open projects
WordPress 6.1 Editor Tasks
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@noisysocks @SergeyBiryukov @oandregal