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

Add X-Redirect-By header to all redirects #9887

Merged
merged 1 commit into from May 29, 2018

Conversation

Projects
None yet
3 participants
@atimmer
Copy link
Member

atimmer commented May 29, 2018

Summary

This PR can be summarized in the following changelog entry:

  • Adds X-Redirect-By header to all redirects. Making the origin of redirects much easier to debug.

Relevant technical choices:

  • Because redirects are not abstracted I just added the header call before every redirect.

Test instructions

This PR can be tested by following these steps:

  • Test a redirect using a tool such as curl -I and see the X-Redirect-By header

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended
Add X-Redirect-By header to all redirects
They were previously missing, this makes redirects much easier
to debug.
@andizer
Copy link
Contributor

andizer left a comment

I have two things that we should keep in mind.

@@ -1675,6 +1676,7 @@ protected function is_multiple_terms_query() {
* @param int $status Status code to use.
*/
public function redirect( $location, $status = 302 ) {
header( 'X-Redirect-By: Yoast SEO' );

This comment has been minimized.

@andizer

andizer May 29, 2018

Contributor

We should be aware that WordPress 5.0.0 introduces a third argument for setting the x-redirect-by header. I don't know what will happen when two arguments are set twice. But I guess this header will be overwritten.

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/pluggable.php#L1341

@@ -1339,6 +1339,7 @@ public function page_redirect() {
$redir = $this->get_seo_meta_value( 'redirect', $post->ID );
if ( $redir !== '' ) {
header( 'X-Redirect-By: Yoast SEO' );

This comment has been minimized.

@andizer

andizer May 29, 2018

Contributor

In WordPress 5.0.0 (which isn't released yet) we can pass a third argument for setting this header.

https://core.trac.wordpress.org/browser/trunk/src/wp-includes/pluggable.php#L1205

This header might be overwritten by the default one.

This comment has been minimized.

@jdevalk

jdevalk May 29, 2018

Member

Good call, let's make sure we fix that when 5.0 comes out :D

@jdevalk jdevalk merged commit e16c70a into trunk May 29, 2018

3 of 4 checks passed

codeclimate/diff-coverage 0% (60% threshold)
Details
codeclimate All good!
Details
codeclimate/total-coverage 30% (0.0% change)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@andizer andizer added this to the 7.7 milestone May 30, 2018

@moorscode moorscode deleted the stories/add-x-redirect-by branch May 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.