1
\$\begingroup\$

I had previously written a very simple CMS in PHP, but it wasn't very good looking so I decided to rewrite and improve it using the PHP standards.

Have I done this well, and what areas can be improved?

<?php
class ContentManager {
    private $db;
    public $websiteInfo = array();
    public $pageInfo = array();
    public $navigationBar;
    //array of css files that should be included into the pages
    public $stylesheets = array("style.css");
    //set current page if no parameter given
    public $current_page = "main";

    public $doubleNavigationPages = array("user", "admin");

    // this should be a valid link: $page_base_url . $_GET['id']
    public $page_base_url = "/?id=";

    public function __construct($db) {
        $this->db = $db;
            //maybe this into a function?
            $stmt = $this->db->prepare("SELECT * FROM settings");
            $stmt->execute();
            $this->websiteInfo = $stmt->fetch();

            $this->getNavigationBar();

            //set page info
            if($this->get('id') !== null) {
                $this->current_page = strtolower($this->get('id'));
            }
            $this->getPage($this->current_page);

            return true;
    }


    //get navigationbar data from db and put it into the var $this->navigation_bar
    public function getNavigationBar() {...}
    //get page data from db and put it into the array $this->pageInfo
    public function getPage($pageId) {...}

    public function get($key) {
        if(isset($_GET[$key])) {

            return $_GET[$key];
        }

        return null;
    }

}
?>

