SIT-2911: Return empty post list from Sophi response #183

oscarssanchez · 2022-02-08T17:58:40Z

Description of the Change

This PR fixes the response coming from Sophi when it returns an empty post list. Previously, the response was ignored and WP_Query's default was used instead.

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests passed.

Applicable Issues

Changelog Entry

barryceelen · 2022-02-08T18:24:27Z

includes/classes/SiteAutomation/Integration.php

+			return $query->num_posts;
+		}
+
+		return $found_posts;


@oscarssanchez wondering why you're taking the route of adding a filter?

Adding a filter is perhaps the "more correct" way to update found_posts, at the same time I'm thinking this filter runs on every query. Perhaps just flat out setting it directly in stead of the hop skip & jump of the $query->num_posts value would be more straightforward. Jetpack doesn't seem to mind doing that (if that's anything to go by...)

Also, and perhaps I'm not reading this correctly, but if someone decides to use the sophi_curated_post_list filter, they might need to override this filter as well?

Hi @barryceelen ,

I believe this will be overwritten as there's some places where set_found_posts() is called after posts_pre_query. I ran a few tests and it seems to be the case.

If someone uses the sophi_curated_post_list, since $query->num_posts has been defined as well, developers can alter its value so that it later replaces $query->found_posts

includes/classes/SiteAutomation/Integration.php

barryceelen · 2022-02-08T18:50:05Z

includes/classes/SiteAutomation/Request.php

@@ -81,7 +81,7 @@ public function get( $page, $widget ) {
 		$this->status = $this->get_status();
 		$site_automation_data = get_option( "sophi_site_automation_data_{$page}_{$widget}" );

-		if ( ! empty( $this->status['success'] ) && $site_automation_data ) {
+		if ( ! empty( $this->status['success'] ) && $site_automation_data && ( defined( 'SOPHI_BYPASS_CACHE' ) && ! SOPHI_BYPASS_CACHE ) ) {


SOPHI_BYPASS_CACHE doesn't bypass data completely: if is_wp_error( $response ) it will still return $site_automation_data on line 101 if that is available. To prevent that from happening, perhaps first define $site_automation_data = false and then fetch using get_option(...) only if ! SOPHI_BYPASS_CACHE?

$site_automation_data = false; if ( ! defined( 'SOPHI_BYPASS_CACHE' ) || ! SOPHI_BYPASS_CACHE ) { $site_automation_data = get_option( "sophi_site_automation_data_{$page}_{$widget}" ); } if ( $site_automation_data && ! empty( $this->status['success'] ) ) { return $site_automation_data; }

Good call @barryceelen I've adjusted here: 749241e

Yay!

Perhaps nitpicking: While the approach here works for the immediate need of bypassing cache in this function, from an architectural point of view I would expect setting a global variable to disable cache would also prevent the plugin from creating those caches and as well as retrieving them.

One fairly straightforward approach could be to introduce something along the lines of sophi_cache_set( .... ) and sophi_cache_get( ... ) functions, in stead of directly using the get_option() and set_option() (or ..transient() ) functions like the plugin currently does, and check the bypass cache global in those functions in stead. (That would also provide a nice point to introduce a filter that could decide to disable cache for a specific page/widget combination, or all of them, in stead of using a global that always disables all of them) @jeffpaul for thoughts.

I do agree that would be nice to have, though wondering how would it fit with the schedule and if leaving this for now would have a huge impact in the future for people using that constant if we go the filter approach as well @felipeelia

felipeelia · 2022-02-10T12:07:08Z

includes/classes/SiteAutomation/Integration.php

+		 * @param {string} $query_vars['sophi_curated_page'] Sophi curated page param.
+		 * @param {string} $query_vars['sophi_curated_widget'] Sophi curated widget param.
+		 * @param {array}  $request_status The request status, whether it was successful or not.
+		 * @param {object} $query Original query.


Not a blocker @oscarssanchez but as we know the type of object here, it would be good to explicitly state @param {WP_Query}.

Fixed here 489f019

felipeelia · 2022-02-10T12:12:27Z

includes/classes/SiteAutomation/Integration.php


-		// Determine how we should format the results  based on the fields parameter.
-		$fields = $query->get( 'fields', '' );
+		if ( $request_status['success'] ) {


As it seems we are not guaranteeing that $this->request->get_status() will return an array nor if that array will contain a 'success' index, it would be to check if ! empty( $request_status['success'] ). What do you think @oscarssanchez? Otherwise, I think people can get a not defined warning.

Thanks @felipeelia I agree. Resolved here fa6b527

felipeelia · 2022-02-10T12:14:00Z

includes/classes/SiteAutomation/Request.php

@@ -79,9 +79,13 @@ public function get( $page, $widget ) {
 		$this->api_url = $this->set_api_url( $page, $widget );

 		$this->status = $this->get_status();
-		$site_automation_data = get_option( "sophi_site_automation_data_{$page}_{$widget}" );
+		$site_automation_data = false;


Currently, the return type is set to array|null, it seems it should be array|bool. Also, would it make sense to also have a third parameter to this function to skip cache or not instead of relying solely on a constant that will affect the behavior every time?

Fixed return type here 4ca47c7 @felipeelia

…bypass

sonarqubecloud · 2022-02-10T18:10:20Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

barryceelen · 2022-02-15T09:20:48Z

includes/classes/SiteAutomation/Request.php

+		 *
+		 * @param {bool} $bypass_cache True or false.
+		 * @param {bool} $page Page name.
+		 * @param {bool} $widget Widget name.


@oscarssanchez the last two parameters are specified as bool, they should be string. (Nitpicking: parameter descriptions should be aligned)

Also, correct me if I'm wrong, I believe phpdoc does not require your parameter type inside of { ...} curly brackets (that's a jsdoc convention).

(The same goes for the 'sophi_curated_post_list' filter doc in Integration.php)

jeffpaul · 2022-02-16T18:57:14Z

Relates to https://voltron.atlassian.net/servicedesk/customer/portal/41/SIT-2911

Return empty post list from Sophi response

73b475e

oscarssanchez requested a review from felipeelia February 8, 2022 17:59

Remove parenthesis

873af13

barryceelen reviewed Feb 8, 2022

View reviewed changes

includes/classes/SiteAutomation/Integration.php Outdated Show resolved Hide resolved

barryceelen reviewed Feb 8, 2022

View reviewed changes

jeffpaul added this to the 1.0.9 milestone Feb 8, 2022

jeffpaul assigned oscarssanchez Feb 8, 2022

oscarssanchez added 3 commits February 8, 2022 16:39

Move sophi_curated_post_list

8aa65f6

Set site_automation_data to false to bypass cache if needed

749241e

Merge branch 'develop' into fix/sophi-post-list-response-empty

c0b80e3

felipeelia reviewed Feb 10, 2022

View reviewed changes

oscarssanchez added 4 commits February 10, 2022 09:24

Check request status is defined and not false

fa6b527

State object type

489f019

Change constant approach and use a filter instead to determine cache …

8b4827a

…bypass

Change return type

4ca47c7

felipeelia approved these changes Feb 10, 2022

View reviewed changes

oscarssanchez merged commit c2afe4f into develop Feb 10, 2022

oscarssanchez deleted the fix/sophi-post-list-response-empty branch February 10, 2022 18:31

barryceelen reviewed Feb 15, 2022

View reviewed changes

jeffpaul changed the title ~~Return empty post list from Sophi response~~ SIT-2911: Return empty post list from Sophi response Feb 18, 2022

jeffpaul mentioned this pull request Jun 14, 2022

SIT-2911: confirm and document resulting plugin behavior on empty Sophi responses #302

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SIT-2911: Return empty post list from Sophi response #183

SIT-2911: Return empty post list from Sophi response #183

oscarssanchez commented Feb 8, 2022

barryceelen Feb 8, 2022 •

edited

Loading

oscarssanchez Feb 8, 2022

barryceelen Feb 8, 2022

oscarssanchez Feb 8, 2022

barryceelen Feb 9, 2022

oscarssanchez Feb 9, 2022

felipeelia Feb 10, 2022

oscarssanchez Feb 10, 2022

felipeelia Feb 10, 2022

oscarssanchez Feb 10, 2022

felipeelia Feb 10, 2022

oscarssanchez Feb 10, 2022

sonarqubecloud bot commented Feb 10, 2022

barryceelen Feb 15, 2022

barryceelen Feb 15, 2022

jeffpaul commented Feb 16, 2022

SIT-2911: Return empty post list from Sophi response #183

SIT-2911: Return empty post list from Sophi response #183

Conversation

oscarssanchez commented Feb 8, 2022

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

Checklist:

Applicable Issues

Changelog Entry

barryceelen Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffpaul commented Feb 16, 2022

barryceelen Feb 8, 2022 •

edited

Loading