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.

Sign up for GitHub

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

Jump to bottom

[bug] Theme Export: Use a better method to determine the theme name #40829

Merged
merged 2 commits into from
May 5, 2022
Merged

[bug] Theme Export: Use a better method to determine the theme name #40829

merged 2 commits into from
May 5, 2022

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented May 4, 2022 •
edited
Loading

What?

When a theme doesn't define a TextDomain, the export fails, and we get this error message:
Screenshot 2022-05-05 at 10 41 01

This changes the method for getting the theme name so that it just uses the folder name, which is safer.

Why?

So that themes can still be exported without defining a TextDomain.

How?

This has to use a bit of string manipulation as we don't store the theme slug anywhere, but I think the logic is sound.

Testing Instructions

  1. Use a theme without a TextDomain defined (for example https://hgg-cloud.de/index.php/s/UOgaldj3BGLYWnR)
  2. Export the theme from the Site Editor
  3. In trunk you get an error message, in this branch it is successful.

@WordPress/block-themers

@scruffian scruffian self-assigned this May 4, 2022
@carolinan
Copy link
Contributor

👍 Themes without text domain in style.css should be valid, not all block themes will have translatable strings.

@carolinan carolinan added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. and removed [a11y] Color Contrast labels May 5, 2022
aristath
aristath approved these changes May 5, 2022
View reviewed changes
Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

I only have a nitpick about alignment of the = signs (I believe it's also part of WPCS), but other than that this looks good.

lib/compat/wordpress-6.0/class-gutenberg-rest-edit-site-export-controller.php Outdated Show resolved Hide resolved
@scruffian @aristath
Update lib/compat/wordpress-6.0/class-gutenberg-rest-edit-site-export…
79ef1be 
…-controller.php

Co-authored-by: Ari Stathopoulos <aristath@gmail.com>
@adamziel adamziel added the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label May 5, 2022
@scruffian scruffian added the [Type] Bug An existing feature does not function as intended label May 5, 2022
@scruffian scruffian changed the title Theme Export: Use a better method to determine the theme name [bug] Theme Export: Use a better method to determine the theme name May 5, 2022
@scruffian scruffian merged commit ad2a96b into trunk May 5, 2022
@scruffian scruffian deleted the update/export-theme-name branch May 5, 2022 09:57
@github-actions github-actions bot added this to the Gutenberg 13.3 milestone May 5, 2022
@gziolo
Copy link
Member

gziolo commented May 6, 2022

I cherry-picked this PR for WordPress 6.0 RC2 with 4ee2055.

@gziolo gziolo removed the Backport to WP 6.6 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label May 6, 2022
gziolo pushed a commit that referenced this pull request May 6, 2022
@scruffian @aristath @gziolo
[bug] Theme Export: Use a better method to determine the theme name (#…
4ee2055 
…40829)

* Theme Export: Use a better method to determine the theme name

* Update lib/compat/wordpress-6.0/class-gutenberg-rest-edit-site-export-controller.php

Co-authored-by: Ari Stathopoulos <aristath@gmail.com>

Co-authored-by: Ari Stathopoulos <aristath@gmail.com>
@jeremyfelt
Copy link
Member

@gziolo It looks like textDomain is still used to determine the theme name in the WP 6.0 branch

Is there still a chance this can be merged before 6.0-RC3? Thanks!

@gziolo
Copy link
Member

gziolo commented May 12, 2022 •
edited
Loading

@peterwilsoncc, @hellofromtonya - it looks like I cherry-picked this change to wp/6.0 branch in Gutenberg by accident. Nothing breaks but it also doesn't fix anything. We need to patch it in the WordPress core directly 😅

Is there still a chance this can be merged before 6.0-RC3? Thanks!

Sure, we will take care of it. Thank you for the ping 👍🏻

dream-encode
dream-encode reviewed May 12, 2022
View reviewed changes
lib/compat/wordpress-6.0/class-gutenberg-rest-edit-site-export-controller.php
@@ -76,7 +76,10 @@ public function export() {
return $filename;
}

$theme_name = wp_get_theme()->get( 'TextDomain' );
$stylesheet = get_stylesheet();
$stylesheet_directories = explode( '/', $stylesheet );
Copy link

Choose a reason for hiding this comment

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

Pardon my ignorance and for jumping in here, but is there any reason why basename couldn't be used instead?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. It might be even better because basename can handle both slash (/) and backslash () which often is an issue on Windows machines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, done here: https://github.com/WordPress/gutenberg/pull/41058/files

@scruffian scruffian mentioned this pull request May 13, 2022
Merged
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request May 17, 2022
@gziolo
Theme: Use a better method to determine the theme name during export
0be18df 
See the same change in the Gutenberg plugin: WordPress/gutenberg#40829.

Props scruffian, davidbaumwald, jeremyfelt.
See #55567.




git-svn-id: https://develop.svn.wordpress.org/trunk@53402 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request May 17, 2022
@gziolo
Theme: Use a better method to determine the theme name during export
102004b 
See the same change in the Gutenberg plugin: WordPress/gutenberg#40829.

Props scruffian, davidbaumwald, jeremyfelt.
See #55567.



Built from https://develop.svn.wordpress.org/trunk@53402


git-svn-id: http://core.svn.wordpress.org/trunk@52991 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@gziolo
Copy link
Member

gziolo commented May 17, 2022

I committed the changes in WordPress core with https://core.trac.wordpress.org/changeset/53402. It still needs to be backported to 6.0 branch today.

gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request May 17, 2022
@gziolo
Theme: Use a better method to determine the theme name during export
f96b076 
See the same change in the Gutenberg plugin: WordPress/gutenberg#40829.

Props scruffian, davidbaumwald, jeremyfelt.
See #55567.



Built from https://develop.svn.wordpress.org/trunk@53402


git-svn-id: https://core.svn.wordpress.org/trunk@52991 1a063a9b-81f0-0310-95a4-ce76da25c4cd
whereiscodedude pushed a commit to whereiscodedude/wpss that referenced this pull request Sep 18, 2022
Theme: Use a better method to determine the theme name during export
c97c57e 
See the same change in the Gutenberg plugin: WordPress/gutenberg#40829.

Props scruffian, davidbaumwald, jeremyfelt.
See #55567.



Built from https://develop.svn.wordpress.org/trunk@53402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo left review comments

@dream-encode dream-encode dream-encode left review comments

@aristath aristath aristath approved these changes

@TimothyBJacobs TimothyBJacobs Awaiting requested review from TimothyBJacobs

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey spacedmonkey is a code owner

Assignees

@scruffian scruffian

Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Bug An existing feature does not function as intended
Projects
No open projects
WordPress 6.0 Editor Tasks
Archived in project
Milestone
Gutenberg 13.3
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@scruffian @carolinan @gziolo @jeremyfelt @aristath @dream-encode @adamziel