wp-rocket icon indicating copy to clipboard operation
wp-rocket copied to clipboard

Preconnect exists in the page while it shouldn't or not exist where it should

Open Mai-Saad opened this issue 7 months ago • 1 comments

Describe the bug Scenario1: 1- fresh install and activate 3.19 2- activate self hosted GF and disable preconnect external domain using filter 3- visit page with gf and check source => preconnect is there while it shouldn't (regression)

Scenario2: 1- fresh install and activate 3.19 2- activate self hosted GF (preconnect external is on by default) 3- visit page with gf and check source => preconnect is there while it shouldn't

Scenario3 1- fresh install and activate 3.19 2- activate self hosted GF (preconnect external is on by default) 3- exclude 1 google font from self host using filter 3- visit page with gf and check source => preconnect isn't there while it should 4- clear cache and revisit page => preconnect is there which is good

Expected behavior

  • If all fonts are self-hosted, no preconnect (while preconnect external on/off) (while preload fonts on/off)
  • If there’s an exclusion and some of the fonts are hosted by Google, preconnect needed

Additional context

  • Slack thread https://group-onecom.slack.com/archives/C08F4LZB2QG/p1748248977430439
  • test page https://new.rocketlabsqa.ovh/combine_v1fonts_template-2

Mai-Saad avatar May 26 '25 08:05 Mai-Saad

Scope a solution

Root Cause Analysis

The issue is in the preconnect() method in GoogleFonts/Subscriber.php. Currently, it always adds the fonts.gstatic.com preconnect when Google Fonts optimization is enabled, regardless of whether fonts are actually being loaded from Google's servers.

The method only checks:

  1. is_allowed() - whether Google Fonts optimization is enabled
  2. relation_type === 'preconnect' - the preconnect filter context

It doesn't consider the self-hosted fonts feature or font exclusions.

Proposed Solution

Modify the preconnect() method to intelligently detect when Google Fonts will actually be loaded from Google's servers before adding the preconnect tag.

Implementation approach:

  1. Add a check for the host_fonts_locally option
  2. If self-hosted fonts are enabled, only add preconnect when there are remaining Google-hosted fonts
  3. Create a helper method has_remaining_google_fonts() to detect this condition

Code Changes Required

File: Subscriber.php

  1. Modify the preconnect() method (lines 74-86):
    <?php
public function preconnect( array $urls, $relation_type ) {
    if ( ! $this->is_allowed() ) {
        return $urls;
    }

    if ( 'preconnect' !== $relation_type ) {
        return $urls;
    }

    // NEW: Check if self-hosted fonts is enabled
    if ( $this->options->get( 'host_fonts_locally', 0 ) ) {
        // Only add preconnect if there are still Google-hosted fonts
        if ( ! $this->has_remaining_google_fonts() ) {
            return $urls;
        }
    }

    $urls[] = [
        'href' => 'https://fonts.gstatic.com',
        1      => 'crossorigin',
    ];

    return $urls;
}
  1. Add new private method has_remaining_google_fonts():
<?php
/**
 * Checks if there are Google Fonts that remain hosted by Google (not self-hosted).
 * This can happen when some fonts are excluded from self-hosting via the filter.
 *
 * @return bool True if there are Google-hosted fonts remaining, false otherwise.
 */
private function has_remaining_google_fonts() {
    // Check if Google Fonts combining/processing has found any fonts
    if ( ! $this->combine->has_google_fonts() && ! $this->combine_v2->has_google_fonts() ) {
        return false;
    }

    // Get font URLs that would be processed by the combine classes
    $v1_urls = $this->combine->get_font_urls();
    $v2_urls = $this->combine_v2->get_font_urls();
    
    // If there are combined font URLs, it means there are Google-hosted fonts
    return ! empty( $v1_urls ) || ! empty( $v2_urls );
}

Scenario Coverage

This solution properly handles all reported scenarios:

  • Scenario 1 (All fonts self-hosted): has_remaining_google_fonts() returns false → preconnect skipped ✅
  • Scenario 2 (Cached page, all self-hosted): Same logic as scenario 1 → preconnect skipped ✅
  • Scenario 3 (Some fonts excluded from self-hosting): has_remaining_google_fonts() returns true → preconnect added ✅

Benefits

  • Precise detection: Only adds preconnect when actually needed
  • Handles exclusions: Properly accounts for fonts excluded via rocket_exclude_locally_host_fonts filter
  • Performance improvement: Eliminates unnecessary preconnect when all fonts are self-hosted
  • Backward compatible: No breaking changes to existing functionality
  • Future-proof: Works regardless of how exclusion mechanisms evolve

