0
\$\begingroup\$

I've got some experience with Laravel but for fun I started a small project (basically a CRUD with also Google Maps) without a framework because I considered that is oversized.

This is the directory structure.

+---assets
|   +---css
|   +---img
|   \---js
+---classes
+---inc
|   +---templates
|   \---views
|       \---partials
+---libs
\---pages
- index.php
- config.php
- initialize.php
- .htaccess
- robots.txt

In pages folder there are the equivalent of MVC Controllers, every file is related to a precise url (route-like).

In .htaccess I re-wrote all requests to index.php except for the assets folder and robots.txt.

RewriteEngine on
RewriteCond $1 !^(index\.php|assets|robots\.txt)
RewriteRule ^(.*)$ index.php/$1 [L]

This is the index.php:

<?php
require_once 'initialize.php';

//slashes for trim() function
$slashes = '\/';

//get the request uri without slashes and query string
$uri = trim(strtok($_SERVER['REQUEST_URI'],'?'), $slashes);

//get the base url from config var
$base_url = config_get('base_url');

//if needed, remove the base url from request uri
if (!empty($base_url) && (substr($uri, 0, strlen($base_url)) == $base_url)) {
    $uri = substr($uri, strlen($base_url));
    $uri = trim($uri, $slashes);
}

// empty request uri = home page
if(empty($uri)){
    $view = new View('home', 'main');
    $view->setTitle('Home')->render();
}else{

    //routing basic
    switch ($uri){
        case 'bts/create':
            require PAGES_PATH . 'bts_add.php';
            break;

        case 'bts/create/save':
            require_once PAGES_PATH . 'bts_add_save.php';
            break;

        case 'bts/edit':
            require_once PAGES_PATH . 'bts_edit.php';
            break;

        case 'map':
            require_once PAGES_PATH . 'map.php';
            break;

        case 'map/api':
            require_once PAGES_PATH . 'map_api.php';
            break;

        default:
            show_404();
            break;
    }
}

Here initialize.php:

<?php
session_start();

require_once 'config.php';

foreach (glob(APP_PATH . '/libs/*.php') as $file){
    require_once $file;
}

spl_autoload_register(function($class){

    $path = APP_PATH . '/classes/' . $class . '.php';

    if(file_exists($path)){
        require_once $path;
    }
});

set_exception_handler(function($e){
    if (ob_get_level()){
        ob_end_clean();
    }

    $page =  <<<HTML
            <!doctype html>
            <head>
            <title>App error</title>
            </head>
            <body>
                <h1>There is a problem</h1>
                <p>{$e->getMessage()}</p>
                <p><strong>File:</strong> {$e->getFile()}</p>
                <p><strong>Line:</strong> {$e->getLine()}</p>
            </body>
HTML;
    @http_response_code(500);
    die($page);
});

Config.php:

<?php
define('APP_PATH', __DIR__);
define('PAGES_PATH', APP_PATH . '/pages/' );
define('INC_PATH', APP_PATH . '/inc/' );



$config = array(
    'base_url' => 'my_project/cells',
    'db' => array(
        'host' => 'localhost',
        'username' => 'root',
        'password' => '',
        'db_name' => 'cell_database'
    ),

);

Basic functions in libs folder:

<?php
/**
 * Shows 404 page with HTTP status code
 */
function show_404(){
    http_response_code(404);
    $view = new View('404', 'main');
    $view->setTitle('Not found');
    $view->render();
    die();
}

/**
 * 
 * @global array $config
 * @param string $key
 * @return mixed
 * 
 * Returns a value from config array
 */
function config_get($key){
    global $config;

    return (isset($config[$key]) ? $config[$key] : null);
}

/**
 * 
 * @param string $path
 * @param array $qs - The query string (optional)
 * @return string
 * 
 * Generates full url from given path
 */
function url($path, $qs = array()){
    $path = trim($path, '\/');
    $url = ('//' . $_SERVER['HTTP_HOST'] . '/' . (config_get('base_url') ? config_get('base_url') . '/' : '') . $path);

    if(is_array($qs) && $qs){
        $url .= '?' . http_build_query($qs, '&amp');
    }

    return $url;
}

/**
 * returns full url of an asset resource
 * 
 * @param string $file
 * @return string
 */
function asset($file){
    return url('assets/' . $file);

}


/**
 * 
 * @param string $url
 * 
 * Redirects to given url
 */
function redirect($url){
    http_response_code(301);
    header("Location: $url");
    die();
}

View Class:

<?php

/**
 * Class for view rendering
 *
 */
class View {

    const GMAP_SCRIPT = '<script async defer
        src="https://maps.googleapis.com/maps/api/js?key=blabla&libraries=places&callback=initMap">
</script>';

    protected $view, $template, $title, $variables = [], $js = [], $css = [], $gmap = false;

    /**
     * 
     * @param string $view - view name
     * @param string $template - template name
     * @param array $variables - index array of variables
     */
    public function __construct($view, $template, $variables = []) {
        $this->view = INC_PATH . 'views/' . $view . '.php';
        $this->template = INC_PATH . 'templates/' . $template . '.php';
        $this->variables = $variables;
    }

    /**
     * 
     * @param string $title - page title
     * @return \View
     */
    public function setTitle($title) {
        $this->title = $title;

        return $this;
    }

    /**
     * 
     * @param array $js - array of javascrips
     * @return \View
     */
    public function setJs($js) {
        $this->js = $js;

        return $this;
    }

    /**
     * 
     * @param array $css - array of css
     * @return \View
     */
    public function setCss($css) {
        $this->css = $css;

        return $this;
    }

    /**
     * Includes gmap script
     * 
     * @return \View
     */
    public function includeGmap(){
        $this->gmap = true;

        return $this;
    }

    /**
     * Render the view and send to output buffer
     *  
     * @param array $variables
     */
    public function render() {
        extract($this->variables);

        ob_start();

        if (isset($this->template) && is_file($this->template)) {
            require_once $this->template;
        } else {
            throw new Exception('Template is invalid');
        }

        ob_end_flush();
    }

}

DB singleton class.

<?php

Class Db{

    private static $pdo;

    public static function getConnection(){

        if(!isset(self::$pdo)){
            self::$pdo = new PDO('mysql:dbname='. config_get('db')['db_name'].';host=' . config_get('db')['host'], config_get('db')['username'], config_get('db')['password']); 
        }

        return self::$pdo;
    }

    private function __construct(){}
    private function __clone() {}

}

My questions are:

  • Is MVC pattern suggested also in small PHP projects?
  • If not, is my app structure good or bad?
  • If bad, what to do to make it better?
\$\endgroup\$
1
\$\begingroup\$

Thoughts:

  • Consider moving classes, includes, config, and initialization out of web directory. There is no reason for them to be there. Eventually if you get to the point of putting your static assets on a CDN, you could conceivably only have your index.php file and robots, sitemaps, etc. files in the web directory.
  • Consider configuring rewrite directly in Apache .conf file rather than .htaccess and turn off the ability to override this config in a per-directory context, as you have no need for it with this sort of front controller design. this is also better from a performance standpoint in that the web server doesn't have to look for .htaccess and read it for every request.
  • Consider not placing DB credentials in code if feasible (i.e. inject them into environment upon environment start up.
  • Your $config array is problematic in that it is mutable. Typically application configuration should not be mutable at run time. Consider sticking with constants or using a class with constants that are statically accessible.
  • You have a reasonable start to a framework of your own. I would encourage you to think about how to better decouple application-specific configuration from the basic workings of the framework. This would allow you to better leverage this code on future projects/applications. For example, consider moving the route definitions into configuration. Be more consistent in how you display error pages relative to configured pages. You should be able to supply your own templates for these error pages and not have the layout so tightly coupled to the framework portion of the application.
  • You don't currently appear to have any consideration for GET vs. POST. Any complex application would need to consider this as part of the routing mechanism, as you may need to use different controllers for the different HTTP methods. This could even extend to the other HTTP verbs - PUT, DELETE, HEAD, etc. if you were looking to expand the framework to work as a RESTful API framework.
  • It would be nice to see one or more of your "page" include files to see what those look like as well.

I have added more context-specific feedback within multi-line comments below.

initialize.php

session_start();
/*
Consider having config as very first thing you load.  What happens if you
change your session persistence mechanism at some point? You may have some
configuration around it that needs to be loaded before the session starts
*/
require_once 'config.php';

/*
This is a little sloppy and potentially dangerous. It is basically the
equivalent of a * import in other languages, which are generally frowned
upon.
Be specific about the dependencies your application needs.  Fail if those
dependencies are not fulfilled. You would never know in this case if one of
your libraries was accidentally deleted from this directory and your app
is not set up properly.
Consider putting dependency information in your app configuration and load
those dependencies here.
*/
foreach (glob(APP_PATH . '/libs/*.php') as $file){
    require_once $file;
}

/*
I have have mixed feelings about autoload, as it is wasteful of resources to do
disk access every time you need to see if a class is available to load.
It is also similar to my comment above in that you then defer loading of 
potentially hard dependencies to further down in the call stack,
when failing fast if a dependency is missing is usually the best bet.
I would suggest perhaps manually requiring those classes you know will be needed
for every (or almost every) request and perhaps only use autoloader for classes
that might be used conditionally.
*/ 
spl_autoload_register(function($class){

    $path = APP_PATH . '/classes/' . $class . '.php';

    if(file_exists($path)){
        require_once $path;
    }
});

/*
I would think that, at a minimum, you would want to log your exception
from within this function as well.
*/
set_exception_handler(function($e){
    if (ob_get_level()){
        ob_end_clean();
    }
/*
Consider splitting out end user display concerns from the function itself.
You are using display templates elsewhere, why not for for this as well?
If you are using heredoc syntax, go ahead and reset indentation to make code
easier to read.
*/
    $page =  <<<HTML
            <!doctype html>
            <head>
            <title>App error</title>
            </head>
            <body>
                <h1>There is a problem</h1>
                <p>{$e->getMessage()}</p>
                <p><strong>File:</strong> {$e->getFile()}</p>
                <p><strong>Line:</strong> {$e->getLine()}</p>
            </body>
HTML;
    @http_response_code(500);
    die($page);
});

/*
Perhaps start session here, as nothing to this point should depend on it.
*/

index.php

<?php
require_once 'initialize.php';

/*
Consider using redirect at server level to make sure that you don't get trailing
slashes.  This also tells caller (through a 301 header) what the correct URL is.
*/
//slashes for trim() function
$slashes = '\/';

//get the request uri without slashes and query string
/*
Consider parse_url() for use throughout follwing section.
No need to trim if you enforce proper URI format at server level as noted above.
*/
$uri = trim(strtok($_SERVER['REQUEST_URI'],'?'), $slashes);

//get the base url from config var
$base_url = config_get('base_url');

//if needed, remove the base url from request uri
if (!empty($base_url) && (substr($uri, 0, strlen($base_url)) == $base_url)) {
    $uri = substr($uri, strlen($base_url));
    $uri = trim($uri, $slashes);
}

/*
Why is empty URI (home page) not part of the rest of your route configuration?
I don't think it makes sense to split this out.
*/
// empty request uri = home page
if(empty($uri)){
    $view = new View('home', 'main');
/*
Should view not set it's title as part of render() rather than leaving this
to calling code?
*/
    $view->setTitle('Home')->render();
}else{

/*
Consider building proper routing class with routes and related
includes derived from configuration.
*/
    //routing basic
    switch ($uri){
        case 'bts/create':
            require PAGES_PATH . 'bts_add.php';
            break;

        case 'bts/create/save':
            require_once PAGES_PATH . 'bts_add_save.php';
            break;

        case 'bts/edit':
            require_once PAGES_PATH . 'bts_edit.php';
            break;

        case 'map':
            require_once PAGES_PATH . 'map.php';
            break;

        case 'map/api':
            require_once PAGES_PATH . 'map_api.php';
            break;

        default:
/*
Why treat 404 differently (i.e. making function call)?
To me the 404 page should be a configured route with same sort of 
treatment as other pages.
*/
            show_404();
            break;
    }
}

functions

<?php
/**
 * Shows 404 page with HTTP status code
 */
function show_404(){
    http_response_code(404);
    $view = new View('404', 'main');
    $view->setTitle('Not found');
    $view->render();
    die();
}

/**
 * 
 * @global array $config
 * @param string $key
 * @return mixed
 * 
 * Returns a value from config array
 */
function config_get($key){
/*
Globals are bad.  Again, if you used constants or statically accessible class
method you eliminate the need for this (and make your config immutable).
You could probably drop this function altogether as well.
For a globally accessible function like this, you should validate that you 
are getting valid input (in this case non-zero-length string) and throw
InvalidArgumentException if validation fails.
*/
    global $config;
/*
Why return null?  If you are missing a config that you expect to be there, I
would expect an Exception to be thrown, possibly causing application to halt
depending on whether the exception is recoverable (probably up to calling code
to determine that).
*/
    return (isset($config[$key]) ? $config[$key] : null);
}

/**
 * 
 * @param string $path
 * @param array $qs - The query string (optional)
 * @return string
 * 
 * Generates full url from given path
 */
/*
What is $qs? Please use meaningful variable name.
*/
function url($path, $qs = array()){
/*
Validate parameters. Perhaps enforce array for $qs via type hinting. I don't
see any reason why this function needs to deal with either an array or a string
here for this parameter. A single string can easily be represented as array with
single value.  I actually don;t see how a single string would work anyway as
you need to enforce key-value pairs for a parameter.
*/
    $path = trim($path, '\/');
/*
This line of code is really hard to read. Too long and questionable use of
ternary operator.
*/
    $url = ('//' . $_SERVER['HTTP_HOST'] . '/' . (config_get('base_url') ? config_get('base_url') . '/' : '') . $path);

    if(is_array($qs) && $qs){
        $url .= '?' . http_build_query($qs, '&amp');
    }

    return $url;
}

/**
 * returns full url of an asset resource
 * 
 * @param string $file
 * @return string
 */
/*
Is this really a full url as per comments above?  Looks like relative URI to me.
Should asset path be a constant in your configuration?
*/
function asset($file){
/*
Validate parameter.
*/
    return url('assets/' . $file);

}


/**
 * 
 * @param string $url
 * 
 * Redirects to given url
 */
/*
If you build proper router class. This should probably be method in that class.
*/
function redirect($url){
/*
Validate $url
*/
    http_response_code(301);
    header("Location: $url");
    die();
}

View.php

<?php

/**
 * Class for view rendering
 *
 */
class View {

/*
I don't think Google Map specific stuff should be included in a generic View
class.  Why wouldn't a caller instantiating this class simply inject this
javascript into the template using setJs() method? You may consider a totally
separate class to set up this script as necessary.  I also question whether
you really need this in PHP at all.  Could this not simply be a javascript
this is conditionally included on appropriate pages as you would for any other
javascript includes?
*/ 
    const GMAP_SCRIPT = '<script async defer
        src="https://maps.googleapis.com/maps/api/js?key=blabla&libraries=places&callback=initMap">
</script>';

/*
Put each property on it's own line to improve readability.
Use more descriptive names for array properties like variableArray, jsArray,
cssArray, etc. so that is really clear what you are working with here.
*/
    protected $view, $template, $title, $variables = [], $js = [], $css = [], $gmap = false;

    /**
     * 
     * @param string $view - view name
     * @param string $template - template name
     * @param array $variables - index array of variables
     */
/*
Use array typehint for $variables parameters to enforce that you must
get array.
*/
    public function __construct($view, $template, $variables = []) {
/*
Validate your parameters and fail with InvalidArgumentException if validation
fails.
Consider validating existence of the view and template files here and failing
if they are not present and readable. Again, you want to fail fast if this code
has been passed bad arguments.
It might make sense to have specific helper methods on this class like
validateView and validateTemplate for this purpose.
*/
        $this->view = INC_PATH . 'views/' . $view . '.php';
        $this->template = INC_PATH . 'templates/' . $template . '.php';
        $this->variables = $variables;
    }

    /**
     * 
     * @param string $title - page title
     * @return \View
     */
/*
Should the title not be configured as part of the view?
*/
    public function setTitle($title) {
        $this->title = $title;

        return $this;
    }

    /**
     * 
     * @param array $js - array of javascrips
     * @return \View
     */
/*
Type hint for array here. Validate that array is non-zero length. Perhaps
use better variable name like jsArray.
Consider documentation in you doc block above that this method is destructive
to any existing array that may be stored on the object - you are not pushing
new js onto existing array.
*/
    public function setJs($js) {
        $this->js = $js;

        return $this;
    }

    /**
     * 
     * @param array $css - array of css
     * @return \View
     */
/*
Same comments as for setJs above.
*/
    public function setCss($css) {
        $this->css = $css;

        return $this;
    }

    /**
     * Includes gmap script
     * 
     * @return \View
     */
/*
Again not sure why external caller wouldn't just load Gmap-specific js with
other JS.
*/
    public function includeGmap(){
        $this->gmap = true;

        return $this;
    }

    /**
     * Render the view and send to output buffer
     *  
     * @param array $variables
     */
    public function render() {
/*
extract() is really problematic.  Do you really want to pollute the global 
namespace with these variables? Someone working with templates may define a
template variable name that could overwrite one of your critical variables.
Additionally, you are not using extract() around your javascript and css arrays
which I assume are also being injected into your template, so why introduce
two different ways to access View data in your template?
*/
        extract($this->variables);
/*
Why do you need to buffer output here? What do you gain by doing this?
This utilizes more memory in your application than is needed.
*/
        ob_start();
/*
Template should be validated for existence in constructor. You shouldn't even
be able to call render() on a view object that could not be correctly
instantiated.
*/
        if (isset($this->template) && is_file($this->template)) {
            require_once $this->template;
        } else {
            throw new Exception('Template is invalid');
        }

        ob_end_flush();
    }

}

Db.php

<?php
/*
Consider more meaningful class name like PDOSingleton. Use doc blocks in this
code as well.
*/
Class Db{

    private static $pdo;

/*
Perhaps more meaningful name like getPDO so all callers are clear as to what
sort of object is being returned.
*/
    public static function getConnection(){
/*
This only considers happy path for PDO instantiation.  What if instantiation
fails?  Consider using PDO in exception mode to be consistent with your approach
of using exceptions throughout.
Consider using following as your test, as it is more specific.
if(self::$pdo instanceof PDO === false)
*/
        if(!isset(self::$pdo)){
/*
This line of code is way too long and hard to read.  Is you make changes to 
config approach as noted earlier this might help, but you still might need
to break this up across multiple lines.
You should not assign PDO object to self::$pdo until you know instantiation
was successful.
*/
            self::$pdo = new PDO('mysql:dbname='. config_get('db')['db_name'].';host=' . config_get('db')['host'], config_get('db')['username'], config_get('db')['password']); 
        }

        return self::$pdo;
    }

    private function __construct(){}
/*
I really like that you are thinking of preventing clone operation here. You
don't always see this in singleton implementation.  Consider throwing exception
here in this method if you truly want to discourage this behavior in your
application.
*/
    private function __clone() {}

}
\$\endgroup\$
  • 1
    \$\begingroup\$ @ClaudioKing I have updated answer from yesterday with some more general thoughts and code-specific feedback. \$\endgroup\$ – Mike Brant Sep 1 '16 at 13:43
  • \$\begingroup\$ Thank you very much, your explanation is very accurate. I'll consider to switch to a pre-built framework in order to avoid bad pratices. \$\endgroup\$ – Claudio King Sep 1 '16 at 21:42
  • 1
    \$\begingroup\$ @ClaudioKing This kind of exploration is good for development. I thought this was good start for someone diving into framework type of development. That is why I wanted to give as much specific feedback as possible, not to discourage further exploration . Certainly the various frameworks out there are good for helping developers accelerate their ability to build more complex apps, but at some point can be limiting to an individual's progress as a developer. \$\endgroup\$ – Mike Brant Sep 2 '16 at 15:20

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.