Skip to content

Conversation

@mattwiebe
Copy link
Contributor

Themes installed from wpcom on dotorg installs will receive a -wpcom suffix and fail this check. Now they won't.

Testing instructions

  1. Get this branch installed and activated as a plugin on a dotorg site.
  2. Download Modern Business to your dotorg site. direct link
  3. Activate Modern Business in wp-admin
  4. In a wp shell session on your dotorg site, verify that the following returns true:
\A8C\FSE\Full_Site_Editing::get_instance()->is_supported_theme( get_stylesheet() )

This is needed for Automattic/jetpack#13196

Themes installed from wpcom on dotorg installs will receive a `-wpcom` suffix and fail this check. Now they won't.
@mattwiebe mattwiebe added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Goal] Full Site Editing labels Aug 21, 2019
@mattwiebe mattwiebe requested review from a team August 21, 2019 21:21
@mattwiebe mattwiebe requested a review from a team as a code owner August 21, 2019 21:21
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

Interestingly we also rely on normalize theme slug here:

public function normalize_theme_slug( $theme_slug ) {
if ( 'pub/' === substr( $theme_slug, 0, 4 ) ) {
$theme_slug = str_replace( 'pub/', '', $theme_slug );
}
return $theme_slug;
}

We just copied the function over manually because... Iirc the context for FullSiteEditing was different from the context of WpTemplate which made it pull the wrong theme slug. I.e. it pulled the theme slug of the API wordpress instance instead of the actual site instance.

Could just add these lines there, but it may be worth putting the function somewhere which can be used by both classes.

$theme_slug = str_replace( 'pub/', '', $theme_slug );
}
if ( '-wpcom' === substr( $theme_slug, -6, 6 ) ) {
$theme_slug = preg_replace( '#-wpcom$#', '', $theme_slug );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, from a purely performance standpoint, if it would be better to use a substr( $theme_slug, 0, -6 ) instead of the regexp, considering we know both the string length and position.
(Same could be said for the previous str_replace, btw)

@mattwiebe
Copy link
Contributor Author

Could just add these lines there, but it may be worth putting the function somewhere which can be used by both classes.

It seems like a fine idea but, as these plugins may be split out in some future, we'll just leave 'em separate. If these ideas.

I'll update both functions for now, using @Copons' optimization suggestion, too.

Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

LGTM!

@mattwiebe mattwiebe merged commit 70a0e20 into master Aug 23, 2019
@mattwiebe mattwiebe deleted the fix/fse-is-supported-theme-in-jetpack branch August 23, 2019 16:38
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants