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 check for included .zip files #151

Merged
merged 4 commits into from
May 10, 2016

Conversation

kkoppenhaver
Copy link
Contributor

Addresses #134

Checks for and .zip files present in the repository and throws an error on finding one.

foreach( $blacklist as $file => $reason ) {
if ( $filename = preg_grep( '/' . $file . '/', $filenames ) ) {
$error = implode( array_unique( $filename ), ' ' );
$this->error[] = sprintf( '<span class="tc-lead tc-required">'.__( 'REQUIRED','theme-check').'</span>: '.__( '<strong>Zip file found.</strong> Plugins are not allowed in themes. The zip file found was ', 'theme-check') . '<em>' . __( $error ) . '</em>.' ) ;
Copy link
Member

Choose a reason for hiding this comment

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

We can't translate variables __( $error ). As you are already using sprintf you can do __( 'The zip file found was %s', 'theme-check' ) and then define $error as the counterpart to %s

@kkoppenhaver
Copy link
Contributor Author

Good call, updated.

@Otto42
Copy link
Member

Otto42 commented May 10, 2016

You don't need to check the $php_files and $css_files variables. Those are literally just *.php and *.css. They can't be zip files.

@kkoppenhaver
Copy link
Contributor Author

Thanks, updated.

@Otto42
Copy link
Member

Otto42 commented May 10, 2016

Also, you have this as a REQUIRED check. Has this been discussed amongst the theme-review team? Is this requirement documented in the theme review documentation somewhere?

If not, then it needs to be a RECOMMENDED check instead. Theme Check only implements the existing requirements as created by the review team, it doesn't create new ones. That said, it would be fine as recommended, because including ZIPs in themes makes little sense.

@grappler
Copy link
Member

Yes, we discussed this before. The requirement this is based on is https://make.wordpress.org/themes/handbook/review/required/#plugins

It does not make sense to have additional zip files in the theme.

@Otto42 Otto42 merged commit 555e0dc into WordPress:master May 10, 2016
@kkoppenhaver
Copy link
Contributor Author

Thanks everyone for the tips and pointers. This was my first PR!

@kkoppenhaver kkoppenhaver deleted the check-for-included-plugins branch May 10, 2016 18:29
@grappler
Copy link
Member

grappler commented Jun 4, 2016

@kkoppenhaver I just wanted to let you know this PR has been a success. I has caught a number of themes who included plugins in the theme.

@kkoppenhaver
Copy link
Contributor Author

@grappler That's awesome to hear!

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