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

wpr-beacon.min.js is added even if DONOTROCKETOPTIMIZE is set to true #6972

Closed
alfonso100 opened this issue Sep 12, 2024 · 6 comments · Fixed by #7022 or #7045 · May be fixed by #7037
Closed

wpr-beacon.min.js is added even if DONOTROCKETOPTIMIZE is set to true #6972

alfonso100 opened this issue Sep 12, 2024 · 6 comments · Fixed by #7022 or #7045 · May be fixed by #7037
Assignees
Labels
effort: [XS] < 1 day of estimated development time type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@alfonso100
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:
Yes - Made sure you’re on the latest version
Yes - Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
wpr-beacon.min.js is added even if DONOTROCKETOPTIMIZE is set to true. So when you visit a page, the script is added, the detection triggers, and the entry is added into the wpr_above_the_fold table.
 
OCI will only obey the filter: add_filter( 'rocket_above_the_fold_optimization', '__return_false' );

To Reproduce
Steps to reproduce the behavior:

  1. add define('DONOTROCKETOPTIMIZE', true);
  2. Visit a page in incognito
  3. See the beacon script injected
  4. See the entry being added to the database

Expected behavior
DONOTROCKETOPTIMIZE should stop all the optimizations including Optimize Critical Images.
And maybe Lazy Render Content in 3.17. That's the logic for other optimizations

Screenshots
3K7V2lP

https://i.imgur.com/3K7V2lP.png

Additional context
related slack thread: https://wp-media.slack.com/archives/C43T1AYMQ/p1726159393548059

Acceptance Criteria (for WP Media team use only)
Clear instructions for developers, to be added before the grooming

@piotrbak piotrbak added this to the 3.17.1 milestone Sep 22, 2024
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement needs: grooming noQA labels Sep 26, 2024
@piotrbak piotrbak modified the milestones: 3.17.1, 3.17.2 Oct 6, 2024
@Miraeld Miraeld self-assigned this Oct 7, 2024
@Miraeld
Copy link
Contributor

Miraeld commented Oct 7, 2024

Reproduce the issue:

  1. Add define('DONOTROCKETOPTIMIZE', true); to your wp-config.php.
  2. Visit a page in incognito mode.
  3. Observe that the beacon script is injected.
  4. Check the database to see the entry added to the wpr_above_the_fold table.

Identify the root cause

The issue is that the wpr-beacon.min.js script is being added even when DONOTROCKETOPTIMIZE is set to true. This causes the detection to trigger and an entry to be added to the wpr_above_the_fold table.

Scope a solution

To solve the issue, we need to modify the inject_beacon function in Processor.php to check for the DONOTROCKETOPTIMIZE constant and skip the injection if it is defined.

Code Changes

Modify Processor.php

  1. Check for DONOTROCKETOPTIMIZE constant:
    • Add a condition to skip the beacon injection if DONOTROCKETOPTIMIZE is defined.
private function inject_beacon( $html, $url, $is_mobile ): string {
    if ( defined( 'DONOTROCKETOPTIMIZE' ) && DONOTROCKETOPTIMIZE ) {
        return $html;
    }

    $min = ( defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ) ? '' : '.min';

    if ( ! $this->filesystem->exists( rocket_get_constant( 'WP_ROCKET_ASSETS_JS_PATH' ) . 'wpr-beacon' . $min . '.js' ) ) {
        return $html;
    }

    $default_width_threshold  = $is_mobile ? 393 : 1600;
    $default_height_threshold = $is_mobile ? 830 : 700;

    $width_threshold = rocket_apply_filter_and_deprecated(
        'rocket_performance_hints_optimization_width_threshold',
        [ $default_width_threshold, $is_mobile, $url ],
        '3.16.4',
        'rocket_lcp_width_threshold'
    );

    $height_threshold = rocket_apply_filter_and_deprecated(
        'rocket_performance_hints_optimization_height_threshold',
        [ $default_height_threshold, $is_mobile, $url ],
        '3.16.4',
        'rocket_lcp_height_threshold'
    );

    if ( ! is_int( $width_threshold ) ) {
        $width_threshold = $default_width_threshold;
    }

    if ( ! is_int( $height_threshold ) ) {
        $height_threshold = $default_height_threshold;
    }

    $default_delay = 500;

    $delay = rocket_apply_filter_and_deprecated(
        'rocket_performance_hints_optimization_delay',
        [ $default_delay, $is_mobile, $url ],
        '3.16.4',
        'rocket_lcp_delay'
    );

    if ( ! is_int( $delay ) ) {
        $delay = $default_delay;
    }

    $data = [
        'url'             => $url,
        'is_mobile'       => $is_mobile,
        'width_threshold' => $width_threshold,
        'height_threshold' => $height_threshold,
        'delay'           => $delay,
        'nonce'           => wp_create_nonce( 'rocket_beacon' ),
        'ajax_url'        => admin_url( 'admin-ajax.php' ),
        'status'          => [
            'atf' => true,
            'lrc' => true,
        ],
    ];

    $data_modified = null;
    foreach ( $this->factories as $factory ) {
        $data_modified = $factory->get_frontend_controller()->modify_beacon_data( $data );
    }

    $inline_script = '<script>var rocket_beacon_data = ' . wp_json_encode( $data_modified ) . '</script>';

    $script_url = rocket_get_constant( 'WP_ROCKET_ASSETS_JS_URL' ) . 'wpr-beacon' . $min . '.js';

    $script_tag = "<script data-name=\"wpr-wpr-beacon\" src='{$script_url}' async></script>"; // phpcs:ignore WordPress.WP.EnqueuedResources.NonEnqueuedScript

    return str_replace( '</body>', $inline_script . $script_tag . '</body>', $html );
}

Development steps:

  • Modify the inject_beacon function in Processor.php to check for the DONOTROCKETOPTIMIZE constant.
  • Ensure the beacon script is not injected when DONOTROCKETOPTIMIZE is set to true.

Effort estimation:

XS

Is a refactor needed in that part of the codebase?

No

@Miraeld Miraeld removed their assignment Oct 7, 2024
@wordpressfan
Copy link
Contributor

I want to relate between this one and the PR: #6998

Checking something before the final confirmation.

@remyperona
Copy link
Contributor

One comment: you can use rocket_get_constant() to check for the constant definition and value, and it makes unit testing easier.

@MathieuLamiot
Copy link
Contributor

Re-opening following this founding: https://wp-media.slack.com/archives/CUT7FLHF1/p1729070587176049
I'd suggest we:

  • Revert the merge to unlock develop branch
  • Consider this fix
  • Would there be a way to capture this issue in an integration test?
  • Re-create a PR.

@Khadreal Khadreal self-assigned this Oct 18, 2024
@Mai-Saad Mai-Saad self-assigned this Oct 21, 2024
@Mai-Saad
Copy link
Contributor

@Mai-Saad Mai-Saad removed their assignment Oct 21, 2024
@remyperona remyperona added effort: [XS] < 1 day of estimated development time and removed needs: grooming labels Oct 22, 2024
@MathieuLamiot MathieuLamiot added this to the 3.17.3 milestone Oct 23, 2024
@Miraeld Miraeld self-assigned this Oct 24, 2024
@bobhuf
Copy link

bobhuf commented Oct 24, 2024

data-wpr-lazyrender keeps breaking my websites. setting rocket_above_the_fold_optimization and DONOTROCKETOPTIMIZE do not work. Hot fix please!?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment