WordPress.org

Make WordPress Themes

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#2872 closed theme (not-approved)

THEME: ATOM - 1.3

Reported by: digitalnature Owned by: Frumph
Priority: Keywords: theme-atom,
Cc: gx@…

Description

ATOM - 1.3

Base theme of ATOM (internal) framework. Check out the project page for a full list of features, there are too many to mention here :) -- Developed by <a href="http://digitalnature.ro/">digitalnature</a>

Theme URL - http://digitalnature.ro/projects/atom-test/
Author URL - http://digitalnature.ro/

SVN - http://themes.svn.wordpress.org/atom/1.3
ZIP - http://wordpress.org/extend/themes/download/atom.1.3.zip?nostats=1

Diff with previous version: http://themes.trac.wordpress.org/changeset?old_path=/atom/1.2.0&new_path=/atom/1.3

All previous tickets for this theme: http://themes.trac.wordpress.org/query?col=id&col=summary&col=keywords&col=owner&col=status&col=resolution&keywords=~theme-atom&order=priority

https://themes.svn.wordpress.org/atom/1.3/screenshot.png

Attachments (1)

menu.png (4.1 KB) - added by pseudoxiah 3 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 digitalnature3 years ago

To spare the theme reviewer a copy/paste from a automated script check:

WARNING: could not find the file comments.php in the theme.

this theme doesn't need one. Comments are handled by the meta.php template, but the comment_template filter is present for plugins like disqus to work.

WARNING: Found ini_set in the file js/jquery.atom.js.php. Themes should not change server PHP settings.
Line 3: if (extension_loaded('zlib') && (ini_get('output_handler') != 'ob_gzhandler')) @ini_set('zlib.output_compression', 1);

that code is commented; if it wasn't, it would enable output compression.

WARNING: Found ini_set in the file core/ip2c/flags.css.php. Themes should not change server PHP settings.
Line 3: if (extension_loaded('zlib') && (ini_get('output_handler') != 'ob_gzhandler')) @ini_set('zlib.output_compression', 1);;

output compression again; the file is not used, flags.css is.

WARNING: Found base64_decode in the file core/admin/interface.php. base64_decode() is not allowed.
Line 721: if(is_array($settings = unserialize(base64_decode($_POST['import_data'])))) atom_update_options($settings); els

Encodes all theme settings in a base64 string, so they can be exported to another blog.
I really hope you lift the restriction on base64_decode, because you can't import theme settings without it!

WARNING: Found .exec in the file js/jquery.init.php. PHP sytem calls should be disabled by server admins anyway!.
Line 172: $matches = /^fx\-(.+)/.exec($classes[i]);
Line 177: $matches = /^ease\-(.+)/.exec($classes[i]);
Line 405: var amount = parseInt(/amount-(\d+)/.exec(classes)[1], 10);
Line 406: var direction = /nudge-([^\s]+)/.exec(classes)[1];

JavaScript... Fix your regex

WARNING: Found eval("?>{$text}"); $text = ob_g in the file core/widgets/widget.text.php. eval() is not allowed..

It allows the user to add php code from the theme settings. This code is being run on the front-end on initialization. This is the same as editing the functions.php file from the theme editor.

REQUIRED: No content width has been defined. Example: 
if ( ! isset( $content_width ) ) $content_width = 900;

yes it is, line 283, core/functions.atom.php

REQUIRED: Could not find wp_enqueue_script( 'comment-reply' ). See: Migrating Plugins and Themes to 2.7/Enhanced Comment Display
 <?php if ( is_singular() ) wp_enqueue_script( "comment-reply" ); ?>

the theme doesn't need one, it has it's own comment reply js (line 314, jquery.init.php)

REQUIRED: Could not find comments_template. See: comments_template
 <?php comments_template( $file, $separate_comments ); ?>

like I said above, the filter is there

REQUIRED: core/admin/interface.php. Themes should use add_theme_page() for adding admin pages.

there are too many theme settings and features to be able to include them into one page.

comment:2 Frumph3 years ago

  • Owner set to Frumph
  • Status changed from new to assigned

To cover everything in one quick sentence.

I'll be doing the review, if it passes security review checks with your filters and scripts fine, however it *will* fail for the add_theme_page, you can - additionally use add_theme_page multiple times to add more then one options menu to appear in the appearance section.

