Skip to content

Introduce AVIF header health check #1612

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Oct 28, 2024
Merged

Introduce AVIF header health check #1612

merged 15 commits into from
Oct 28, 2024

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Oct 21, 2024 •
edited
Loading

Summary

Add a health check to verify that AVIF images are served correctly.

Fixes #1263

Relevant technical choices

Currently this offers miinimal suggestions for how to fix the issue.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 21, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: westonruter <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: swissspidy <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@adamsilverstein adamsilverstein added [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature labels Oct 21, 2024
@swissspidy
Copy link
Member

Great idea! Definitely re-using this for https://github.com/swissspidy/media-experiments to check the content type of WASM files.

@swissspidy swissspidy added this to the performance-lab n.e.x.t milestone Oct 21, 2024
@swissspidy swissspidy mentioned this pull request Oct 21, 2024
Open
westonruter
westonruter reviewed Oct 21, 2024
View reviewed changes
plugins/performance-lab/includes/site-health/avif-headers/helper.php
*/
function avif_headers_check_avif_headers_test(): array {
$result = array(
'label' => __( 'Your site sends AVIF image headers', 'performance-lab' ),
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember the specifics of escaping fields like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will match other instances

Copy link
Member Author

Choose a reason for hiding this comment

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

Escaping for labels is only included in plugins/performance-lab/tests/data/class-site-health-mock-responses.php, maybe we should remove it there.

mukeshpanchal27
mukeshpanchal27 reviewed Oct 22, 2024
View reviewed changes
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Left some feedbacks.

Does it possible that Site health shows Your site does not support AVIF as failed but Your site sends AVIF image headers return headers 🤔

@adamsilverstein
Copy link
Member Author

Does it possible that Site health shows Your site does not support AVIF as failed but Your site sends AVIF image headers return headers 🤔

@mukeshpanchal27 yes, it is possible - AVIF server support and AVIF file header support are two very separate things.

@swissspidy
Copy link
Member

Does it possible that Site health shows Your site does not support AVIF as failed but Your site sends AVIF image headers return headers 🤔

@mukeshpanchal27 yes, it is possible - AVIF server support and AVIF file header support are two very separate things.

Begs the question: is it worth adding this health check if the server doesn't support AVIF files anyway?

@mukeshpanchal27
Copy link
Member

Begs the question: is it worth adding this health check if the server doesn't support AVIF files anyway?

That's my next question. Thanks @swissspidy for asking.

@adamsilverstein
Copy link
Member Author

Begs the question: is it worth adding this health check if the server doesn't support AVIF files anyway?

I thought about that and I'd say yes - one reason is you could potentially migrate a site with AVIF images to a new server where they still need to get served. Also, AVIF files could be included in the theme even if not supported by the server for uploads, eg as part of a template.

@mukeshpanchal27 mukeshpanchal27 merged commit 692a9c5 into WordPress:trunk Oct 28, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westonruter westonruter westonruter approved these changes

@mukeshpanchal27 mukeshpanchal27 mukeshpanchal27 approved these changes

@felixarntz felixarntz Awaiting requested review from felixarntz felixarntz is a code owner

@thelovekesh thelovekesh Awaiting requested review from thelovekesh thelovekesh is a code owner

@swissspidy swissspidy Awaiting requested review from swissspidy

Assignees
No one assigned
Labels
[Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Milestone
performance-lab 3.6.0
Development

Successfully merging this pull request may close these issues.

Site Health: detect web server AVIF mime type support
4 participants
@adamsilverstein @swissspidy @mukeshpanchal27 @westonruter