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 must instead of should for 'or' relations #2997

Merged
merged 6 commits into from
Oct 12, 2022
Merged

Use must instead of should for 'or' relations #2997

merged 6 commits into from
Oct 12, 2022

Conversation

MARQAS
Copy link
Contributor

@MARQAS MARQAS commented Sep 13, 2022

Description of the Change

This PR will change the condition from using should to must for 'or' relations in meta queries.

Closes #2932

How to test the Change

  • While adding the query to sort the posts using key, it was taking the key inside the should clause, but now it should be under the must clause
  • example:
  'ep_integrate' => true,
  'post_type' => 'event',
  'meta_key' => 'event_start',
  'meta_query' => [
    'relation' =>'OR',
    [
      'key' => 'event_start',
      'value' => date('Ymd'),
      'compare' => '>=',
      'type' => 'NUMERIC',
    ],
    [
      'key' => 'event_end',
      'value' => date('Ymd'),
      'compare' => '>=',
      'type' => 'NUMERIC',
    ],
  ],
  'orderby' => 'meta_value_num',
  'order' => 'ASC',
] 
  • Observe that the key is inside the must clause instead of should clause

Changelog Entry

Changed - The clause for meta_query when sorting by key and using 'or' relation

Credits

Props @MARQAS

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

Copy link
Member

@felipeelia felipeelia left a comment

Choose a reason for hiding this comment

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

@MARQAS although the code you have here might fix the outlying issue, I'm not sure this will actually fix the problem outlined in #2932 without introducing a side-effect.

The problem seems to be coming from this block of code, where we add a new meta query "block" if the 'meta_key' parameter is set in WP_Query.

The problem outlined in #2932 will only happen when the WP_Query object has a meta_key set AND a meta_query using that same meta field inside an OR condition.

We will also need a new PHP Unit test for this one, so it might be helpful to start with the test and adjust the code so it passes.

@felipeelia felipeelia assigned MARQAS and unassigned tott and felipeelia Sep 15, 2022
@felipeelia
Copy link
Member

@MARQAS the code you had before was giving you a false positive: if you switch the first clause to <=, it should return all 3 posts but it returned just 2. In general, every time we have an or in WP it will become a should in Elasticsearch, and that was not happening.
I've pushed a commit changing things a bit and I think we are all set with this one. Do you mind giving it a review and letting me know if you have any questions? Thanks!

@MARQAS
Copy link
Contributor Author

MARQAS commented Oct 12, 2022

@felipeelia Yes, It is working as expected. Thank you!

@felipeelia felipeelia merged commit e053673 into develop Oct 12, 2022
@felipeelia felipeelia deleted the fix/2932 branch October 12, 2022 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants