-
Notifications
You must be signed in to change notification settings - Fork 110
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 replacing of images only in frontend context #424
Conversation
@@ -240,3 +240,19 @@ function webp_uploads_get_attachment_sources( $attachment_id, $size = 'thumbnail | |||
// Return an empty array if no sources found. | |||
return array(); | |||
} | |||
|
|||
/** | |||
* Verify if the request if for the frontend context. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be Verify if the request is for the frontend context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akshitsethi This looks like a good start, however some of the conditions need to be refined a bit. In combination with that, some more accurate test coverage would be helpful too.
|
||
$tag = wp_get_attachment_image( $attachment_id, 'medium', false, array( 'class' => "wp-image-{$attachment_id}" ) ); | ||
$this->assertNotSame( $tag, webp_uploads_update_image_references( $tag ) ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add one more test that covers the scenario where you're in a frontend context, but before the user facing content (i.e. template_redirect
has run but wp_head
hasn't). Maybe that could be called it_should_not_replace_image_references_when_in_frontend_context_but_before_body
.
… coverage for the function specifically.
…ement tests to still pass.
@akshitsethi @adamsilverstein I have made the additional changes here around the conditions and enhanced test coverage, made it specific to the new function. @akshitsethi Your previous point on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @akshitsethi!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
||
public function test_webp_uploads_in_frontend_body_without_wp_query() { | ||
unset( $GLOBALS['wp_query'] ); | ||
|
||
$this->assertFalse( webp_uploads_in_frontend_body() ); | ||
} | ||
|
||
public function test_webp_uploads_in_frontend_body_with_feed() { | ||
$this->mock_empty_action( 'template_redirect' ); | ||
$GLOBALS['wp_query']->is_feed = true; | ||
|
||
$this->assertFalse( webp_uploads_in_frontend_body() ); | ||
} | ||
|
||
public function test_webp_uploads_in_frontend_body_without_template_redirect() { | ||
$this->assertFalse( webp_uploads_in_frontend_body() ); | ||
} | ||
|
||
public function test_webp_uploads_in_frontend_body_before_template_redirect() { | ||
$result = webp_uploads_in_frontend_body(); | ||
$this->mock_empty_action( 'template_redirect' ); | ||
|
||
$this->assertFalse( $result ); | ||
} | ||
|
||
public function test_webp_uploads_in_frontend_body_after_template_redirect() { | ||
$this->mock_empty_action( 'template_redirect' ); | ||
$result = webp_uploads_in_frontend_body(); | ||
|
||
$this->assertTrue( $result ); | ||
} | ||
|
||
public function test_webp_uploads_in_frontend_body_within_wp_head() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of nit-pick here but the test function names are different than what we actually use the names here like it_should
or it_
like that. And these functions doesn't have function doc comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mehulkaklotar I didn't think it was too critical to point it out in earlier tests for the plugin, but the convention for WordPress core tests is to always use test_
followed by the function name, then optionally anything else. So I stuck with that here.
I would advise to follow that convention for future tests to monitor the core plan.
Regarding doc comments, test methods do not require any doc comments per core specifications - only when helpful. Preferably, the name of the method should be clear enough to know what is going on, or when it makes sense to clarify specific code, inline comments should be used.
cc @akshitsethi @adamsilverstein @mukeshpanchal27 @jjgrainger @eugene-manuilov
Summary
Fixes #379
Relevant technical choices
Bails early if the request is not for the frontend context or is coming from a feed url. We also check if the request is made inside the
wp_head
hook so that original images are returned for use within<head>
tags.In-progress:
Checklist
[Focus]
orInfrastructure
label.[Type]
label.no milestone
label.