-
Notifications
You must be signed in to change notification settings - Fork 322
Guard Instant Results facets against invalid taxonomy values #4255
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
base: develop
Are you sure you want to change the base?
Guard Instant Results facets against invalid taxonomy values #4255
Conversation
felipeelia
left a 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.
@laraib15 thanks for the PR! Do you mind addressing my comment there so we can merge it in 5.3.3?
| if ( ! ( $taxonomy instanceof \WP_Taxonomy ) ) { | ||
| continue; | ||
| } |
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.
Instead of silently continuing here, let's add a _doing_it_wrong() call, so developers can know something is going wrong. You can follow what we do here.
It would be awesome if we could also add a unit test for this method (and this _doing_it_wrong call) to the tests/php/features/TestInstantResults.php file.
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.
Hi! I’ve implemented the requested change.
Added a _doing_it_wrong() call when ep_facet_include_taxonomies returns an invalid taxonomy (includes the taxonomy slug in the message; version set to ElasticPress 5.3.2).
Added a unit test in tests/php/features/TestInstantResults.php
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, @laraib15! I have linters running now, but I can already say some indentations there will need to be fixed (you can see the misalignment when you check your code here on GitHub)
Also, the next version is actually 5.3.3 and not 5.3.2, as you added. Mind making those changes, please? (It is worth waiting for GitHub Actions to run, just in case there is something else.)
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.
@felipeelia i have pushed the fixes. Please take a look when you get a chance.
Description of the Change
This PR prevents a fatal error in Instant Results taxonomy facets when the
ep_facet_include_taxonomiesfilter returns unexpected values (e.g., a taxonomy slug string such asinternal_tagsrather than aWP_Taxonomyobject).InstantResults::get_facets()currently assumes each filtered taxonomy entry is aWP_Taxonomyobject and passes it directly toget_taxonomy_labels(). If a third-party integration appends a slug string, WordPress core fatals when it tries to treat a string as an object (e.g.,Attempt to assign property "labels" on stringinwp-includes/taxonomy.php).This change adds a defensive guard in the taxonomy facets loop:
get_taxonomy( $slug )WP_Taxonomy, skip itThis prevents wp-admin lockouts while preserving existing behavior for correctly shaped data.
Closes #4254
How to test the Change
internal_tagsas a string viaep_facet_include_taxonomies)./wp-admin//wp-admin/admin.php?page=elasticpressVerified on: WordPress 6.9; PHP 8.4.
Changelog Entry
Credits
Props @laraib15
Checklist: