WordPress.org

Make WordPress Themes

Opened 4 years ago

Closed 4 years ago

#17254 closed theme (not-approved)

THEME: AcosminBlogger - 1.0.4

Reported by: acosmin Owned by: ZGani
Priority: previously reviewed Keywords: theme-acosminblogger
Cc: support@…

Description

AcosminBlogger - 1.0.1

AcosminBlogger is a theme with clean lines and an open-spaced design, that can be used to show off your latest articles.

Theme URL - http://www.acosmin.com/theme/acosminblogger/
Author URL - http://www.acosmin.com/

SVN - https://themes.svn.wordpress.org/acosminblogger/1.0.1
ZIP - https://wordpress.org/themes/download/acosminblogger.1.0.1.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=/acosminblogger/1.0&new_path=/acosminblogger/1.0.1

History:

Ticket Summary Status Resolution Owner
#17160 THEME: AcosminBlogger - 1.0 closed not-approved ZGani
#17254 THEME: AcosminBlogger - 1.0.4 closed not-approved ZGani

(this ticket)


https://themes.svn.wordpress.org/acosminblogger/1.0.1/screenshot.png

Change History (20)

#1 @themetracbot
4 years ago

  • Summary changed from THEME: AcosminBlogger - 1.0.1 to THEME: AcosminBlogger - 1.0.2

AcosminBlogger - 1.0.2

AcosminBlogger is a theme with clean lines and an open-spaced design, that can be used to show off your latest articles.

Theme URL - http://www.acosmin.com/theme/acosminblogger/
Author URL - http://www.acosmin.com/

SVN - https://themes.svn.wordpress.org/acosminblogger/1.0.2
ZIP - https://wordpress.org/themes/download/acosminblogger.1.0.2.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=/acosminblogger/1.0.1&new_path=/acosminblogger/1.0.2

History:

Ticket Summary Status Resolution Owner
#17160 THEME: AcosminBlogger - 1.0 closed not-approved ZGani
#17254 THEME: AcosminBlogger - 1.0.4 closed not-approved ZGani

(this ticket)


https://themes.svn.wordpress.org/acosminblogger/1.0.2/screenshot.png

#2 @ZGani
4 years ago

  • Owner set to ZGani
  • Status changed from new to reviewing

#3 @ZGani
4 years ago

General Check

Required:

  • Themes must explicitly declare license and copyright of any resources used in constructing the theme. i.e. scripts, images and fonts in the following format:
    Resource name here - ​http://resource.url/
    License: Distributed under the terms of the License Name
    Copyright: Author, authorurl.com
    

Additional notes for your consideration: On the FAQ page there are some confusing terms that you might want to look at.

For example under "Purchasing questions" section it states that the standard license carries a 1 year update and support while the developer carries 2 of the same.

But further down in the "Theme related questions" it states "Free lifetime updates are included." which contradicts one half of the above statement.

In the question "Can I upgrade my license, from single to developer?" change "single" to standard for consistency.

You'd also want to term the last question/answer a bit better - GPL allows for modifications and redistribution without any restrictions. I'm actually not sure how that statement fits in with the GPL so we'll have to get it clarified by one of the Admins before theme approval.

Code Check.

Required:

  • Please include the unminified version of slider.js for code reference and to facilitate end user access to code for modifications.
  • Both social-single.php and social-index.php files are plugin territory and must be removed from the theme
  • header.php: @lines 30 & 82 use esc_url(home_url()) instead of home_url() - do the same for footer.php
  • header.php: The ad code does not seem to be escaped correctly.
  • header.php The above is also true for the social links.
  • footer.php: wp_footer(); must be placed immediately before the closing </body> tag.
  • footer.php: Please remove the word "Themes" from the credit link title.
  • sidebar-browse.php: ad code needs escaping correctly.
  • featured-content.php: Use wp_reset_postdata(); instead of wp_reset_query();
  • functions.php: Themes are required to use theme-slug (or a reasonably unique slug) as a prefix for anything in the public namespace, including all custom function names, classes, hooks, public/global variables, database entries (Theme options, post custom metadata, etc.) - You are using ac_ for some variables and acosminblogger_ for others.
  • functions.php: Other than the Google fonts (external resource) you do not need to register the bundled resources - simply enqueue the resources and WordPress will take care of it.
  • functions.php: The function ac_body_classes( $classes ) is not actually in use - please remove it.
  • The "Update Framework" under Appearance section can be a bit confusing to end users especially when there's nothing to update - I'd suggest moving it to a tab within the Options page if there's a need for it otherwise remove it.
  • Themes must not add widgets anywhere in the Admin area other than the Designated options page - Please remove the Acosmin Latest News widget.
  • functions-theme-customizer.php: Insufficient or no sanitization for certain sections - capability check is not sufficient on itself, you must use sanitize callback on all untrusted user input data.
  • functions-default.php: Google Analytic is plugin territory - please remove it.
  • Likewise is the code for Facebook SDK in the same file above.
  • functions-user.php: This file is redundant and of little use - all user data will be lost on theme update so its not wise to encourage such usage. Provide a functionality plugin instead or recommend one of the many free ones in the repository.
  • Please make sure that all user input data is being escaped correctly on out - this refers to a number of files in the theme in addition to those mentioned above.