comment:3 Frumph3 years ago

  • Resolution set to not-approved
  • Status changed from assigned to closed

"Note: Import works only in the official release of the theme (from the project page). This is because the WordPress Theme Directory is apparently run by amateurs :("

This theme will not pass unless all "warning" and "required" from the theme check plugin has been dealt with.

The guidelines for placing a theme on the repository are specific to allow developers such as yourself create new and innovative functionality while still maintaining positive coding usage with the core API.

This was a cursory review and based on findings that were reported by the developer and that which have been found, this theme will not pass.

=> base64 is not allowed
=> login forms must use wp_login_form();
=> all functions and classes must have a unified prefix

/ Should use settings API

  • Theme is utilizing functionality that would be better served by utilizing a plugin as example the User CSS - there are several plugins that would serve the same purpose.
  • Theme is not doing anything new an innovative that would need the necessity of the work arounds that it is utilizing. Recommending developer utilize more of the core API.

comment:4 pross3 years ago

JavaScript?... Fix your regex

The regex is looking for exec() in all php files, the file in question was js/jquery.init.php.

comment:5 chipbennett3 years ago

If code is commented out, why is it left in the Theme to begin with? If it's commented, it's not needed. If it's not needed, remove it.

Although most/all of the Theme Review Team agree with the restriction, the base64 restriction comes from WPORG, not from us. The only reason you were even able to upload your Theme is because the upload script is temporarily disabled, pending the release of WP 3.1; otherwise, it would have failed the upload process, and never even gotten to us. Simply put: if you have a problem with the base64 restriction, I'm sorry. We can't help you there.

"Note: Import works only in the official release of the theme (from the project page). This is because the WordPress? Theme Directory is apparently run by amateurs :("

Indeed, we are, by definition, amateurs. None of us earns a living in any way related to WordPress. Each one of us is a volunteer, who does Theme reviews in our spare time, as a service to the WordPress community.

If I may invoke a rule from the Ubuntu Community Code of Conduct: be respectful. We reviewers can, will, and do make mistakes. The Guidelines aren't perfect. But none of that makes it any more appropriate to be disrespectful.

Quite frankly, a team of volunteers is generally disinclined to deal with disrespect and condescension such as is evident in your comments on this ticket, and in your inline documentation.

comment:6 cais3 years ago

"Note: Import works only in the official release of the theme (from the project page). This is because the WordPress?? Theme Directory is apparently run by amateurs :("

I see the above as somewhat of a compliment, based on what might be a less common usage of amateur, stemming from the French form of the Latin root of the word meaning a "lover of". In this sense, retaining its French inflexion ("am-a-tEUR"), an amateur is motivated by a love or passion for the activity, like a connoisseur."

Last edited 3 years ago by cais (previous) (diff)

comment:7 follow-ups: digitalnature3 years ago

:)

call_user_func('base64_'.''.'decode', $whatever)

and there goes your "security check"...

what I am saying is that all these requirements are ridiculous. Your basically forcing all theme authors to create identical and shallow themes, and you're encouraging bad coding because theme authors that can't implement a certain feature using the existing WP API will resort to hacks...

comment:8 in reply to: ↑ 7 chipbennett3 years ago

Replying to digitalnature:

:)

call_user_func('base64_'.''.'decode', $whatever)

and there goes your "security check"...

Easily-enough fixed. Do you really want a cat-and-mouse game?

what I am saying is that all these requirements are ridiculous. Your basically forcing all theme authors to create identical and shallow themes, and you're encouraging bad coding because theme authors that can't implement a certain feature using the existing WP API will resort to hacks...

I call shenanigans. That claim is utterly specious. The Guidelines are - by intent and design - mostly silent on Theme design and aesthetics. The Guidelines in no way whatsoever restrict Theme developers' design creativity.

Show us an API that cannot be easily understood and implemented, and we'll work on tutorials for it. Part of the major focus of the Guidelines is to weed out such workaround-hacks, and part of the objectives of the Theme Review Team is to educate Theme developers to help them better understand how to code better-quality Themes.

What are your primary complaints here? The the restriction against using base64 and eval is preventing you from implementing some feature in your Theme?

comment:9 in reply to: ↑ 7 FurciferRising3 years ago

What doesn't get caught by the check, will be caught by us.

Ya know? we actually look through the code as well. ;)

Nice try by the way

Replying to digitalnature:

:)

call_user_func('base64_'.''.'decode', $whatever)

and there goes your "security check"...

what I am saying is that all these requirements are ridiculous. Your basically forcing all theme authors to create identical and shallow themes, and you're encouraging bad coding because theme authors that can't implement a certain feature using the existing WP API will resort to hacks...

comment:10 follow-up: digitalnature3 years ago

Easily-enough fixed. Do you really want a cat-and-mouse game?

what? restrict call_user_func too? :)

Show us an API that cannot be easily understood and implemented, and we'll work on tutorials for it. Part of the major focus of the Guidelines is to weed out such workaround-hacks, and part of the objectives of the Theme Review Team is to educate Theme developers to help them better understand how to code better-quality Themes.

I did on a previous ticket. the comment_form() function which forces you to follow a specific html structure (same with wp_login_form - there should be templates for these, just like there's searchform.php)

More:

  • the category walker, you can't add your own attributes on the select field, so you end up with your own walker
  • the post thumbnail functions - you get a browser-resized image if a thumbnail is missing, I ended up creating my own function which fires a ajax call to create the missing thumbnail size.
  • the wp_page_menu which produces completely different html than a custom menu, I had to create my own function (same with the categories)

Give me one reason why should I use these functions as they are instead of improving them.

What are your primary complaints here? The the restriction against using base64 and eval is preventing you from implementing some feature in your Theme?

The entire idea of running auto-check scripts on theme upload. If you guys review the theme manually, what's the point?

btw: the base64 was perfectly justified in my case - check the code to see what is used for.
Same as eval. What's the problem in allowing the super-admin to run php code from a text widget?
Also what's wrong with using @ini_set() to enable zlib output buffering for example?

comment:11 in reply to: ↑ 10 chipbennett3 years ago

Replying to digitalnature:

Easily-enough fixed. Do you really want a cat-and-mouse game?

what? restrict call_user_func too? :)

You probably don't want to know what we're considering as a "fix" for such a use.

Show us an API that cannot be easily understood and implemented, and we'll work on tutorials for it. Part of the major focus of the Guidelines is to weed out such workaround-hacks, and part of the objectives of the Theme Review Team is to educate Theme developers to help them better understand how to code better-quality Themes.

I did on a previous ticket. the comment_form() function which forces you to follow a specific html structure (same with wp_login_form - there should be templates for these, just like there's searchform.php)

More:

  • the category walker, you can't add your own attributes on the select field, so you end up with your own walker
  • the post thumbnail functions - you get a browser-resized image if a thumbnail is missing, I ended up creating my own function which fires a ajax call to create the missing thumbnail size.
  • the wp_page_menu which produces completely different html than a custom menu, I had to create my own function (same with the categories)

Give me one reason why should I use these functions as they are instead of improving them.

Kindly link to your core Trac tickets where you have submitted patches to improve these functions, and then we can discuss.

What are your primary complaints here? The the restriction against using base64 and eval is preventing you from implementing some feature in your Theme?

The entire idea of running auto-check scripts on theme upload. If you guys review the theme manually, what's the point?

What's the point? To weed out the common issues, so that we can spend more time on the more time-intensive aspects of Theme reviews. Would you rather we spend our time looking for body_class() and wp_list_comments(), or would you rather we spend time looking at the Theme output, options, and the like?

btw: the base64 was perfectly justified in my case - check the code to see what is used for.
Same as eval. What's the problem in allowing the super-admin to run php code from a text widget?
Also what's wrong with using @ini_set() to enable zlib output buffering for example?

Again: take it up with someone above our pay grade. You'll get no traction here, arguing to be able to use base64_encode/decode.

comment:12 FurciferRising3 years ago

"The entire idea of running auto-check scripts on theme upload. If you guys review the theme manually, what's the point"

you pretty much answered that yourself with this

call_user_func('base64_'..'decode', $whatever)

comment:13 follow-up: digitalnature3 years ago

What's the point? To weed out the common issues, so that we can spend more time on the more time-intensive aspects of Theme reviews. Would you rather we spend our time looking for body_class() and wp_list_comments(), or would you rather we spend time looking at the Theme output, options, and the like?

And I just showed you in my first comment that these are not issues at all - at least some of them - so a theme would be prevented from being uploaded just because your script thinks it's a issue?

you pretty much answered that yourself with this

great. disable the automatic check then :)

