Project link: http://drupal.org/sandbox/EricoVinicius/1111334
Vintage is an D6 starter theme with an attitude that is build on top of the HTML5 boilerplate and the Basic theme.

It also include some extra features like collapsible blocks with cookies, setting to fixed or fluid width thought the settings page, and dynamic font replacement with google web-fonts.

Screenshot attached.

Comments

Arnold Leung’s picture

Status: Active » Needs review
laura s’s picture

Status: Needs review » Needs work

Overall this looks pretty good. In IRC @Heine and I noted a potential issue with passing args. I posted in the project issue queue.

EricoVinicius’s picture

Status: Needs work » Needs review

Args issue fixed.

EricoVinicius’s picture

Status: Needs review » Fixed

Args issue fixed.

Arnold Leung’s picture

Status: Fixed » Needs review

Still needs reviewed before marking as RTBC..

Arnold Leung’s picture

Priority: Normal » Major

Any news with this?

meba’s picture

Status: Needs review » Needs work

I seriously dislike this in template.php:

    $body_classes[] = 'tax-' . eregi_replace('[^a-z0-9]', '-', $term->name);

Apart from the fact that ereg was deprecated, I don't want to think about the fact that $term->name is insecure. It is mitigated by the ereg but still, we don't want to open a can of worms

Once this is fixed, I give it RTBC

EricoVinicius’s picture

Status: Needs work » Needs review

Updated to:

	$body_classes[] = 'tax-' . preg_replace('/[^a-z0-9]/i', '-', check_plain($term->name));
meba’s picture

Status: Needs review » Reviewed & tested by the community

Rtbc imho

dave reid’s picture

Status: Reviewed & tested by the community » Needs work

Looks like there is a possible XSS vulnerability - the footer_copyright() does not escape the site_name variable:

  12 function vintage_preprocess_page(&$vars, $hook) {
  13 
  14         $vars['footer_copy'] = footer_copyright();

 471  function footer_copyright(){
 472     $year = '' ;
 473     $time = time () ;
 474     //This line gets the current time off the server
 475     $year .= date("Y",$time);
 476     //This line formats it to display just the year
 477     return t('Copyright ') . $year . ' ' . variable_get('site_name', 'drupal') . '.';
 478 }
EricoVinicius’s picture

Status: Needs work » Needs review

Updated line 477 to:
return t('Copyright ') . $year . ' ' . check_plain(variable_get('site_name', 'drupal')) . '.';

EricoVinicius’s picture

Thanks for all the feedback guys, its been a very interesting experience. Good to be involved with the community.
Any more info on this? Are we ready to go?

Thx!
Erico Nascimento

EricoVinicius’s picture

Status: Needs review » Fixed

No more feedback?

Arnold Leung’s picture

Priority: Major » Critical
Status: Fixed » Needs review
sreynen’s picture

Priority: Critical » Normal

Everything is normal priority in this queue.

sreynen’s picture

Issue tags: +PAReview: Theme

Tagging for easier finding.

joachim’s picture

Status: Needs review » Needs work
 	// Don't display empty help from node_help().
  if ($vars['help'] == "<div class=\"help\"><p></p>\n</div>") {
    $vars['help'] = '';
  }

That's not a theme's job.

  if (module_exists('taxonomy') && $vars['node']->nid) {
    foreach (taxonomy_node_get_terms($vars['node']) as $term) {
			$body_classes[] = 'tax-' . preg_replace('/[^a-z0-9]/i', '-', check_plain($term->name));
    }
  }

That hits the database, and if you have a $node in the vars, then it already contains the taxonomy terms!
Also, why the preg if you have vintage_id_safe() to hand?

          array_pop($body_classes); // Remove 'section-node'

A lot of work round here just to clean up classes on node add / edit / delete. Is ability to theme those pages so important?

  if ($vars['node']->type != "") {

This bunch of checks needs to be inside a check that node exists.

- Bad indentation in vintage_preprocess_block() and vintage_links()

- Review how you lay out docblock comments -- only one space after the initial * and no blank line before the function declaration.

- vintage_breadcrumb missing docblock

Overall, I'm not sure this qualifies as a *starter* theme... there's a lot in there that I'd find I don't need or would outright have to revert.

EricoVinicius’s picture

Status: Needs work » Needs review

Removed:

 	// Don't display empty help from node_help().
  if ($vars['help'] == "<div class=\"help\"><p></p>\n</div>") {
    $vars['help'] = '';
  }

foreach (taxonomy_node_get_terms($vars['node']) as $term) {
updated with
foreach (($vars['node']->taxonomy) as $term) {

Replaced: preg_replace() with vindage_id_safe();
Indentation fixed.
docblocks fixed

sarah_p’s picture

Component: new project application » theme
sarah_p’s picture

EricoVinicius’s picture

Any more feedback?

ccardea’s picture

Status: Needs review » Reviewed & tested by the community

Indentation went from bad to worse. Each indent should be 2 spaces. I don't think we need to hold up this application for indentation issues. Several people have looked at this and all of the issues have been addressed. It would be nice if reviewers would follow up.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

If they think that the indentation was fixed, it becomes a bigger problem than if it was just wrong.

klausi’s picture

Status: Needs work » Closed (won't fix)

No activity in several months. Reopen and set the status to "needs review" if you are still pursuing this application.

Arnold Leung’s picture

Status: Closed (won't fix) » Needs review
andrewyager’s picture

StatusFileSize
new37.38 KB

Patch applied to fix indentation, the issue is most likely that their editor replaced double spaces with \t characters.

andrewyager’s picture

Status: Needs review » Needs work

The above patch does not fix all of the code formatting problems; check for tabs that should be spaces. Also there was a number of relatively minor formatting issues in your template file. The coder module (drupal.org/project/coder) will help you find the specific formatting issues.

It also appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

Good work though :) Keep at it!

misc’s picture

@EricoVinicius has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

greggles’s picture

Issue tags: +PAreview: security

For #10.

klausi’s picture

Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.