WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 years ago

#14999 closed enhancement (fixed)

Reject invalid theme.zip uploads

Reported by: jstrebel Owned by: dd32
Milestone: 3.3 Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: ui-feedback
Focuses: Cc:

Description

When a user uploads (/wp-admin/theme-install.php) a theme.zip file from their desktop that may have an invalid folder structure WP accepts the upload. The theme does not appear in the /wp-admin/themes.php list of available themes so the user assumes it was not taken and they try the upload again and are met with a cryptic "Folder already exists" message.

Perhaps WP could validate the internal file/folder structure of the theme.zip and reject the upload with a clearer message such as: "This theme does not comply with the standard WP folder structure for themes. Please see this topic <link to topic> that explains the proper standard. Note: Please alert the theme author to modify their theme.zip download to comply"

Attachments (5)

14999.diff (2.5 KB) - added by dd32 3 years ago.
initial draft
14999.2.diff (2.7 KB) - added by SergeyBiryukov 3 years ago.
14999.3.diff (1.7 KB) - added by dd32 2 years ago.
14999.minor-whitespace-fix.patch (1.0 KB) - added by SergeyBiryukov 2 years ago.
14999.4.diff (3.1 KB) - added by ryan 2 years ago.
Refresh of .3.diff

Download all attachments as: .zip

Change History (34)

comment:1 follow-up: hakre4 years ago

There is no such "standard WP folder structure for themes". The only thing that can be checked for are the checks that are done to iterate over installed themes and filtering out invalid ones.

I dunno if it's possible to do that decision in reverse to find out if a theme is valid or not. Must be the up-most sub-directory within the themes directory then. Maybe some code needs to be restructured for that and then it's possible.

comment:2 in reply to: ↑ 1 nacin4 years ago

Replying to hakre:

There is no such "standard WP folder structure for themes".

Simply checking for a style.css file is all we need to do here.

The problem does not exist for themes coming from the directory.

comment:3 hakre4 years ago

There should exist a function that checks for a style.css already, isn't it?

With "the directory" nacin assumes wordpress.org/extend/themes, not the directory in which the broken theme has been copied into.

comment:4 blepoxp4 years ago

I looked at this briefly this morning and even tested it by zipping a file with no style.css in it (or other legit theme files for that matter).

The zip is uploaded, unpacked, and moved to the theme folder. If all that works, the importer responds positively because it did upload, unpack, and move a folder to the themes directory as requested.

If you then go look at the available themes, you will see the improperly documented theme at the bottom of the screen in the 'broken themes' sections. Also confirmed is that 'folder already exists' message when attempting to upload it again... but that makes since.

If you didn't want to do verification at the point of unpacking the zip (this process takes place in the parent class BTW and not just the install themes extended class), we could possibly tack on an error message in the theme instal skin that identifies that folder as being broken along with a link as Joshua mentioned . We could possibly even provide a link to delete the folder...

comment:5 hakre4 years ago

Related: #13230 - Incomplete Theme feature discussion might add some valuable thoughts as it's related.

comment:6 dd323 years ago

  • Milestone changed from Awaiting Review to Future Release

See also: #15191

comment:7 dd323 years ago

Closed #18473 as a duplicate.

comment:8 SergeyBiryukov3 years ago

  • Keywords needs-patch added; theme upload removed

dd323 years ago

initial draft

comment:9 dd323 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Milestone changed from Future Release to 3.3
  • Owner set to dd32
  • Status changed from new to accepted

comment:10 ocean903 years ago

  • Cc ocean90 added

SergeyBiryukov3 years ago

comment:11 SergeyBiryukov3 years ago

Perhaps more descriptive messages would be useful (see 14999.2.diff).

comment:12 dd323 years ago

I don't think we need to go into fine grained messages such as that, I think linking to a codex article explaining the format which theme (and plugin) zip's are expected to be in would be a better option.. perhaps we can offer a different message if WP_DEBUG is enabled for developers testing their plugins out..

