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 url params in post url #324

Merged

Conversation

justinmaurerdotdev
Copy link
Contributor

Description of the Change

This addresses/closes: #323

The Tweet URL was being escaped by esc_url(), which converts ampersands to HTML entities, breaking any query parameters.
Example: http://example.org/?p=16&utm_source=x&utm_medium=social&utm_campaign=my_test&utm_content=auto-tweet&utm_term=post

Changing the escaping to use esc_url_raw() seems appropriate (since the URL isn't intended for HTML rendering, but an actual link) and fixes the problem.
Example: "http://example.org/?p=16&utm_source=x&utm_medium=social&utm_campaign=my_test&utm_content=auto-tweet&utm_term=post

Note: It looks like PHPCBF got a little excited about replacing global class references (e.g. \WP_Post) with use statements in includes/utils.php. Let me know if those should not added and I can revert those bits.

How to test the Change

I've added test_filter_url_with_params() to TestPublish_Tweet.php, and it is working as expected (fails with esc_url() and succeeds with esc_url_raw(). But, you can also just add a filter to see the behavior.

add_filter(
	'autoshare_for_twitter_post_url',
	function ( $url ) {
		return add_query_arg( array(
			'utm_source'   => 'x',
			'utm_medium'   => 'social',
			'utm_campaign' => 'my_test',
			'utm_content'  => 'auto-tweet',
			'utm_term'     => 'post',
		), $url );
	}
);

Changelog Entry

Fixed - Ampersands no longer converted to HTML entities when adding query parameters to the post URL with the 'autoshare_for_twitter_post_url' filter.

Credits

Props @justinmaurerdotdev

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@jeffpaul jeffpaul requested review from iamdharmesh and removed request for dkotter and jeffpaul May 2, 2024 18:33
@jeffpaul jeffpaul added this to the 2.3.0 milestone May 2, 2024
Copy link
Member

@iamdharmesh iamdharmesh left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR @justinmaurerdotdev. LGTM.

@iamdharmesh iamdharmesh merged commit 38ad796 into 10up:develop May 3, 2024
12 checks passed
@justinmaurerdotdev
Copy link
Contributor Author

Thanks for getting it merged!

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.

Query parameters incorrectly escaped in Tweet URL
3 participants