And here is the HTML/template/output processing part (I don't know what to call it). It processes the dynamic content, the header, the navigation bar, the page content, and the footer.

<?php
$db = new PDO("mysql:host=LALALA;dbname=LALALA;", "LALALA", "LALALA");
$ContentManager = new ContentManager($db);
if($ContentManager->pageInfo['dynamic_page']) {
    include($ContentManager->pageInfo['dynamic_file']);
} ?>
<!DOCTYPE html>
<html>
<head>
    <title><?=$ContentManager->pageInfo['title'];?></title>
    <link rel="icon" type="icon/ico" href="/favicon.ico" />
    <?php foreach($ContentManager->stylesheets as $css_file) { ?><style type="text/css"><?=str_replace(array("\r\n", "\r", "\n", "\t", '  ', '    ', '    '), '', str_replace(': ', ':', preg_replace('!/\*[^*]*\*+([^/][^*]*\*+)*/!', '', file_get_contents($css_file))));?></style><?php } ?>
</head>
<body>
<div id="wrap">
    <h1 id="title"><a href="//<?=$_SERVER['HTTP_HOST'];?>"><?=$ContentManager->websiteInfo['name'];?></a></h1>
    <ul id="nav" class="nav"><li><a href="/"<?php if($ContentManager->current_page == 'main') { ?> id="active"<?php } ?>>Home</a></li>

        <?php foreach($ContentManager->navigationBar as $page) { ?><li<?php if($ContentManager->current_page == $page['id']) {?> id="active"<?php } ?>>
            <a href="<?php if(!$page['external_url']) { echo $ContentManager->page_base_url; } echo $page['id'];?>"><?=$page['title'];?></a>
        </li><?php } ?>
    </ul>

    <?php 
    if(!in_array($ContentManager->current_page, $ContentManager->doubleNavigationPages)) { ?>
        <div id="main">
    <?php 
    }
    include($ContentManager->pageInfo['file']); 
    ?>

    </div>
    <div id="footer">
        <p> Copyright &copy; <?=date("Y");?> - <a href="//<?=$_SERVER['HTTP_HOST'];?>"><?=$ContentManager->websiteInfo['name'];?></a></p>
    </div>
</div>
</body>
</html>
\$\endgroup\$
3
\$\begingroup\$

This is not what I think most would call a "content management" class, but rather a data provider for a rudimentary templating tool.

This is probably not something I would be thinking about using as a foundation for future projects, as there are way better, more complete tools for both content management and templating out there that would do a lot better job of separating your content from your template. The two are tightly coupled as it stands now, as a change in either the class or the template probably necessitates a change in both. That being said, as a way to inject some dynamic content into simple web pages, I think what you have is workable.

As such, I am going to focus my comments not so much on the design, which I think has some flaws/limitations, but rather on the practical aspects that you as a (presumably) learning programmer can apply to future code.

  • Your class is doing too much including: providing site-wide config, getting content for insertion into templates, dealing with user input for parameters passed via GET, sorting list of stylesheets used by the site, etc. As you gain programming experience you will find that classes and methods on classes are best geared to do one specific thing or set of things.
  • Your get() method adds zero value. The caller could do the exact same operations against $_GET directly.
  • I really like that you are passing the DB dependency to this class. That is a good pattern (dependency injection) to use across a broad range of use cases where a class needs some external dependency. You do however do absolutely nothing to enforce that you are getting a valid object to operate against. You also can't tell what $db is from looking at this code. Is it a PDO object? Mysqli object? Some other DB object defined elsewhere in your system? Someone looking at this piece of code (and perhaps modifying it) would have to go back up the call stack to figure out what it is working with here. If for example you are expecting a PDO object (as you are when you look at calling code), you should a) type hint your parameter as PDO object and b) rename your property $pdo or similar to clearly indicate what dependency is stored there.
  • Your PDO database interactions assume happy path execution, doing no validation at all that, for example, statement preparation was successful. This is going to lead to fragile and hard to debug code. Any time you instantiate an object, call a function, etc. you should make sure you understand all the possible exceptions that could be thrown and values that could be returned and make sure you make reasonable provision in your code to handle those cases.
  • Don't use 'SELECT *'. It is a terribly lazy programming habit, which makes your code harder to read and understand (what fields do I have to work with?), more fragile (you mean that new field I added into DB broke something?), and can consume more bandwidth than necessary (do I really need to send all this data if I am only using one or two fields?).
  • I agree with your comment in constructor that perhaps some of that code needs to go into class methods. I could foresee something like this.

Example:

public function __construct(PDO $pdo) {
    $this->pdo = $pdo;
    $this->loadConfig();
    $this->loadPage();
}
  • Why is your constructor returning true?
  • Be consistent about using either snake_case or camelCase within your own code. I know PHP is bad about this, but it doesn't mean you should be. Develop a consistent coding style. What you have here is lack of any coherent coding style.
  • Consider visibility of your properties carefully. You should ask yourself whether something like current page, stylesheets, etc. should be mutable after this object is instantiated. If you don't have a valid reason for allowing calling code to change these values, make the values protected/private and provide getters to allow for reading the data. My guess is that many of your properties should be made immutable.
  • Why would you read in and echo out your stylesheet content rather than just providing link to stylesheet, as would be considered best practice?
  • I find your template very difficult to read, but this may just be my opinion. While I think it OK to have some PHP directly in-line with HTML (for example for simple variable injections), there are several cases where where you have more complex flow control structures and additional PHP variable prep work that you inline with HTML, making things look cluttered.

I will take this example:

<head>
    <title><?=$ContentManager->pageInfo['title'];?></title>
    <link rel="icon" type="icon/ico" href="/favicon.ico" />
    <?php foreach($ContentManager->stylesheets as $css_file) { ?><style type="text/css"><?=str_replace(array("\r\n", "\r", "\n",
"\t", '  ', '    ', '    '), '', str_replace(': ', ':',
preg_replace('!/\*[^*]*\*+([^/][^*]*\*+)*/!', '',
file_get_contents($css_file))));?></style><?php } ?>
</head>

This might look much cleaner like:

<head>
    <title><?=$ContentManager->pageInfo['title'];?></title>
    <link rel="icon" type="icon/ico" href="/favicon.ico" />
<?php
foreach($ContentManager->stylesheets as $css_file) {
?>
    <link rel="stylesheet" type="text/css" href="<?=$css_file;?>">
<?php
}
?>
</head>
  • You do nothing to sanitize your output from DB against XSS vulnerabilities.
  • You do nothing to validate parameters on your public class methods. You should always consider making sure that caller is passing you valid values before working with them.
\$\endgroup\$

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.