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

Adding files to zip subfolder called as theme slug #357

Merged
merged 7 commits into from
Jun 19, 2023

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented May 9, 2023

What?

Create theme files inside a subfolder called as the theme slug.

Example of zip file structure:

Before:

  • style.css
  • screeenshot.png
  • templates

After:

  • themename/style.css
  • themename/screeenshot.png
  • themename/functions.php

Why?

Avoid problems with theme paths when exporting/cloning themes as zip files.

For example, when you download the theme zip file with a filename different from the expected theme slug. It's common to download files as 'themename (5).zip'. Let's see what happen if you upload that file to a WordPress instance: the theme will live in wp-content/themes/themename-5 instead of wp-content/themes/themename meanwhile the theme slug will continue being themename. There could be references to the theme slug in the theme, for example, if your theme is linking template parts in patterns. The end result will be missing template parts because the parts will be searched in wp-content/themes/themename/parts when they will be located in wp-content/themes/themename-5/parts.

With this change no matter how is the zip file called. The theme will be created in wp-content/themes/themename so we will avoid the errors described before.

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

I get quite a few warnings in WP admin when I apply this branch:

Warning: Declaration of CbtZipArchive::addEmptyDir(string $dirname, int $flags = 0) should be compatible with ZipArchive::addEmptyDir($dirname) in /admin/create-theme/cbt-zip-file.php on line 24

Warning: Declaration of CbtZipArchive::addFromString(string $name, string $content, int $flags = ZipArchive::FL_OVERWRITE) should be compatible with ZipArchive::addFromString($name, $content) in /admin/create-theme/cbt-zip-file.php on line 14

Warning: Declaration of CbtZipArchive::addFile(string $filepath, string $entryname = '', int $start = 0, int $length = 0, int $flags = ZipArchive::FL_OVERWRITE) should be compatible with ZipArchive::addFile($filepath, $entryname = NULL, $start = NULL, $length = NULL) in /admin/create-theme/cbt-zip-file.php on line 19

And a couple of: Warning: Cannot modify header information - headers already sent... warnings.

It looks like CbtZipArchive doesn't quite match the ZipArchive class.

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

I got the same warnings as @mikachan.

@@ -0,0 +1,31 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about naming this file cbt-zip-archive instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, updated!

@matiasbenedetto
Copy link
Contributor Author

I get quite a few warnings in WP admin when I apply this branch:
It looks like CbtZipArchive doesn't quite match the ZipArchive class.

It seems like overriding the ZipArchive class methods is not the best idea because the declaration of the methods is not exactly the same in all PHP versions. To work around this, I called the functions we are using with another name to avoid conflicts. It should work as expected now.

@matiasbenedetto matiasbenedetto requested review from mikachan and jffng May 30, 2023 18:48
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

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

Here's a workflow that I tried that doesn't seem to be working as expected:

  • Export "Adventurer" (a theme that lives in wp-content/themes/themes/adventurer
  • There already exists adventurer.zip in my downloads, so the export is named adventurer (2).zip
  • Upload that theme zip to the same site and receive the following error:
The package could not be installed. The theme is missing the style.css stylesheet.

Theme installation failed.

When I expand the zip locally, it results in a folder themes containing a subfolder adventurer.

@matiasbenedetto
Copy link
Contributor Author

Here's a workflow that I tried that doesn't seem to be working as expected:

Thanks for testing @jffng ! I could not reproduce this issue even with adventurer or any other theme I tried.

@jffng
Copy link
Contributor

jffng commented May 31, 2023

@matiasbenedetto what is the path of the themes you are testing with? I did some more testing and get the error for exported themes that live in a subfolder within the themes folder, e.g. wp-content/themes/community-themes/stacks.

@matiasbenedetto matiasbenedetto requested a review from jffng June 19, 2023 13:47
@matiasbenedetto
Copy link
Contributor Author

@jffng The problem was only reproducible when the original theme came from a subfolder. In that case, the slug we get is a path instead of just the slug of the theme (ex: subfolder/themeslug instead of themeslug). Because of this, the files created in the zip file were added in this path subfolder/themeslug/file.html instead of themeslug/file.html that was causing the errors. With the latest commit that is fixed, and the zip files are created in themeslug/....

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

This is working well for me now using themes in subfolders. I followed @jffng's testing steps from here:

Export "Adventurer" (a theme that lives in wp-content/themes/themes/adventurer) ✅
Upload that theme zip to the same site, no errors and theme activates successfully ✅
Theme installation works ✅
Expanded the zip locally, I can see the theme files in the root of the zip folder ✅

I think this is good to come in now 👍

@matiasbenedetto matiasbenedetto merged commit 74f287b into trunk Jun 19, 2023
@matiasbenedetto matiasbenedetto deleted the add/subfolder-to-zip branch June 22, 2023 16:50
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.

3 participants