comment:14 FurciferRising3 years ago

What for? The theme would still be rejected, even after a manual check, the automated script just makes it quicker for us.

comment:15 follow-up: digitalnature3 years ago

so you would still reject a theme just because it uses a certain function, regardless of the context and purpose?

take the javascript exec() in my case - you would reject the theme because it tests for a match in a string, just like for eg. php's strpos does? :)

would you reject it because it doesn't use the comment-reply script, even though it supports this feature?

comment:16 pross3 years ago

The upload scripts are not controlled by us, we cannot switch it off. At the moment is is not even switch on! It will be re-enabled with the release of 3.1. The plugin we use ( Theme-Check ) is just an extended version of the same script API used in the uploader script.

comment:17 pross3 years ago

If you had included your js as an external js file instead of putting it inline in a php file it would have not been flagged. As i stated before, the plugin reads ALL php files for exec().

comment:18 digitalnature3 years ago

so now I'm forced to include all js in .js files? what about dynamically generated javascript?

comment:19 in reply to: ↑ 15 chipbennett3 years ago

Replying to digitalnature:

so you would still reject a theme just because it uses a certain function, regardless of the context and purpose?

Again: above our pay grade.

take the javascript exec() in my case - you would reject the theme because it tests for a match in a string, just like for eg. php's strpos does? :)

Again: above our pay grade.

would you reject it because it doesn't use the comment-reply script, even though it supports this feature?

No. If the Theme comment reply-form JS in a non-standard but legitimate manner, it would not be rejected on the basis of that implementation.

comment:20 in reply to: ↑ 13 chipbennett3 years ago

Replying to digitalnature:

What's the point? To weed out the common issues, so that we can spend more time on the more time-intensive aspects of Theme reviews. Would you rather we spend our time looking for body_class() and wp_list_comments(), or would you rather we spend time looking at the Theme output, options, and the like?

And I just showed you in my first comment that these are not issues at all - at least some of them - so a theme would be prevented from being uploaded just because your script thinks it's a issue?

I still don't agree that e.g. comment_form() shouldn't be used, and filtered in a manner that supports core implementation/features. It is hooked to within inches of its life. If you can't get it to do what you need/want it to do, then I don't know what to tell you.

you pretty much answered that yourself with this

great. disable the automatic check then :)

Uh, no. You completely missed his point.

Again: the scripts allow us to focus on less-common, and less-easily-noticed, issues (such as your example workaround).

comment:21 pseudoxiah3 years ago

While I do have my own reservations regarding some guidelines, the one restricting use of shady functions is the only one I don't even dare to touch.

Users are TERRIFIED when they find functions like eval, base64_decode, gzinflate & co. in the themes they're using.

They're also ignorant and have absolutely no idea what is it, what it does and why it's there. They only know it's bad and it could do bad things to their website. And then they drop the theme and delete everything.

So why export an encoded string when you can export a text file or an XML? Semantical, human understandable and editable?

Why use all those methods to allow users to insert content into themes when you have hooks, filters, widgets, shortcodes? As far as I see this theme is intended for development purposes and to serve as a framework. So do you really think people will use all the bloat you're coding inside? Do you think they're willing to learn a whole new approach when they have Genesis, Thematic, Hybrid, Carrington and so on?

No offense, I had my own fails at meeting user's needs and am trying to speak from experience. And I'm a big fan of your themes.

Salutari din Bucuresti

comment:22 digitalnature3 years ago

Actually I never meant to release this one on wp.org, only themes based on this framework. But a lot of users on my site requested a "auto-update" feature, so my first idea was to upload it here.

Do you think they're willing to learn a whole new approach when they have Genesis, Thematic, Hybrid, Carrington and so on

Why not, what's wrong with having one more to choose from?
I started working on my own framework exactly because none of the above your mentioned met my requirements...

Why use all those methods to allow users to insert content into themes when you have hooks, filters, widgets, shortcodes?

can you be more specific?

comment:23 pseudoxiah3 years ago

OK, I'll give my try:

To spare the theme reviewer a copy/paste from a automated script check:

WARNING: could not find the file comments.php in the theme.

this theme doesn't need one. Comments are handled by the meta.php template, but the comment_template filter is present for plugins like disqus to work.

WARNING: Found base64_decode in the file core/admin/interface.php. base64_decode() is not allowed.
Line 721: if(is_array($settings = unserialize(base64_decode($_POST['import_data'])))) atom_update_options($settings); els

Encodes all theme settings in a base64 string, so they can be exported to another blog.
I really hope you lift the restriction on base64_decode, because you can't import theme settings without it!

As said above, the export could be a text or XML file, see Atahualpa for an example (not that I like that theme).

WARNING: Found eval("?>{$text}"); $text = ob_g in the file core/widgets/widget.text.php. eval() is not allowed..

That's what Child Themes are for. If one is willing to add additional functionality to the theme, he's probably capable of doing a child theme.

REQUIRED: core/admin/interface.php. Themes should use add_theme_page() for adding admin pages.

there are too many theme settings and features to be able to include them into one page.

See the attached menu.png. That's how a client's menu looks like. Would you like your theme's menu entry to be number 4 in that list? But wait, you've assigned it a custom position. If a plugin also requests that position, either yours or the other is gone.

You can use one submenu entry for the theme options and then use tabs to split the pages, that's the way I find to be most simple and elegant, but you can also generate it with jQuery and so on.

And other notes:

The theme appears to dynamically generate CSS and JavaScript. I didn't check but I assume you require wp-load.php once or twice to do that. If so, it puts an extremely heavy load on the request, and that just to fetch some variables from the database.

Instead, you could use static scripts, I'm sure there's a workaround, and put small snippets of scripts in the main document where you ask for those variables.

Again, I'm not sure if you do this so sorry if I misguessed.

The ad code block allow raw scripts to be placed in the document. This could be a huge security threat. Instead, you could do a simple widget area and allow ads to be placed through text widgets.

I don't see the point of the custom CSS and PHP code. If one needs to change this, they'll do a child theme.

Since this is a theme and not a plugin, and a theme's main purpose is to offer visual customization, it would be an idea to have options to customize colors, layouts, typography, content areas.

The theme has very basic options in these areas and instead offers some options that are better left to plugins. What if a user decides to switch the theme? They'll lose all of the custom PHP and ad blocks.

I know, you put an awful lot of work into this theme and lots of ideas into practice and now some assholes come and shit on it.

Believe me, nobody is trying to maintain military order here and we can be flexible up to one point. But when it comes to reinventing the wheel, using shady code and techniques that could create conflicts, and above all that most of then could be easily done otherwise, nobody is going to tolerate.

And I'm sure I still didn't cover all the areas. Hope I was clear enough.

pseudoxiah3 years ago

comment:24 follow-up: Frumph3 years ago

The comment_form() can use the 'fields' portion of the $args array that can be passed to the comment_form().

What I see you trying to justify with the comment_form() is the use of the jquery unhide on mouseover for the reply - which while looking at your output code and the $args that can be passed to the comment_form it appears there's no reason why you couldn't use the comment_form().

  • post thumbnail functions, there's a plugin to regenerate your thumbnail images if you need to. If there is an issue EVER where it doesn't find a thumbnail then this should be brought up as a core trac ticket, I have never seen a case where a thumbnail was not generated. There is even code for you to create your own additional thumbnail sizes.

comment:25 digitalnature3 years ago

See the attached menu.png. That's how a client's menu looks like. Would you like your theme's menu entry to be number 4 in that list? But wait, you've assigned it a custom position. If a plugin also requests that position, either yours or the other is gone.

thanks! I didn't know that.

You can use one submenu entry for the theme options and then use tabs to split the pages, that's the way I find to be most simple and elegant, but you can also generate it with jQuery and so on.

I thought of that, but the problem is some javascript in the theme settings that would need to be loaded/unloaded on tab switch...

The theme appears to dynamically generate CSS and JavaScript?. I didn't check but I assume you require wp-load.php once or twice to do that. If so, it puts an extremely heavy load on the request, and that just to fetch some variables from the database.