Testing Considerations

  • Test all three scenarios described in the issue
  • Verify behavior with various exclusion filter configurations
  • Ensure no regression in cases where self-hosted fonts are disabled
  • Test with mixed v1/v2 Google Fonts usage

Miraeld avatar Jun 11 '25 23:06 Miraeld

@Miraeld Do you know if the proposed solution would also apply to scenario 3 in the issue?

jeawhanlee avatar Jul 01 '25 09:07 jeawhanlee

@jeawhanlee It wouldn't have. I've changed the full grooming :)

Miraeld avatar Jul 01 '25 11:07 Miraeld

@Miraeld Did you validate whether has_google_fonts and get_font_urls method would have the expected values at this point if it is used in preconnect which is hooked to the wp_resource_hints?

jeawhanlee avatar Jul 07 '25 08:07 jeawhanlee

@jeawhanlee , I don't know how did you figure that out, but you are right, after looking at it from another angle, that would probably not work properly. 🤔

After some time checking it back to find another solution, I came up with this new grooming:

New solution

File: /inc/Engine/Optimization/GoogleFonts/Subscriber.php

  • Modify the process( $html ) function to include a condition that checks should_remove_preconnect():
	public function process( $html ) {
		if ( ! $this->is_allowed() ) {
			return $html;
		}

		// Combine Google Font API V2.
		$html = $this->combine_v2->optimize( $html );
		// Combine Google Font API V1.
		$html = $this->combine->optimize( $html );

		// Enhanced preconnect removal logic to handle ALL scenarios
		if ( $this->should_remove_preconnect() ) {
			$html = preg_replace( '/<link\s+(?:[^>]+[\s"\'])?href\s*=\s*[\'"]https:\/\/fonts\.gstatic\.com[\'"](?:[^>]+[\s"\'])?\s?\/?>/', '', $html );
		}

		return $html;
	}
  • Creates the function should_remove_preconnect():
/**
	 * Determines if Google Fonts preconnect should be removed.
	 * Handles all scenarios: self-hosted fonts, external domain filters, and font pre-links removal.
	 *
	 * @since 3.19.2
	 * @return bool True if preconnect should be removed, false otherwise.
	 */
	private function should_remove_preconnect() {
		// Scenario 1: All Google Fonts are self-hosted
		if ( ! $this->combine->has_google_fonts() && ! $this->combine_v2->has_google_fonts() ) {
			return true;
		}

		// Scenario 2: Check preconnect external domains feature filters
		if ( $this->is_preconnect_disabled_by_external_domains_feature() ) {
			return true;
		}

		// Scenario 3: Check rocket_remove_font_pre_links filter
		/**
		 * Filters the removal of Google preconnect/prefetch links.
		 *
		 * @since 3.18
		 *
		 * @param bool $enable_removal Enable or disable removal of Google preconnect/prefetch links.
		 */
		$remove_links = wpm_apply_filters_typed( 'boolean', 'rocket_remove_font_pre_links', true );
		if ( ! $remove_links ) {
			return false; // If filter says don't remove, respect that
		}

		return false;
	}
  • Create a function is_preconnect_disabled_by_external_domains_feature():
/**
	 * Checks if preconnect is disabled by the preconnect external domains feature.
	 * This handles the rocket_preconnect_external_domains_optimization filter and exclusions.
	 *
	 * @since 3.19.2
	 * @return bool True if preconnect should be disabled, false otherwise.
	 */
	private function is_preconnect_disabled_by_external_domains_feature() {
		// Check if preconnect external domains feature is disabled globally
		/**
		 * Filters to manage preconnect external domains optimization
		 *
		 * @param bool $allow True to allow, false otherwise.
		 */
		$preconnect_external_domains_enabled = wpm_apply_filters_typed( 'boolean', 'rocket_preconnect_external_domains_optimization', true );
		if ( ! $preconnect_external_domains_enabled ) {
			return true;
		}

		// Check if fonts.gstatic.com is excluded via preconnect external domain exclusions
		/**
		 * Filters the array of elements to be excluded from being processed by the preconnect external domain beacon.
		 *
		 * @since 3.19
		 *
		 * @param string[] $exclusions Array of patterns used to identify elements that should be excluded.
		 */
		$exclusions = wpm_apply_filters_typed( 'string[]', 'preconnect_external_domain_exclusions', [] );
		
		foreach ( $exclusions as $exclusion ) {
			// Check if the exclusion pattern matches fonts.gstatic.com
			if ( strpos( $exclusion, 'fonts.gstatic.com' ) !== false || 
				strpos( $exclusion, 'gstatic.com' ) !== false ||
				preg_match( '#' . preg_quote( $exclusion, '#' ) . '#i', 'https://fonts.gstatic.com' ) ) {
				return true;
			}
		}

		// Check if prefetch is preferred over preconnect for this domain
		/**
		 * Filters whether to use prefetch instead of preconnect for external domains.
		 *
		 * @since 3.19
		 *
		 * @param bool   $use_prefetch Whether to use prefetch instead of preconnect.
		 * @param string $domain       The domain being processed.
		 */
		$use_prefetch = wpm_apply_filters_typed( 'boolean', 'rocket_preconnect_external_domains_use_prefetch', false, 'https://fonts.gstatic.com' );
		if ( $use_prefetch ) {
			return true; // If prefetch is preferred, remove preconnect
		}

		return false;
	}

