Skip to content

Boost: Add active modules to speed score request #21471

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.

Sign up for GitHub

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

Merged
karthikax merged 4 commits into from
Oct 25, 2021
Merged

Boost: Add active modules to speed score request #21471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12802ca
Add active modules data to pagespeed request
karthikax Oct 20, 2021
8912078
Add changelog
karthikax Oct 20, 2021
7f28681
Remove active modules parameter for no-boost request
karthikax Oct 22, 2021
a62f877
Merge branch 'master' into update/speed-score-tracking
karthikax Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions projects/plugins/boost/app/lib/class-speed-score-request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ class Speed_Score_Request extends Cacheable {
*/
private $url;

/**
* Active Jetpack Boost modules.
*
* @var array $active_modules Active modules.
*/
private $active_modules;

@pyronaur pyronaur Oct 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The constructor already does this, but perhaps worth adding that here as well - private $active_modules = [] to be clear and ensure that the default value is an empty array.

@karthikax karthikax Oct 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since 3 other properties do not declare the default properties, I think not declaring the default value will have better code consistency
Also, as you mentioned, the constructor already does this and the assigned default value will never be used before that, I don't think this is needed.
Screenshot 2021-10-25 at 12 36 59 PM

@pyronaur pyronaur Oct 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sounds good! 👍


/**
* When the Speed Scores request was created, in seconds since epoch.
*
Expand All @@ -50,17 +57,19 @@ class Speed_Score_Request extends Cacheable {
* Constructor.
*
* @param string $url The URL to get the Speed Scores for.
* @param array $active_modules Active modules.
* @param null $created When the Speed Scores request was created, in seconds since epoch.
* @param string $status Status of the Speed Scores request.
* @param null $error The Speed Scores error.
*/
public function __construct( $url, $created = null, $status = 'pending', $error = null ) {
public function __construct( $url, $active_modules = array(), $created = null, $status = 'pending', $error = null ) {
$this->set_cache_id( self::generate_cache_id_from_url( $url ) );

$this->url = $url;
$this->created = is_null( $created ) ? microtime( true ) : $created;
$this->status = $status;
$this->error = $error;
$this->url = $url;
$this->active_modules = $active_modules;
$this->created = is_null( $created ) ? microtime( true ) : $created;
$this->status = $status;
$this->error = $error;
}

/**
Expand All @@ -81,11 +90,12 @@ public static function generate_cache_id_from_url( $url ) {
*/
public function jsonSerialize() {
return array(
'id' => $this->get_cache_id(),
'url' => $this->url,
'created' => $this->created,
'status' => $this->status,
'error' => $this->error,
'id' => $this->get_cache_id(),
'url' => $this->url,
'active_modules' => $this->active_modules,
'created' => $this->created,
'status' => $this->status,
'error' => $this->error,
);
}

Expand All @@ -99,6 +109,7 @@ public function jsonSerialize() {
public static function jsonUnserialize( $data ) {
$object = new Speed_Score_Request(
$data['url'],
$data['active_modules'],
$data['created'],
$data['status'],
$data['error']
Expand Down Expand Up @@ -133,8 +144,9 @@ public function execute() {
sprintf( '/sites/%d/jetpack-boost/speed-scores', $blog_id ),
null,
array(
'request_id' => $this->get_cache_id(),
'url' => Url::normalize( $this->url ),
'request_id' => $this->get_cache_id(),
'url' => Url::normalize( $this->url ),
'active_modules' => $this->active_modules,
)
);

Expand Down
3 changes: 2 additions & 1 deletion projects/plugins/boost/app/lib/class-speed-score.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ public function dispatch_speed_score_request( $request ) {
$score_request = $this->get_score_request_by_url( $url );
if ( empty( $score_request ) || ! $score_request->is_pending() ) {
// Create and store the Speed Score request.
$score_request = new Speed_Score_Request( $url );
$active_modules = array_keys( $this->jetpack_boost->get_active_modules() );
$score_request = new Speed_Score_Request( $url, $active_modules );
$score_request->store( 1800 ); // Keep the request for 30 minutes even if no one access the results.

// Send the request.
Expand Down
4 changes: 4 additions & 0 deletions projects/plugins/boost/changelog/add-active-modules-pagespeed-request
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Added active modules data with page speed score request.