The strings were simply my late night (its 1am) replacements for 'FOO!' 'Whops, BAH!' :)

comment:13 dd323 years ago

In [18618]:

First slice of rejecting invalid Plugin and Theme zip uploads (Incompatible file structure, not containing a Plugin/Theme, etc). See #14999

comment:14 jane3 years ago

The average user who is following upload instructions from some site has no idea what the files even are, let alone formatting complexities. I vote for a more user-targeted message also.

comment:15 dd323 years ago

I vote for a more user-targeted message also.

You're both right.. A more descriptive error is needed for users, but I'm not sure a developer-specific error of "Hey, You're missing the style.css in the only folder in the archive" is going to be best for end users? Developer directions should be left for a more detailed Codex article instead (which admittedly, I'm not aware of one existing) - but some attempts at filling both uses are below:

  • The uploaded archive was not in an expected format, for more information on the expected formats of WordPress [Theme|Plugin] archives please see the <a>Some-decent-title</a> codex article.
  • WordPress couldn't detect a valid [Theme|Plugin] in the uploaded archive, for more information see ...

After writing those, and there actually being less cases to reject items based on, a more specific developer-aimed message could be along the lines of these adapted from SergeyBiryukov's patch could be used:

  • WordPress couldn't detect a valid Theme in the uploaded archive: Multiple Directories were detected, for more information on the expected format of WordPress [Plugin|Theme] install archives see the blahblah codex article.
  • WordPress couldn't detect a valid Theme in the uploaded archive: a valid style.css file could not be located, for more information on the expected format of WordPress [Plugin|Theme] install archives see the blahblah codex article.

comment:16 chipbennett2 years ago

  • Cc chip@… added

comment:17 Ipstenu2 years ago

If we include developer directions for the error message, I doubt that would help users at all. Even a link would just boggle the average user. Telling them flat out something like "The zip for this [theme|plugin] was not packaged correctly and cannot be installed." might be as good as we can do. It's telling you what the problem is.

We can't begin to tell them how to fix it, since adding in something like "Please contact the source from which you acquired this [plugin|theme] and inform them of this error." just isn't going to happen. And linking them to the codex is a nice idea, but breaking down 'So you got this error. It means the zip file was made incorrectly and you need to tell the developer who made it.' is the most needed information. And that leads to 'How do I tell them?' If we ask them to report it, they'll just tell wordpress.org "Hey your plugin's broken."

Yes, I have low expectations ;)

comment:18 kezzbracey2 years ago

I can see the reason for this enhancement, however it is definitely going to cause many thousands of users to receive the error message when the sole issue is there being no containing folder for a theme or plugin.

This will be the case in a huge number of instances where said plugin or theme previously installed and ran perfectly.

I myself experienced it just recently when playing with the beta, and posted about it here: http://wordpress.org/support/topic/plugintheme-installation-the-package-is-corrupt-or-not-in-the-correct-format

The issues arose with themes and plugins that had worked perfectly in the past.

The error message lead me to think there was an issue with the actual archive itself, so I ended up testing four different types of archiving processes to try and isolate whether it was a certain type of app that was creating the "corrupt" zip.

I do feel that the number of plugins and themes out there that don't have a containing folder, (and hence won't be installable), will far outweigh the number of packages out there that have any other type of invalid folder structure.

As a result, I suspect that if a lack of containing folder is not accounted for in some way, the check will actually cause a far higher number of problems than it actually solves.

Could there not be some way to check if the only issue is lack of a containing folder, and then make an exception and proceed with installation in such a case?

If so, could the archive not just be extracted into a folder of the same name, allowing it to install and function smoothly, just as the process has been up until now?

If such a check can be done, it would allow for other forms of invalid folder structure to still be flagged, but without causing a huge wave of failed installations on all the themes and plugins out there with no containing folder that currently install and run perfectly.

