PHP-framework with MVC architecture and Active Record pattern for the DB management

I tried to create the PHP-framework, with no experience in the task like this:

Folders:

www/
  |-- protected/
  |      |-- controllers/
  |      |      |-- Site.php
  |      |
  |      |-- core/
  |      |      |-- App.php
  |      |      |-- Controller.php
  |      |      |-- Criteria.php
  |      |      |-- Model.php
  |      |      |-- View.php
  |      |      |-- Pagination.php
  |      |     
  |      |-- models/
  |      |-- views/
  |      |-- inip.php
  |      |-- config.php
  |
  |-- index.php
  |-- .htaccess

.htaccess

AddDefaultCharset utf-8

RewriteEngine On  
RewriteCond %{REQUEST_FILENAME} !-f  
RewriteCond %{REQUEST_FILENAME} !-d  
RewriteRule .* index.php [L]

index.php

require_once __DIR__.'/protected/init.php';

//Application start (singleton)
App::getInstance($connectParams);
App::route();

config.php

define('CORE_DIR', __DIR__.'/core/');
define('MODEL_DIR', __DIR__.'/models/');
define('CONTROLLER_DIR', __DIR__.'/controllers/');
define('VIEW_DIR', __DIR__.'/views/');
//DB connection params
$connectParams = array(
    'user' => 'root',
    'pass' => '123456',
    'host' => 'localhost',
    'dbname' => 'test',
    );

init.php

require_once __DIR__.'/config.php';
//Classes autoload
spl_autoload_register('autoload');
function autoload($className)
{
    $classFile = false;
    $fileName = $className.".php";
    $paths = array(
        CORE_DIR,
        MODEL_DIR,
        CONTROLLER_DIR,
        VIEW_DIR,
        );

    foreach ($paths as $path) {
        if (file_exists($path.$fileName)) {
            $classFile = $path.$fileName;
            break;
        }
    }

    if ($classFile!==false) {
        include $classFile;
    }
    else {
        die('File for class '.$className.' not found!');
    }
}

core/App.php

```
class App
{
protected static $instance;
public static $dbHandler;

//Getting an App singleton
public static function getInstance($connectParams=array())
{
if (

Alright... here it goes! I'll section each part by the headings you gave.

Folders

This all looks fine and dandy. I haven't seen any rules or anything to prevent the public from accessing protected though. Maybe I missed it, but it should be obvious this needs to be accessible only by certain pages (no people).

.htaccess

• I suggest replacing your flag [L] with [END].

Using the [END] flag terminates not only the current round of rewrite
processing (like [L]) but also prevents any subsequent rewrite
processing from occurring in per-directory (htaccess) context.

Via Apache.org

index.php

Not too much code here, but I think I'll be commenting on getInstance() later!

config.php

Again, very little code here.

• If you folders are plural, I think the constants should be too. So MODEL_DIR should be MODELS_DIR.

• I suggest changing the name of $connectParams to something more meaningful. What are you connecting too? If the name had more to it, that comment before it wouldn't be necessary. I also don't see any need to concatenate "parameters" into "params".

• The closing parenthesis shouldn't be indented.

init.php

• Why is this called "init" if all it does is autoload? It does more than just "initialize".

-
Before this project gets too far, I highly recommend two things: namespaces and PHP-FIG's autoloader. For a framework, namespaces will be all too great (see quote), and PHP-FIG really knows their stuff.


PHP Namespaces provide a way in which to group related classes,
interfaces, functions and constants.

Here's beginner-friendly a namespace introduction.

-
The closing parenthesis of $paths shouldn't be indented.

-
This whole section:

foreach ($paths as $path) {
    if (file_exists($path.$fileName)) {
        $classFile = $path.$fileName;
        break;
    }
}

if ($classFile!==false) {
    include $classFile;
}
else {
    die('File for class '.$className.' not found!');
}

can simply be:

foreach ($paths as $path) {
    if (file_exists($path . $fileName)) {
        if (!(include $path . $fileName)) {
            // Was a file, but it couldn't be included
        }
        break;
    }
}

Notice $classFile is now eliminated too.

• I did take out your die() function. I think this is a bad practice and should be avoided, especially in something production worthy. Try and implement a user friendly error reporting system.

core/App.php

• protected static $instance; Why is this protected? What could this class extend? What in the world is an "instance"? Give the variables some meaning.

• public static $dbHandler; Why is this public? I think the "public" shouldn't be able to set this, so I say encapsulate and give it a get function. The name is not very readable. Avoid the vague "handlers" and "managers". There are plenty of options.

• Space out the name and the default in $connectParams=array(). I also suggest type hinting an array for this.

• The conditional self::$instance==null can really become an easier to read is_null(self::$instance).

• It's strange that the constructor is protected. Will this class be extended? (I'd say that's unlikely) If not, I'd suggest private.

• The first four initialization in that constructor seem useless. Check if any of the keys are not set, and then throw an error. There's no point in setting up a database that's guaranteed to fail.

• What good does it do to echo $e->getMessage();? No one visits this page, so no one will see that message. If they do see it, unwanted information may be contained in it. Again, you need to set up proper error handling.

• Confusing names here: self::$dbHandler = $dbHandler. I say change both. This line is weird to read.

• __clone() is dead. What good does it do?

• Same with __wakeup(). Except he's missing his sister, __sleep(). These two go hand in hand, why one and not the other?

• Will the default page always be "Site"? I see a lack of future proofing here.

• $params is a terrible name.

• All that URL parsing might be in need of parse_url(). That whole chunk needs re-factoring. It's ugly, inconsistent, and difficult to follow.

• Will the actions always end with "Action"? Future-proof this baby...

core/Criteria.php

• What's a "Criteria"? Based on the class name, I immediately have no idea what it will do.

• That's a lot of variables. Do you really need that many? And will they all be extended?

• I see two public variables, do those two need to be public?

• I suggest (is_object($model)) be ($modelinstanceofModel).

• I think you're trying to make a query builder class? I suggest you check out how others do this.

I'm not going to comment on the rest, because I think it could use some major alterations

core/Model.php

-
I don't see the point in this:

foreach ($test as $key => $value) {
    $lastId = $key;
}

This must be dead code...

• There's no default for the switch in the __set(). I recommend one.

-
This:

``

StackExchange Code Review Q#52611, answer score: 2