This new solution should handle all 3 scenarios mentioned in the issue, it respects all existing filters and hooks, clean seperation of the code.

Testing Scenarios:

The solution should be tested with:

  1. Self-hosted fonts only → preconnect removed
  2. rocket_preconnect_external_domains_optimization = false → preconnect removed
  3. preconnect_external_domain_exclusions contains fonts.gstatic.com → preconnect removed
  4. rocket_preconnect_external_domains_use_prefetch = true for fonts.gstatic.com → preconnect removed
  5. rocket_remove_font_pre_links = false → preconnect preserved regardless of other conditions
  6. Mixed scenarios → appropriate logic precedence

What do you think ?

Miraeld avatar Jul 08 '25 06:07 Miraeld

@Miraeld I think we can do something simpler. I have 2 simple options we can choose from

Option 1

In WP_Rocket\Engine\Media\Fonts\Frontend\Controller

  • Update this method to check matching gstatic or googleapis preconnect urls in the $items array and unset them if exist or keep them if $exclusion is not empty.

Option 2

What if we update this method to be:

public function preconnect( array $urls, $relation_type ) {
	if ( ! $this->is_allowed() ) {
		return $urls;
	}

	if ( 'preconnect' !== $relation_type ) {
		return $urls;
	}

        if ( wpm_apply_filters_typed( 'boolean', 'rocket_remove_font_pre_links', false ) ) {
                return $urls;
        }

	$urls[] = [
		'href' => 'https://fonts.gstatic.com',
		1      => 'crossorigin',
	];

	return $urls;
}

In WP_Rocket\Engine\Media\Fonts\Frontend\Controller

  • Add a new method:
public function maybe_remove_pre_links() {
        if ( ! $this->optimization_context->is_allowed() ) {
            	return false;
        }
    
        $exclusions = wpm_apply_filters_typed( 'string[]', 'rocket_exclude_locally_host_fonts', [] );
        if ( empty( $exclusions ) ) {
                return false;
        }

        return true;
}

In WP_Rocket\Engine\Media\Fonts\Frontend\Subscriber

  • Add a new method that calls the one we just created in the controller
public function maybe_remove_pre_links() {
        return $this->frontend_controller->maybe_remove_pre_links();
}
  • Hook this method to rocket_remove_font_pre_links in the get_subscribed_events method

I'm with Option 1 because it fixes the regression directly that was introduced without adding exponentially increasing the codebase.

jeawhanlee avatar Jul 08 '25 08:07 jeawhanlee

Hello hello @jeawhanlee ,

I'm not sure the option 1 would work with the scenario 2 & 3 from the issue.

Scenario 2: Only Google Fonts combine enabled (no self-hosting) + filters ❌ NOT Covered - Controller doesn't run, preconnects remain

Scenario 3: Cache clearing scenarios ❌ Partially Covered - Only works if self-hosted fonts also enabled

Do you confirm if my search are correct ?

Miraeld avatar Jul 08 '25 10:07 Miraeld

After jumping in a call with @jeawhanlee , We confirm that the Option 1 mentioned:

Option 1

In WP_Rocket\Engine\Media\Fonts\Frontend\Controller

  • Update this method to check matching gstatic or googleapis preconnect urls in the $items array and unset them if exist or keep them if $exclusion is not empty.

should cover every case scenario. So forget my last comment.

And Also, that's a simpler approach than I offered. So let's go with this. No need to groom even more. Looks good to me.

Miraeld avatar Jul 08 '25 11:07 Miraeld