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

Adding the refresh patch and unit tests #5673

Closed
wants to merge 12 commits into from

Conversation

Rajinsharwar
Copy link

Adding the proper SQL code using wpdb:prepare, and unit tests.

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


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.

// phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared
$post_id = $wpdb->get_var( "SELECT ID FROM $wpdb->posts WHERE $where AND post_status IN ('" . implode( "', '", esc_sql( $publicly_viewable_statuses ) ) . "')" );
$post_id = $wpdb->get_var( $wpdb->prepare( "SELECT ID FROM $wpdb->posts WHERE $where AND post_status IN (%s) AND post_type IN (%s)", implode( "', '", esc_sql( $publicly_viewable_statuses ) ), implode( "', '", esc_sql( $publicly_viewable_post_types ) ) ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

The %s replacement doesn't support IN queries, this change will result in escaped quotes, eg post_type IN ('\'post\', \'page\'') so will never return any results. It will look for a single post type with the slug 'post', 'page' which won't exist.

I saw your screen shot that my original patch caused an error, so I'll see where I goofed -- it's possibly because I duplicated the post_type IN clause.

Copy link
Contributor

@audrasjb audrasjb left a comment

Choose a reason for hiding this comment

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

I left a comment about one inline comment


// Test 'strict' redirect guess.
add_filter( 'strict_redirect_guess_404_permalink', '__return_true' );
// Modify the following line to expect false due to the patch
Copy link
Contributor

Choose a reason for hiding this comment

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

I remove this inline comment as it seems to be useless to me since it references a "patch" without mentioning what patch ;)

@peterwilsoncc
Copy link
Contributor

@Rajinsharwar I've created a follow up PR #5867 which incorporates your fix to my original, flawed, patch.

Rather than modify assertCanonical, it uses a different test to ensure that private posts are not redirected to.

@peterwilsoncc
Copy link
Contributor

peterwilsoncc commented Feb 16, 2024

Follow up PR merged r57645 / 96de28c

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.

3 participants