no, the generated css is inline, so no extra request is made for it. The generated javascript can either be included at the end of the document, or loaded with another request, separately (there's an option for this because in some situations the 2nd alternative might be faster).

The ad code block allow raw scripts to be placed in the document. This could be a huge security threat. Instead, you could do a simple widget area and allow ads to be placed through text widgets.

no, all input data from $_POST is filtered based on the current user capabilities.

I don't see the point of the custom CSS and PHP code. If one needs to change this, they'll do a child theme.

this is the problem... most people don't use child themes, they just change the style.css or functions.php and they loose all their changes when they update. I know this from hundreds of comments asking me "do I loose any changes I made to style.css when I update?"...

Since this is a theme and not a plugin, and a theme's main purpose is to offer visual customization, it would be an idea to have options to customize colors, layouts, typography, content areas.

The user shouldn't be able to change the theme appearance _too much_. What's the purpose of the theme if he wants to be the designer? Also the options you mentioned are present, less for colors.

The theme has very basic options in these areas and instead offers some options that are better left to plugins. What if a user decides to switch the theme? They'll lose all of the custom PHP and ad blocks.

no, they don't. these are saved in the db. also there's a theme export/import settings feature.

I know, you put an awful lot of work into this theme and lots of ideas into practice and now some assholes come and shit on it.

not really, I love criticism, my problem here is with these unnecessary restrictions which basically say "take off feature X if you want the theme approved".

And I'm sure I still didn't cover all the areas. Hope I was clear enough.

please go on, your first point helped making Atom a little better :)

comment:26 in reply to: ↑ 24 digitalnature3 years ago

Replying to Frumph:

The comment_form() can use the 'fields' portion of the $args array that can be passed to the comment_form().

What I see you trying to justify with the comment_form() is the use of the jquery unhide on mouseover for the reply - which while looking at your output code and the $args that can be passed to the comment_form it appears there's no reason why you couldn't use the comment_form().

no.
check out how atom_comment_form() looks like, and try implementing the same markup with comment_form().
the only way to achive that is by passing certain html tags in $args, and closing these tags in a hook. That's unacceptable, and I think you understand why...

  • post thumbnail functions, there's a plugin to regenerate your thumbnail images if you need to. If there is an issue EVER where it doesn't find a thumbnail then this should be brought up as a core trac ticket, I have never seen a case where a thumbnail was not generated. There is even code for you to create your own additional thumbnail sizes.

yes, I know about that plugin, but other people might not. The theme uses these functions to create additional thumbnails. The problem appears when the user changes the thumbnail size (Atom allows you to do this). Obviously you didn't look or test the theme so you don't quite understand what I'm talking about...

comment:27 Frumph3 years ago

We have told you what you needed to do to pass the theme review. If you wish further explanations of "how to" - I suggest asking for assistance from people on the forums, stack overflow or some other location at this point.

comment:28 digitalnature3 years ago

I didn't ask for explanations on how to code my theme.

comment:29 Frumph3 years ago

This theme including every theme that you will ever upload to be added to the repository will not pass the theme review process unless you follow through with the guidelines. To that end we try to assist as much as possible to help you understand your problematic coding.

However, at this point the theme review team is being declined by you as a developer to accept any assistance. Any further communication will be primarily a standard review will more then likely be a fail until it passes the theme review guidelines.

comment:30 follow-up: digitalnature3 years ago

right, improving wp and fixing its bugs is problematic...

comment:31 in reply to: ↑ 30 ; follow-up: chipbennett3 years ago

Replying to digitalnature:

right, improving wp and fixing its bugs is problematic...

Great! Then you've reported those bugs, and submitted patches to fix them? What are your Trac ticket numbers? I'd love to read them.

comment:32 Frumph3 years ago

This ticket should no longer be responded to by any of the review team members.

comment:33 in reply to: ↑ 31 digitalnature3 years ago

Replying to chipbennett:

Replying to digitalnature:

right, improving wp and fixing its bugs is problematic...

Great! Then you've reported those bugs, and submitted patches to fix them? What are your Trac ticket numbers? I'd love to read them.

Most of them are already reported, for eg.
http://core.trac.wordpress.org/ticket/13998
http://core.trac.wordpress.org/ticket/15311
http://core.trac.wordpress.org/ticket/14686

Note: See TracTickets for help on using tickets.