KSES: allow port numbers in pdf object url #1998
Conversation
Testing a way to cater for port numbers in pdf $url argument. Renaming $value to $url to match PHP doc comment. Adding tests
ramonjd
changed the title
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
A few notes, two of which are nitpicks.
Co-authored-by: Peter Wilson <[email protected]>
Co-authored-by: Peter Wilson <[email protected]>
…if I read the docs for this filter.
|
Tests are passing now 😄 Are there any further tests required for this change (assuming the change itself is sound)? |
|
It'd be good to put some unhappy paths in to make sure they fail with the ports too, for example example.org should be removed if the upload url is example.org:8888 I don't think the new test will need as many scenarios as the other data provider as it covers most of the unrelated to port unhappy paths. According to the WordPress Slack, your on vacation at the moment so I can look in to that and push to your repo if you do not mind. |
Great call. I've added a couple more, but feel free to edit/delete/change as you like.
Hah, I'm mostly around. Thanks for offering!! I just like trees 🌴 |
Unit tests are currently failing on `trunk` (when used with a current snapshot of the WordPress svn or git repo). Specifically: `Block_Fixture_Test::test_kses_doesnt_change_fixtures` fails for datasets #54 and #55. Reading WordPress/wordpress-develop#1998, I believe that the reason might be that our KSES routines are verifying that the specified port matches the one of the local install. And since our tests normally run `localhost:8889`, I'm updating the relevant test fixtures (from previously `8888`) to reflect that.
Unit tests are currently failing on `trunk` (when used with a current snapshot of the WordPress svn or git repo). Specifically: `Block_Fixture_Test::test_kses_doesnt_change_fixtures` fails for datasets #54 and #55. Reading WordPress/wordpress-develop#1998, I believe that the reason might be that our KSES routines are verifying that the specified port matches the one of the local install. And since our tests normally run `localhost:8889`, I'm updating the relevant test fixtures (from previously `8888`) to reflect that.
Unit tests are currently failing on `trunk` (when used with a current snapshot of the WordPress svn or git repo). Specifically: `Block_Fixture_Test::test_kses_doesnt_change_fixtures` fails for datasets #54 and #55. Reading WordPress/wordpress-develop#1998, I believe that the reason might be that our KSES routines are verifying that the specified port matches the one of the local install. And since our tests normally run `localhost:8889`, I'm updating the relevant test fixtures (from previously `8888`) to reflect that.
2 participants
This patch tests a way to cater for port numbers in pdf
$urlargument.Renaming
$valueto$urlto match PHP doc comment.See comment https://core.trac.wordpress.org/ticket/54261#comment:15
Trac ticket: https://core.trac.wordpress.org/ticket/54261
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.