-
Notifications
You must be signed in to change notification settings - Fork 165
Incorporate page state into ETag computation #1722
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
Changes from 1 commit
60740a8
2239317
3c7842b
3c57a1d
6a5e421
cc8e252
72f4e54
8651d05
f66012e
56cbe8e
38d0097
d61a8bc
072140b
4ffb79f
1794cff
3c3a1a3
65dc803
1c0bdac
e562726
6d80095
73e4519
59879f0
df306e1
c9c0dd8
4632350
5b4e791
1073780
9a19113
03d1683
8325cb4
1b3ba00
a71fe76
03ef8c8
73edbdb
55d56d3
63bb613
18e0476
57a0ba6
804d71e
8fe2f8f
1869689
7c0cfe1
1fc20af
b083a29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Signed-off-by: Shyamsundar Gadde <shyamsundar.gadde@rtcamp.com>
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,17 +158,23 @@ function od_get_url_metrics_slug( array $query_vars ): string { | |
| * @return non-empty-string Current ETag. | ||
| */ | ||
| function od_get_current_url_metrics_etag( OD_Tag_Visitor_Registry $tag_visitor_registry ): string { | ||
| $data = array( | ||
| 'tag_visitors' => array_keys( iterator_to_array( $tag_visitor_registry ) ), | ||
| 'queried_posts' => array_map( | ||
| if ( isset( $GLOBALS['wp_the_query']->posts ) && is_array( $GLOBALS['wp_the_query']->posts ) ) { | ||
| $queried_posts = array_map( | ||
| static function ( WP_Post $post ): array { | ||
| return array( | ||
| 'ID' => $post->ID, | ||
| 'last_modified' => $post->post_modified_gmt, | ||
| ); | ||
| }, | ||
| $GLOBALS['wp_the_query']->posts | ||
| ), | ||
| ); | ||
| } else { | ||
| $queried_posts = array(); | ||
| } | ||
|
|
||
| $data = array( | ||
| 'tag_visitors' => array_keys( iterator_to_array( $tag_visitor_registry ) ), | ||
| 'queried_posts' => $queried_posts, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I considered using separate keys like Would separate keys be more appropriate instead?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The queried object ID may not be a post at all, however. It may be a Additionally, in Reading settings when you select a static homepage and designate a page as the "posts page" I don't believe this page is included in the posts loop array.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the approach to collecting IDs, I referenced the issue description, which suggested something like this: if ( is_singular() ) {
$normalized_query_vars['queried_object_id'] = get_queried_object_id();
} else {
$normalized_query_vars['queried_post_ids'] = join( ',', wp_list_pluck( $GLOBALS['wp_query']->posts, 'ID' ) );
}Based on this, my understanding was that whenever In the Regarding static homepages, I observed the following:
Given this, I didn’t see a need to include the static page’s ID or modification date in the ETag data for the "Posts Page" since its content is not rendered and is essentially overridden by the home template displaying the posts. Are there additional considerations or scenarios I may not have accounted for?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Actually, why wouldn't the Posts Page content not be rendered? I mean, some aspect of the posts page will be rendered. At the very least, the title of the Posts Page is displayed: https://stimulating-mosquito-343fdf.instawp.xyz/the-bloooooooooog/ With Full Site Editing, the I think there should be two keys then, one for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, and even a classic theme template may decide to show the featured image for the Posts Page. For example, Twenty Seventeen has this header logic: if ( twentyseventeen_should_show_featured_image() ) :
echo '<div class="single-featured-image-header">';
echo get_the_post_thumbnail( get_queried_object_id(), 'twentyseventeen-featured-image' );
echo '</div><!-- .single-featured-image-header -->';
endif;And you can opt-in to showing the featured image with plugin code like this: add_filter( 'twentyseventeen_should_show_featured_image', '__return_false' );Example: https://stimulating-mosquito-343fdf.instawp.xyz/the-bloooooooooog/ But any other theme may decide to show the featured image by default in this case. There's no telling with WordPress 😄 So yeah, I think all of the information about the queried object should be stored in addition to whatever is stored for the posts in the loop, even perhaps if this means some data is duplicated. It won't matter since it all ends up as a hash anyway.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah, that's a great point! I was testing with the Twenty Twenty-Five theme without much modification, so none of the page content seemed to be used—clearly, that was a gap in my testing. 😅
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a key for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would it make sense to also account for the template content when generating the ETag? While global styles might only have an indirect impact on URL metrics, the template itself seems directly relevant since it determines the layout and rendered content. For block themes, instead of just considering the template slug, perhaps we should also include the template's modified date? One way to implement this might be: $data['current_template'] = get_block_template( $GLOBALS['_wp_current_template_id'], 'wp_template' );
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Including the modified date makes sense to me. |
||
| 'active_theme' => array( | ||
| 'template' => get_template(), | ||
| 'template_version' => wp_get_theme( get_template() )->get( 'Version' ), | ||
|
|
@@ -179,9 +185,9 @@ static function ( WP_Post $post ): array { | |
|
|
||
| if ( wp_is_block_theme() ) { | ||
|
ShyamGadde marked this conversation as resolved.
Outdated
|
||
| // Extract the template slug from $_wp_current_template_id, which has the format 'theme_slug//template_slug'. | ||
| $data['current_template'] = explode( '//', $GLOBALS['_wp_current_template_id'] )[1]; | ||
| $data['current_template'] = explode( '//', $GLOBALS['_wp_current_template_id'] ?? '' )[1] ?? ''; | ||
| } else { | ||
| $data['current_template'] = basename( $GLOBALS['template'] ); | ||
| $data['current_template'] = basename( $GLOBALS['template'] ?? '' ); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Continuing the analogy from the issue description, I thought it might be helpful to include the
last_modifiedfor posts from The Loop as well. This would address cases where a post is updated (e.g., adding a featured image) rather than just when new posts are added.