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

Refactor the part of SaaS visits optimizations #7299

Open
wordpressfan opened this issue Feb 10, 2025 · 5 comments
Open

Refactor the part of SaaS visits optimizations #7299

wordpressfan opened this issue Feb 10, 2025 · 5 comments
Labels
effort: [S] 1-2 days of estimated development time

Comments

@wordpressfan
Copy link
Contributor

Is your feature request related to a problem? Please describe.
That's a dev initiative, currently we detect the SaaS visit using a query string called wpr_imagedimensions or wpr_lazyrendercontent but we needed a proper way to detect the SaaS visits for warmup or doing SaaS optimizations.

This is the plugin part of this issue:
https://github.com/wp-media/nodejs-treeshaker/issues/810

Describe the solution you'd like
Currently in SaaS we implemented a way to identify that the current visit is from SaaS not a real visit by adding a header called Wpr-Opt-List which has the values rucss (we need to set a standard here with @mostafa-hisham so we all be aligned on the names here)

@mostafa-hisham told me that the values of this header is coming from a config we should send with the request named optimization_list so we need to make sure that we correctly send this config.

So we will create a new buffer based on this header in the request exactly like we did here:

public function start_performance_hints_buffer() {
if ( ! isset( $_GET['wpr_imagedimensions'] ) && ! isset( $_GET['wpr_lazyrendercontent'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
return;
}
if ( ! $this->buffer_tests->can_process_any_buffer() ) {
return;
}
ob_start( [ $this, 'performance_hints_buffer' ] );
}
/**
* Update images that have no width/height with real dimensions for the SaaS
*
* @param string $buffer Page HTML content.
*
* @return string Page HTML content after update.
*/
public function performance_hints_buffer( $buffer ) {
/**
* Filters the buffer content for performance hints.
*
* @since 3.17
*
* @param $buffer Page HTML content.
*/
return wpm_apply_filters_typed( 'string', 'rocket_performance_hints_buffer', $buffer );
}

Then with each feature that we need to apply optimizations specifically with SaaS visits, we will need to use this filter after checking the value of the header, so let's take an example:

Header Wpr-Opt-List has the value rucss, lrc
So if we want to target rucss visits from SaaS we will check if the header contains the value of rucss then add anything to the buffer HTML and so on.

We need here to mention all the features that needs to be refactored, I'll mention what I'm sure about but feel free to update it with grooming:

  1. Performance hints buffer
  2. Apply Image dimensions for here I think this is also related to performance hints.
@Khadreal
Copy link
Contributor

Khadreal commented Feb 25, 2025

Do we need to include the feature value as part of the body when we send the url to SaaS ?

@wordpressfan
Copy link
Contributor Author

@Khadreal let's think about it, I think it'd be better to send the value of the feature because we will get it with the response and we may use it (or may not).

If it's hard or will take much time, we can send a default value for this first phase and change this if needed.

what do u think?

@Khadreal
Copy link
Contributor

Khadreal commented Feb 25, 2025

We can create a function to get the header when SaaS visit, somehting like this

public function get_saas_request_header( string $feature ) {
      $headers = getAllHeaders()
      return isset($headers[$header_name]) ? $headers[$header_name] : null; 
}

In this file we would include the feature available(probably use filter to get the features that's enabled) and send as array (eg ['lrc','lcp','rucss']) so the optimization_list include the features.

In this section

if ( ! isset( $_GET['wpr_imagedimensions'] ) && isset( $_GET['wpr_lazyrendercontent'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended

instead of checking for get request here, we would pass the feature name to the function created above to check if it's part of the header if( ! $this->get_saas_request_header('lrc') && ! $this->get_saas_request_header('lcp') {...}
Change the get request here
if ( ! isset( $_GET['wpr_imagedimensions'] ) && ! isset( $_GET['wpr_lazyrendercontent'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended

Update test

Estimation
[S]

@Khadreal Khadreal added the effort: [S] 1-2 days of estimated development time label Feb 25, 2025
@Miraeld
Copy link
Contributor

Miraeld commented Feb 26, 2025

LGTM

@wordpressfan
Copy link
Contributor Author

I just have one concern about a case here:
We have RUCSS enabled and lrc disabled so we sent a request to the SaaS Queue and before the SaaS visit we changed the lrc to be enabled, in this case the header will not contain lrc so the functionality won't happen, correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time
Projects
None yet
Development

No branches or pull requests

3 participants