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

Try to cache the theme json file reading #3401

Conversation

cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Oct 4, 2022

Comment on lines 203 to 204
$theme_json = apply_filters( 'theme_json_theme', new WP_Theme_JSON_Data( $theme_json_data, 'theme' ) );
$theme_json_data = $theme_json->get_data();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we set static::$theme_json_data after these two lines. Then the theme json file does not apply.

static::$theme_json_data = $theme_json_data;
$theme_json = apply_filters( 'theme_json_theme', new WP_Theme_JSON_Data( $theme_json_data, 'theme' ) );
$theme_json_data = $theme_json->get_data();
static::$theme = new WP_Theme_JSON( $theme_json_data );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should test it with child themes.

@cbravobernal
Copy link
Contributor Author

cc/ @oandregal, would this approach be correct?

@ockham
Copy link
Contributor

ockham commented Oct 5, 2022

Rebased.

@ockham ockham force-pushed the try/fix-cache-block-styles-theme-json branch from 4f69396 to e2704ab Compare October 5, 2022 12:45
@ockham
Copy link
Contributor

ockham commented Oct 5, 2022

I think this is working now 🎉

I was about to implement file caching for the parent theme (see #3401 (comment)), but then it occurred to me that we might solve this slightly differently:

We can add a single $theme_json_file_cache static var, and use it from within static::read_json_file().

I'll give that a try.

@cbravobernal
Copy link
Contributor Author

#3408 does the work, so we can close this one.

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.

2 participants