[D7] Delta theme

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxordasoft2276961git Unable to complete because of the branch issue mentioned below.

Manual Review

No duplication

Issue with project name: The name of the theme conflicts with an existing project: https://www.drupal.org/project/delta. Please rename the theme to an original project name.

Master Branch

No: Does not follow the guidelines for master branch.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code.

• Drupal bootstrap (https://www.drupal.org/project/bootstrap)
• Fancybox (https://www.drupal.org/project/fancybox)
• SUPERSCROLLORAMA
• Tubular
• jQuery (use https://www.drupal.org/project/jquery_update)
• TweenMax
• FontAwesome

All of these libraries need to use sites/all/libraries and be maintained outside of the theme.

README.txt

No: Does not follow the guidelines for in-project documentation and/or the README Template. Reason: The readme offers people support on a different website however it is a condition of Drupal projects to have all issues reported and handled in the Drupal.org issue queue for the given project.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) Template files reference field names and values directly of a node. Always use render and rather than referencing values directly and preprocess to create printable variables rather than referencing the node object.
2. (*) The template files make assumptions about a node object having fields however there is no mention anywhere to install or enable those fields on all nodes on the site.
3. (*) Templates are given for 'featurs', 'home_page_gallery', 'our_team', 'services', 'slideshow_top' content types however no mention is made as to how to create those content types or what fields they should have.
4. (*) All template files should have file doc declarations.
5. (*) All functions withing template.php and tehme-settings.php need function doc declarations and need to follow Drupal coding standards. Install coder module on your site to analyze those files and get reported issues.
6. (*) Inline styles in html.tpl.php is bad practise. All styles must be declared in css files that can be overridden by site developers.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

This review uses the Project Application Review Template.

Hi Ordasoft, Thanks for Contribution !!

Please find some issues in coding :

core = 7.x mentioned two times in Info file.

<?php 
    define('__ROOT__', dirname(__FILE__));
    require_once __ROOT__ . '/css/style_setting.css';
  ?>

It would be good if you can import css file in info rather than in Head.

Hello,

I cannot git clone what you have posted. You need to post this:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Ordasoft/2276961.git os_delta___drupal_7_business_theme

I found few areas of concern.

README

• You spelled 'libraries' in 'sites/all/libraries' wrong. The files won't work if you follow the incorrect spelling.

HTML.tpl.php

• Why isn't this in your /templates folder?
• As mentioned, your 'require_once' for the style_settings.css file should either be in your .info or your template.php ass drupal_add_css

style_setting.css

• There is PHP in this CSS file. That will not work. Either remove the PHP or make the file .php as outlined here: http://stackoverflow.com/questions/19338116/how-to-use-php-inside-css-file

Libraries

• You are loading 3rd party libraries in your html.tpl.php file. These should be loaded using the Drupal libraries system to be compliant and for best practices for upgrade paths.

@Ordasoft Updating git link :)

Hi @Ordasoft,
In tab the hamburger not is not open, its show the undefined error in template.

Sorry, its by mistake

Ha, your JS files are quite malformed and caused an endless loop in Coder/pareview.sh. This is now fixed, attached is the automated report.

The theme has a security issue and am I assigning this to Manjit.Singh as part of our git admin training so that he can take a look. If he does not find the security issue I'm going to post details about the vulnerability in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

I see one error in the pareview:

http://pareview.sh/pareview/httpgitdrupalorgsandboxordasoft2276961git

The "?>" PHP delimiter at the end of files is discouraged, see https://www.drupal.org/node/318#phptags

./templates/style_setting.php

So is there a '?>' at the end of your style_setting.php file?

Are you sure you've pushed your latest code to your git?

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2480797

Project 2: https://www.drupal.org/node/2395279

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

Let's keep this one open which already has a security issue reported.

@Ordasoft os_delta_form_system_theme_settings_alter this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> in the copyright section of admin settings, I will get a nasty javascript popup. You need to sanitize user provided text before printing, Please check https://www.drupal.org/node/28984 and https://api.drupal.org/api/drupal/includes%21common.inc/group/sanitization/7

Also Please check screenshot for the same.

Yep, good catch @Manjit.Singh!

There are some more XSS issues, for example in node--home_page_gallery.tpl.php the node title is printed unsanitized. Make sure to read https://www.drupal.org/node/28984 again.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

The problem is with node titles for examplke that you simply don't know if it was provided by the site admin or some other less-trusted editor. The point is: all user provided text must be sanitized before printing into HTML to make sure that XSS issues from untrusted sources don't sneak in.

manual review:

1. "print drupal_set_title($node->title);": is wrong in a template file. A template file should only print out already prepared variables. Any processing that you need to do should happen in a preprocess hook. See https://www.drupal.org/node/223430
2. js/custom.js: indentation errors, every level should 2 spaces.
3. don't use "jQuery(document).ready(function ()", use Drupal.behaviors instead. See https://www.drupal.org/node/756722
4. "t('Copyright') . ' © ' . date("Y") . ' ' . check_plain($copyright_developedby);": do not concatenate variables to translatable string, use placeholders with t() instead. See https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7
5. os_delta_preprocess_page(): this is vulnerable to XSS exploits. If I enter f"><script>alert('XSS');</script> as Twitter URL for example in the theme settings I will get a nasty javascript popup. Since the user permission required to change these settings is "administer themes", which is not marked for trusted users only, this is a security blocker. Please check all your functions where you prepare variables for the templates and whether they need to get sanitized.

please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Review of the 7.x-1.x branch (commit 1d02efb):

• The "?>" PHP delimiter at the end of files is discouraged, see https://www.drupal.org/node/318#phptags
  

  ./templates/style_setting.php
  

• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. os_delta_preprocess_page(): why do you run check_plain() twice on the $gmass variables? Looks like double escaping which is bad?
2. There is still a drupal_set_title() in node--home_page_gallery.tpl.php

But otherwise looks RTBC to me.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

Automated Review

There is no commit after `1d02efb` so its same as above (1 warning).

Manual Review

os_delta_modules_check(): IMHO you should be consistently use curly braces with if-else operator in template.php file, usage of colon if ($no_modules) : looks odd here.

After gone through the code, found one more recommendation(above) for you otherwise looks RTBC to me as well, so...

Thanks for your contribution, Ordasoft!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.