Skip to content

Fix @import/@charset loss in CSS concat (closure scoping bug)#98

Merged
mreishus merged 1 commit into
mainfrom
reish20260203/fix-dropped-imports
Feb 3, 2026
Merged

Fix @import/@charset loss in CSS concat (closure scoping bug)#98
mreishus merged 1 commit into
mainfrom
reish20260203/fix-dropped-imports

Conversation

@mreishus
Copy link
Copy Markdown
Contributor

@mreishus mreishus commented Feb 3, 2026

Fixes the bug shown by the failing test added in PR #97.

The problem:

When concatting CSS, service.php hoists @charset and @import rules to the top of the output via a $pre_output buffer. The callbacks that collect these rules used global $pre_output - $pre_output is a local variable inside page_optimize_build_output(), not a global.

In PHP, global $pre_output references $GLOBALS['pre_output'], which is a completely different variable. So the callbacks were:

  1. Stripping @import from the source buffer ✓
  2. Appending it to the wrong $pre_output

The lifted rules vanished completely.

The fix:

Change both preg_replace_callback closures from:

function ( $match ) {
    global $pre_output;

to:

function ( $match ) use ( &$pre_output ) {

This captures the local $pre_output by reference, so hoisted rules actually make it into the output.

Testing:

The test from #97 now passes. You can also hit the concat endpoint directly with a CSS file containing @import and confirm it appears in the response.

Example plugin:

<?php
/**
 * Plugin Name: PO CSS Import Order Test
 * Description: Test @import ordering behavior with Page Optimize concatenation.
 */

add_action( 'wp_enqueue_scripts', function() {
	if ( ! isset( $_GET['po_import_test'] ) ) {
		return;
	}

	$upload = wp_upload_dir();
	$dir    = trailingslashit( $upload['basedir'] ) . 'po-import-test';
	$url    = trailingslashit( $upload['baseurl'] ) . 'po-import-test';

	if ( ! file_exists( $dir ) ) {
		wp_mkdir_p( $dir );
	}

	// a.css: Sets .po-test to red.
	$files = [
		'a.css'     => '.po-test { color: red; }',
		'b.css'     => '@import "b-dep.css";',
		'b-dep.css' => '.po-test { color: blue; }',
	];

	foreach ( $files as $name => $contents ) {
		$path = $dir . '/' . $name;
		// Always rewrite to ensure contents are current.
		file_put_contents( $path, $contents );
	}

	// Enqueue A then B.
	// Expected: If processed correctly, blue should win (B comes after A).
	wp_enqueue_style( 'po-import-a', $url . '/a.css', [], null, 'all' );
	wp_enqueue_style( 'po-import-b', $url . '/b.css', [ 'po-import-a' ], null, 'all' );
}, 20 );

// Add test element to the page.
add_action( 'wp_footer', function() {
	if ( ! isset( $_GET['po_import_test'] ) ) {
		return;
	}
	?>
	<div style="position:fixed;bottom:20px;right:20px;padding:20px;background:#fff;border:2px solid #333;z-index:99999;font-family:monospace;">
		<p style="margin:0 0 10px;font-weight:bold;">PO @import Test</p>
		<p class="po-test" style="margin:0;font-size:18px;">
			This text should be <strong>BLUE</strong> if @import is handled correctly.
		</p>
		<p style="margin:10px 0 0;font-size:12px;color:#666;">
			Red = @import lost in concatenation<br>
			Blue = Correct cascade order
		</p>
	</div>
	<?php
}, 100 );

// Debug output for Page Optimize concatenated tags.
add_filter( 'page_optimize_style_loader_tag', function( $tag, $handles, $href, $media ) {
	if ( ! isset( $_GET['po_import_test'] ) ) {
		return $tag;
	}
	$list = is_array( $handles ) ? implode( ',', $handles ) : $handles;
	return $tag . "<!-- po-concat: {$list} | media={$media} | href={$href} -->\n";
}, 100, 4 );

// Debug output for standard style tags.
add_filter( 'style_loader_tag', function( $tag, $handle, $href, $media ) {
	if ( ! isset( $_GET['po_import_test'] ) ) {
		return $tag;
	}
	return $tag . "<!-- po-core: {$handle} | media={$media} | href={$href} -->\n";
}, 100, 4 );
  • Visit example.com/?po_import_test - Note that the rule from b-dep.css is gone completely on trunk, and shows up in this PR. However, there's still an ordering issue (we will fix later).

@mreishus
Copy link
Copy Markdown
Contributor Author

mreishus commented Feb 3, 2026

It seems this was previously fixed in #51 7e4e3cd (2021-06-28 12:56 o Fix use of global pre-output var) but then reverted in d42b8d5 (2021-12-14 10:07 o Revert to v0.5.1 to allow a simple readme update), and never restored.

@mreishus mreishus merged commit 188b7db into main Feb 3, 2026
3 checks passed
@mreishus mreishus deleted the reish20260203/fix-dropped-imports branch February 3, 2026 16:36
mreishus added a commit that referenced this pull request Feb 3, 2026
mreishus added a commit that referenced this pull request Feb 3, 2026
mreishus added a commit that referenced this pull request Feb 3, 2026
mreishus added a commit that referenced this pull request Feb 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant