Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial PHPUnit coverage for the REST layer in OneSearch, focusing on controller route registration, permission-sensitive behavior, and governing/consumer data flows. This fits the codebase by expanding unit coverage around the plugin’s cross-site REST integration points.
Changes:
- Adds new unit test files for the abstract REST controller and the main REST controllers/handler.
- Covers route registration, option reads/writes, brand config retrieval, indexable entities, and reindex behavior.
- Adds permission and origin/header checks for same-origin and cross-origin REST requests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
tests/php/Unit/Modules/Rest/Abstract_REST_ControllerTest.php |
Adds tests for hook registration, API permission checks, and host/origin matching. |
tests/php/Unit/Modules/Rest/Basic_Options_ControllerTest.php |
Adds tests for basic settings REST endpoints such as site type, shared sites, health check, and governing site. |
tests/php/Unit/Modules/Rest/Governing_Data_ControllerTest.php |
Adds tests for brand-config and all-post-types controller behavior across site types. |
tests/php/Unit/Modules/Rest/Governing_Data_HandlerTest.php |
Adds tests for governing-data fetch/caching, child-site post type retrieval, and cache clearing. |
tests/php/Unit/Modules/Rest/Search_ControllerTest.php |
Adds tests for search controller routes, Algolia credentials, indexable entities, and reindex behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param string $host Host to compare. | ||
| * @param int|null $port Optional port. | ||
| */ | ||
| public function test_is_url_from_host( string $url, string $host, ?int $port = null ): bool { |
There was a problem hiding this comment.
Is this function for testing or just grabbing the value from this function? If this is not the test function, then we can rename it to expose_is_url_from_host or something else
There was a problem hiding this comment.
It's just for grabbing the value from the protected is_url_from_host method.
There was a problem hiding this comment.
Then you can rename the function.
| $this->assertTrue( $data['success'] ); | ||
| $this->assertCount( 2, $data['shared_sites'] ); | ||
| $this->assertSame( 'Brand A', $data['shared_sites'][0]['name'] ); | ||
| $this->assertNotSame( 'Brand B', $data['shared_sites'][0]['name'] ); |
There was a problem hiding this comment.
Does this assertion need to be?
There was a problem hiding this comment.
@sabbir1991 are you referring to this?
$this->assertNotSame( 'Brand B', $data['shared_sites'][0]['name'] );| add_filter( 'pre_http_request', $filter, 10, 3 ); | ||
|
|
||
| Governing_Data_Handler::clear_brand_config_cache(); | ||
|
|
||
| remove_filter( 'pre_http_request', $filter ); | ||
|
|
There was a problem hiding this comment.
This is fine, but we can also use try and finally blocks for handling this types of scenario.
| }; | ||
| add_filter( 'pre_http_request', $filter, 10, 3 ); | ||
|
|
||
| Governing_Data_Handler::clear_brand_config_cache( 'https://site-a.example.com/' ); |
There was a problem hiding this comment.
Make sure the the valure stores with trainling slashes before doing the check.
There was a problem hiding this comment.
Could you please clarify this a little? Are you referring to the assert section?
There was a problem hiding this comment.
clear_brand_config_cache( 'https://site-a.example.com/' ) — called with trailing slash, but set_shared_sites stores 'https://site-a.example.com' (no trailing slash). If production does an exact string match, the filter never fires and assertCount( 1, $requested_urls ) will fail for the wrong reason. Normalize URLs to be consistent (both with or both without a trailing slash)
| // Governing with no Algolia: get_post_types_to_index() returns [] (no WP_Error), | ||
| // then index_all_posts() calls delete_by() which fails on missing Algolia | ||
| // credentials, collecting an error entry and returning success: false. | ||
| update_option( Settings::OPTION_SITE_TYPE, Settings::SITE_TYPE_GOVERNING ); |
There was a problem hiding this comment.
You are setting the site type governing but the function name is standalone, do you think it can be test_reindex_governing_retunrs_failure_without_algolia
What
Added REST PHPUnit test cases.
Why
Improves development workflow by enabling faster and more reliable validation of REST functionality, reducing repetitive manual testing efforts.
Related Issue(s):
How
AI Disclosure
Hybrid approach = Written by Me + Copilot (Claude Opus 4.6, GPT 5.3) + Audited by me
Testing Instructions
Screenshots
Additional Info
Checklist