comment:19 follow-up: nacin2 years ago

  • Keywords ui-feedback added; has-patch needs-testing removed

The current string in trunk is: "The package is corrupt or not in the correct format."

Requesting ui-feedback to finalize what we want to say and link to.

comment:20 in reply to: ↑ 19 chipbennett2 years ago

Note: related WPORG Alpha/Beta support forum topic.

Replying to nacin:

The current string in trunk is: "The package is corrupt or not in the correct format."

Requesting ui-feedback to finalize what we want to say and link to.

You're working from Diff 1, then, right?

The problem is that three different types of errors are returning this same string:

if ( !$dirlist || count($dirlist) > 1 ) { // A proper archive should have a single directory entry

...and:

if ( ! isset($dirlist[ 'style.css' ]) ) // A proper archive should have a style.css file in the single subdirectory

So:

  1. No root file directory
  2. Multiple root file directories
  3. No style.css

An error message that encompasses all three:

  • The package is not in the correct format. Themes are required to package files within exactly one root folder, and must include a style.css file.

However, I think it would make sense to return three different messages for three different problems:

  • No root file directory:

The package is not in the correct format. Theme is packaged with files in the root, rather than within a folder.

  • Multiple root file directories:

The package is not in the correct format. Theme is packaged with multiple root folders.

  • No style.css

The package is not in the correct format. Theme package is missing the style.css file.

comment:21 dd322 years ago

If you find a plugin or theme which previously worked, but now fails, it'd be appreciated if you can link to, or attach an example.

Any plugin which previously worked should pass the checks, but as noted, the first scrape may have missed some cases. Dividing developer documentation and user documentation is a fine line, the intentions here is to error out on install attempts where it would otherwise 'install correctly' but would be otherwise not installed or installed incorrectly..

Sorry for the brevity, i'm replying from my mobile phone here.

comment:22 follow-up: dd322 years ago

In [19057]:

Themes not within a subdirectory of the zip need to have the working directory Trailingslashed. See #14999

dd322 years ago

comment:23 dd322 years ago

attachment 14999.3.diff added

Based on feedback, above patch results in a string similar to this:

  • The package is corrupt or not in the correct format. The theme is missing the <code>style.css</code> stylesheet.

I can separate the corrupt/not correct format sentence too. Just need some feedback on the exact strings to use.

comment:24 follow-up: Ipstenu2 years ago

Maybe just "The package could not be installed."

Then the second sentence:

  • The theme is missing the <code>style.css</code> stylesheet.
  • The theme is not packaged in it's own folder.
  • The plugin is not packaged in it's own folder.
  • The plugin contains no files.
Last edited 2 years ago by Ipstenu (previous) (diff)

comment:25 in reply to: ↑ 24 chipbennett2 years ago

Replying to Ipstenu:

Maybe just "The package could not be installed."

I agree. Using the term "corrupt" here is incorrect and misleading. Referring to the upload package as "corrupt" should refer to the uploader's inability to read/unzip the package.

Then the second sentence:

  • The theme is missing the <code>style.css</code> stylesheet.
  • The theme is not packaged in it's own folder.
  • The plugin is not packaged in it's own folder.
  • The plugin contains no files.

These all sound good, too! +1

comment:26 in reply to: ↑ 22 SergeyBiryukov2 years ago

In [19057]

For some reason, [19057] used spaces instead of tabs (see 14999.minor-whitespace-fix.patch).

comment:27 dd322 years ago

For some reason, [19057] used spaces instead of tabs

Cheers. I blame this new IDE for not doing what I tell it to!

comment:28 dd322 years ago

In [19115]:

Tabs > Spaces. Props SergeyBiryukov. See #14999

ryan2 years ago

Refresh of .3.diff

comment:29 ryan2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In [19246]:

More specific error strings for failed theme and plugin package uploads. Props dd32. fixes #14999

Note: See TracTickets for help on using tickets.