Recommended:

  • header.php: It would be ideal to place the searchform code in a searchform.php and then reference it correctly where ever its needed.
  • 404.php: It would be a good idea to add a searchform to this file.

Post Installation.

Theme Check: PASS'''

Theme Unit Test.

Required: I've marked this item as required because as it is the issue defeats the purpose of having a sticky menu.

  • On page scroll the fixed navbar partially falls behind the Admin bar when active - you should be adding a clearance of about 28px to the navbar to take account of an active Admin bar.

Recommended:

No other issues detected.

This is a complete review and all items marked as Required must be addressed before the theme is resubmitted for further consideration.

Leaving ticket open to facilitate a speedy follow up review - please upload a revised version when ready and I'll continue with the review.
If you have any questions feel free to ask in the comments.

Zulf.

P.S. Re the "Update Framework" could you explain how this would work it is allowed to remain in the theme? Do bare in mind that the entire theme is required to be updated from within WordPress i.e. from the hosted copy!

Last edited 4 years ago by ZGani (previous) (diff)

#4 follow-up: @acosmin
4 years ago

Hi!

Because the theme is updated from within WordPress I will remove the "Update Framework" option.

I have a few questions:

Both social-single.php and social-index.php files are plugin territory and must be removed from the theme.

I understand removing social-single (because it also comes with some javascript resources) but can I leave social-index. Those are only links and there is also a option (in the options panel) to disable them. I can set the option to disabled by default.


functions.php: Themes are required to use theme-slug (or a reasonably unique slug) as a prefix for anything in the public namespace, including all custom function names, classes, hooks, public/global variables, database entries (Theme options, post custom metadata, etc.) - You are using ac_ for some variables and acosminblogger_ for others.

Do I need to change theme-slug for Options Framework too (eg: of_get_option)? Is it necessary?


404.php: It would be a good idea to add a searchform to this file.

There is a link under 404, when clicked it will open the search form. Could I leave it as it is?


Thank you for the review and your time!

#5 in reply to: ↑ 4 @ZGani
4 years ago

Replying to acosmin:

Hi!

Because the theme is updated from within WordPress I will remove the "Update Framework" option.

I have a few questions:

Both social-single.php and social-index.php files are plugin territory and must be removed from the theme.

I understand removing social-single (because it also comes with some javascript resources) but can I leave social-index. Those are only links and there is also a option (in the options panel) to disable them. I can set the option to disabled by default.

I'm afraid that's a no for both as they are "Social Share scripts" and not "Social profile links" - you can provide option for end user to link to their social profiles but not content sharing scripts!


functions.php: Themes are required to use theme-slug (or a reasonably unique slug) as a prefix for anything in the public namespace, including all custom function names, classes, hooks, public/global variables, database entries (Theme options, post custom metadata, etc.) - You are using ac_ for some variables and acosminblogger_ for others.

Do I need to change theme-slug for Options Framework too (eg: of_get_option)? Is it necessary?

No, the prefix for the framework is fine - just theme related global variables and functions need prefixing with the theme_slug or variant of.


404.php: It would be a good idea to add a searchform to this file.

There is a link under 404, when clicked it will open the search form. Could I leave it as it is?

Yes that is perfectly fine - Sorry, I didn't notice the link and had based the review on file code rather than operation. Perhaps a subtle hint to click would let the end user know that its a link to be clicked?


Thank you for the review and your time!

You are welcome :) - its a very nice theme.

Please upload a revised version when ready.

Regards,
Zulf

#6 @acosmin
4 years ago

I have a small problem. The ads located in header.php and sidebar-browse.php are escaped by Options Framework. What should I do to escape them correctly?

I know I allowed the script tag to go through wp_kses, is that the problem? I can remove it if necessary, but Adsense won't work. I am talking about ../acosmin/framework/extensions/functions-extended.php on line 38.

#7 @themetracbot
4 years ago

  • Summary changed from THEME: AcosminBlogger - 1.0.2 to THEME: AcosminBlogger - 1.0.3

AcosminBlogger - 1.0.3

AcosminBlogger is a theme with clean lines and an open-spaced design, that can be used to show off your latest articles.

Theme URL - http://www.acosmin.com/theme/acosminblogger/
Author URL - http://www.acosmin.com/

SVN - https://themes.svn.wordpress.org/acosminblogger/1.0.3
ZIP - https://wordpress.org/themes/download/acosminblogger.1.0.3.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=/acosminblogger/1.0.2&new_path=/acosminblogger/1.0.3

History:

Ticket Summary Status Resolution Owner
#17160 THEME: AcosminBlogger - 1.0 closed not-approved ZGani
#17254 THEME: AcosminBlogger - 1.0.4 closed not-approved ZGani

(this ticket)


https://themes.svn.wordpress.org/acosminblogger/1.0.3/screenshot.png

#8 follow-up: @acosmin
4 years ago

I managed to finish all your requests... except the issues @ comment #6.

#9 in reply to: ↑ 8 @ZGani
4 years ago

Replying to acosmin:

I managed to finish all your requests... except the issues @ comment #6.

Let me take a look at the theme and retrace some of the issues and fixes and then we can work on any (if any) outstanding issues?

I'll get this done later tomorrow and then come back with and updated feedback for you.

Regards,
Zulf

#10 @ZGani
4 years ago

Code check.

Required:

  • In functions-theme-customizer.php: Use esc_url_raw for sanitization instead of esc_url - esc_url should only be used for output values.
  • In the same file, use sanitize_text_field for input and then use esc_html for the data output.
  • In addition to the above sanitization for checkbox and select options are required.
  • functions-extended.php: The function ac_render_toolbar_menu() is not allowed as themes are required to use add_theme_page for adding option and related page/links.
  • header.php: escape the home_url() in the search form.
  • With reference to the Public Domain License for the images used is the screenshot, I don't believe these can be treated as being GPL compatible as the license is not explicitly stated - there a variants of the public domain (CreativeCommoms) license termed as CC By followed by version number and the only CC license compatible with GPL is the CC By 0 license.

No issues found with post installation and/or Theme Unit Test.

This is a full review - Other than the above issues all other previously raised issues have been resolved satisfactory. Please address the above issues and upload a revised version so that I can conclude this review with a view to approving the theme.

As always, if you have any questions please feel free to ask in the comments.

#11 @ZGani
4 years ago

Additional notes on the above review.

You do not need to register the bundled resources before enqueueing them - simply enqueue them and WordPress will take care of the rest.

The only time you need register a resource before enqueueing it is if its an allowed external resources i.e. Google fonts.

Please sure to change this before submit the revised version.

Regards,
Zulf

#12 @themetracbot
4 years ago

  • Summary changed from THEME: AcosminBlogger - 1.0.3 to THEME: AcosminBlogger - 1.0.4

AcosminBlogger - 1.0.4

AcosminBlogger is a theme with clean lines and an open-spaced design, that can be used to show off your latest articles.

Theme URL - http://www.acosmin.com/theme/acosminblogger/
Author URL - http://www.acosmin.com/

SVN - https://themes.svn.wordpress.org/acosminblogger/1.0.4
ZIP - https://wordpress.org/themes/download/acosminblogger.1.0.4.zip?nostats=1

Diff with previous version: https://themes.trac.wordpress.org/changeset?old_path=/acosminblogger/1.0.3&new_path=/acosminblogger/1.0.4

History:

Ticket Summary Status Resolution Owner
#17160 THEME: AcosminBlogger - 1.0 closed not-approved ZGani
#17254 THEME: AcosminBlogger - 1.0.4 closed not-approved ZGani

(this ticket)


https://themes.svn.wordpress.org/acosminblogger/1.0.4/screenshot.png

#13 @ZGani
4 years ago

  • Status changed from reviewing to approved

Resources are still being registered before enqueueing @line 109 to 114 in functions.php - Approving theme with recommendation to address this issue in the next update.

All the best,
Zulf

#14 @emiluzelac
4 years ago

  • Status changed from approved to reopened

Theme Name Guidelines are required for new Themes, and recommended for existing Themes.

  • Themes are not to use related terms (e.g. Blog, Web Log, Template, Skin, etc.) in their name.

see
---
Solution is to rename and resubmit. Note the ticket and Zulf will approve again.

Sorry!

#15 @emiluzelac
4 years ago

  • Status changed from reopened to reviewing

#16 @acosmin
4 years ago

Now that’s a bummer! :) I need to change some things now (links, redirects...) but is "Simplified" ok for a theme name?

Just to be sure.

Thanks for the review!

#17 @emiluzelac
4 years ago

Sorry about that. As far as the new name, you will know once the Theme is resubmitted. Not sure if that name is already taken. To me, it's fine :-)

#18 @acosmin
4 years ago

Can I submit it now and make sure it gets approved before I change names, links, redirects on my website? (it will be exactly the same as http://www.acosmin.com/theme/acosminblogger/ but with the name changed)

#19 @acosmin
4 years ago

I resubmitted it, you can find it here https://themes.trac.wordpress.org/ticket/17402

I've also added support for child themes (functions.php).

I will change all the links, redirects on my website if the name is accepted.

Thank you!

#20 @ZGani
4 years ago

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

Closing due to name change requirements - theme renamed and resubmitted: https://themes.trac.wordpress.org/ticket/17402#comment:1

Note: See TracTickets for help on using tickets.