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

Use author ID instead of nicename for author archive queries #46

Closed
wants to merge 8 commits into from
Closed

Use author ID instead of nicename for author archive queries #46

wants to merge 8 commits into from

Conversation

kasparsd
Copy link
Contributor

@kasparsd kasparsd commented Dec 15, 2016

Ready for code review.

WordPress core maps author_name to the user ID for the author archive queries.

We are having an issue on VIP (ZD ticket 60751) where author archives http://example.com/author/username which rewrite to http://example.com/?author_name=username are returning a 404. It appears that VIP doesn't have a field for post_author.user_nicename in the ES index.

Maybe also related to #43.

@mboynes
Copy link
Contributor

mboynes commented Dec 15, 2016

Great catch and thanks for the PR! I would rather fix this in the VIP adapter instead of the plugin's core class. If the adapter has post_author.user_nicename available, it would potentially save a database query. Any objections to going that route?

@kasparsd
Copy link
Contributor Author

@mboynes From what I understand, VIP doesn't have post_author.user_nicename available and indexed even though the comment says it should be. We would need somebody from VIP to confirm this.

@kasparsd
Copy link
Contributor Author

@mboynes Or did you mean adding this logic to the query_es() method of the VIP adapter?

@mboynes
Copy link
Contributor

mboynes commented Dec 15, 2016

I meant adding it to the query_es() method or adding a new method which leverages one of the actions/filters in the query wrapper. Of course, if VIP has the field available and it's just wrong in the adapter, that would be even easier!

@gibrown
Copy link

gibrown commented Dec 16, 2016

Ya, we don't have that field. See: https://developer.wordpress.com/docs/elasticsearch/post-doc-schema/

@kasparsd
Copy link
Contributor Author

@mboynes I just added the user ID lookup to the VIP adapter.

if ( function_exists( 'es_api_search_index' ) ) {
return es_api_search_index( $es_args, 'es-wp-query' );
if ( ! function_exists( 'es_api_search_index' ) ) {
return new WP_Error( 'vip-es-api-missing', 'Missing es_api_search_index method' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mboynes Previously it was failing silently. I noticed that set_posts() checks for is_wp_error() so this shouldn't introduce any regression.

* @see https://developer.wordpress.com/docs/elasticsearch/post-doc-schema/
*/
function vip_es_query_filter( $filter ) {
foreach ( $filter as $filter_no => $filter_rule ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mboynes I really don't like the way this looks so please let me know if you think there is a better way to approach this.

I was considering adding a new filter to the output of es_map() but I was worried of how many times it will be called during a single query and the potential performance issues because of that.

Secondly, I don't know what happens when we combine this with author__in and author__not_in filters. @gibrown Do you have any feedback on that?

@kasparsd
Copy link
Contributor Author

Replaced by #47.

@kasparsd kasparsd closed this Dec 21, 2016
@kasparsd kasparsd deleted the bugfix/use-author-id branch December 21, 2016 17:00
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