6
\$\begingroup\$

I am trying a very simple method of getting the theme information via a theme.xml within each theme folder.

What it should do:

  • Scan the theme directory ../themes
  • Return the names of all the theme folders (array)
  • Use the name of the theme folder returned to create a link to search in file_exists()
  • Find the theme.xml in every folder
  • Parse every theme.xml with simplexml_load_file()
  • Return an array ready for output in a foreach()
  • If !file_exists(), output "theme.xml Not Found in (themename)"

I tried to make this in 2 functions but it would always output the first or last themes info, so this is what I'm doing now, can anyone suggest a way to help separate the function from the output.

Get available theme folder names function

 public function getAvailableThemes() {
    $path = '../themes';
    $themeDir = glob($path . '/*' , GLOB_ONLYDIR);
    return $themeDir;
}

Output in wcx_themes.php file

    <?php 
    $themeDir = $backend->getAvailableThemes(); 
    foreach($themeDir as $themeDir) {
        $preview = $backend->getThemePreview($themeDir);
        $xmlfile = $themeDir.'/theme.xml';
    if(file_exists($xmlfile)) {
        $xml = simplexml_load_file($xmlfile);
        $themename = $xml->themename;
        $themedescription = $xml->themedescription;
        $themeversion = $xml->themeversion;
        $themeauthor = $xml->themeauthor;

    echo '<div class="themewrap">
    <div class="themepreview">
    <img src="'.$preview.'"/>
    </div>
    <div class="themeinfo">
    <div class="themename">Name:&nbsp;'.$themename.'</div>
    <div class="themedescription">Description:&nbsp;'.$themedescription.'</div>
    <div class="themeversion">Version:&nbsp;'.$themeversion.'</div>
    <div class="themeauthor">Author:&nbsp;'.$themeauthor.'</div>
    </div>
    <div class="themebar">
    <div class="themebarname">'.$themename.'</div>
    <div class="themebutton">
    <a href="../controllers/themecontrol.php">INSTALL</a>
    </div>
    </div>
    </div>';
    } 
    else {
    echo ' 
    <div class="themewrap">
    <div class="themepreview">
    <img src="'.$backend->getThemePreview($themeDir).'"/>
    </div>
    <div class="themeinfo">
    <div class="themenoexist">THEME.XML DOES NOT EXIST</div>
    </div>
    </div>';    
    }
    }
    ?>

Get theme preview image

    public function getThemePreview($theme) {
    $ext = '.png';
    $preview = '../themes/'.$theme.'/preview'.$ext;
    if(file_exists($preview)) {
       return $preview;
    }
    else {
      $preview = 'images/nopreview'.$ext;
    return $preview;    
   }
   }

Theme.xml

<?xml version="1.0" encoding="utf-8"?>
 <theme>
  <themename>Default</themename>
  <themedescription>This is the Default Theme</themedescription>
  <themeversion>1.0</themeversion>
  <themeauthor>WCX</themeauthor>
 </theme>
\$\endgroup\$
4
\$\begingroup\$

Yes you can move the output away from the code, however that's usually difficult to do with this type of procedural code. The code is extremely tightly bound, which makes separation harder than it should be!

The two function you call are fine as they are. So lets focus on those for now.

getAvailableThemes()

This function, since it's only being called once, is what I believe to be clutter. I don't see a need to separate it from the main logic. Doing so only increases the amount of times we need to look back at the code.

Also, I see no reason to cut of "Directory" in $themeDir. Spell it out for clarity.

getThemePreview()

  • Is the return value a "preview" or a "file"? Perhaps a better name would be getThemePreviewFile.
  • We can refactor the code a lot:

    $preview = '../themes/'.$theme.'/preview.png';
    
    if(!file_exists($preview)) {
        $preview = 'images/nopreview.png';
    }
    return $preview;    
    

    I shortened it, and I added the extension into the literal because it doesn't look like the extension will change.

Now for the main file!

wcx_themes.php

First off, indentation should be key here. Indentation is also missing here! Readability is very important.

  • foreach($themeDir as $themeDir) - I don't like this at all. Collections should be pluralized.
  • The immediate solution to separating the HTML and the PHP that I can think of, is a templating library. Other than that, you could store the HTMl in a string, and then use sprintf or another form of replacement to build the output.
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks Alex, i don't remember seeing this answer / notification.. :/ \$\endgroup\$ – CodeX Sep 2 '14 at 10:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.