3
\$\begingroup\$

The code itself is divided into 3 logical parts:

  1. Core modules (I call them Kernel modules, my love for Linux) - those are basic modules a web-developer would need, e.g. database wrapper class (PDO), User class that works with user session etc.
  2. Developer modules - the modules a web-developer might write for themselves.
  3. Output modules - the HTML output that might include other modules - Kernel, Developer or Output modules.

I am showing only the Kernel class here, since it does all the job handling everything. I will also show a small example of how to work with this framework.

<?php

namespace Qobox;

class Kernel {

    private $modules;

    public function __construct() {
        // Get the $_REQUEST and filter it into $request
        $request = array();
        foreach($_REQUEST as $key => $value) {
            $key = htmlspecialchars($key, ENT_QUOTES|ENT_DISALLOWED|ENT_HTML5, "UTF-8");
            $value = htmlspecialchars($value, ENT_QUOTES|ENT_DISALLOWED|ENT_HTML5, "UTF-8");
            if(empty($value)) {
                $request[$key] = $key;
            } else {
                $request[$key] = $value;
            }
        }
        // Done

        // Now let's load the output modules
        // Load the Config and select the module we will load
        $this->modules = require_once "Config/Modules.php";

        if (!empty($request)) {
            if(key_exists(array_keys($request)[0], $this->modules)) {
                $load = array_shift($request);
                // Load is the name of the module now
                // and it removed the name of the module from $request
            } else {
                $load = array_keys($this->modules)[0];
            }
        } else {
            $load = array_keys($this->modules)[0];
            // Or just load the first in list module
        }
        // Done

        // Set this module as the class parameter
        $this->moduleLoaded = $load;
        // Done

        // The module may have multiple files in it
        // We need to load the right one
        if(is_array($this->modules[$load]) && !empty($this->modules[$load])) {
            if(!empty($request)) {
                if(in_array(array_keys($request)[0], $this->modules[$load])) {
                    $load .= "\\".array_shift($request);
                } else {
                    $load .= "\\".$this->modules[$load][0];
                }
            } else {
                $load .= "\\".$this->modules[$load][0];
            }
        }
        // Done

        // Initialize the module with all it's parameters
        $module = "\Output\\" . $load;

        try {
            $moduleReflection = new \ReflectionClass($module);
        } catch (\ReflectionException $e) {
            print_r($e);
        }

        if($moduleReflection->isInstantiable() === TRUE) {
            $module = $moduleReflection->newInstance();

            // get the array od module properties
            $moduleProperties = $moduleReflection->getProperties();
            foreach($moduleProperties as $property) {
                $moduleName = $property->name;
                $property->setAccessible(true);
                $propertyValue = $property->getValue($module);

                // if a property is an array, then load each array element into $Property->ArrayElement etc....
                if(is_array($propertyValue) === TRUE) {
                    $propertyClass = $this->PropertyRecursiveLoad($propertyValue, $moduleName);
                    $property->setValue($module, $propertyClass);
                } else {
                    // ... or load a simple "string" property as the module
                    $property->setValue($module, $this->KernelModuleLoad($moduleName."\\".$moduleName));
                }
            }
            // Done

            // Initialize the needed method
            // We grab only public methods

            $moduleMethods = $moduleReflection->getMethods(\ReflectionMethod::IS_PUBLIC);

            if(!empty($request) && $moduleReflection->hasMethod(array_keys($request)[0])) {
                // the method exists, load it
                $moduleMethod = $moduleReflection->getMethod(array_shift($request));
            } else {
                // failed to locate the method, load the default
                if($moduleMethods[0]->name == "__construct") {
                    $moduleMethod = $moduleMethods[1];
                } else {
                    $moduleMethod = $moduleMethods[0];
                }
            }

            $argumentArray = array();
            $moduleMethodArguments = $moduleMethod->getParameters();
            foreach($moduleMethodArguments as $Parameter) {
                if(isset($request[$Parameter->name]) && !empty($request[$Parameter->name]) && $request[$Parameter->name] != $Parameter->name) {
                    $argumentArray[$Parameter->name] =  $request[$Parameter->name];
                } else {
                    if($Parameter->isDefaultValueAvailable() === TRUE) {
                        $argumentArray[$Parameter->name] = $Parameter->getDefaultValue();
                    } else {
                        $argumentArray[$Parameter->name] = "";
                    }
                }
            }

            $moduleMethod->invokeArgs($module, $argumentArray);
        }
    }

    private function KernelModuleLoad(String $moduleName) {
        $moduleNamespace = "\Qobox\\" . $moduleName;

        try {
            $kernelModuleReflection = new \ReflectionClass($moduleNamespace);
        } catch (\ReflectionException $e) {
            // The needed kernel module does not exist. Try to connect same module component
            // Module components should not have same names as Kernel Modules!!!
            $moduleNamespace = "\Modules\\" . $moduleName;

            try {
                $kernelModuleReflection = new \ReflectionClass($moduleNamespace);
            } catch (\ReflectionException $e) {
                // Even a user module does not exist. Try to connect output module component
                $moduleNamespace = "\Output\\" . $moduleName;
                try {
                    $kernelModuleReflection = new \ReflectionClass($moduleNamespace);
                } catch (\ReflectionException $e) {
                    // nothing found... Maybe it's just an empty private property
                    // do nothing
                }
            }
        }

        $kernelModule = $kernelModuleReflection->newInstance();
        $kernelModuleProperties = $kernelModuleReflection->getProperties(\ReflectionProperty::IS_PROTECTED);

        foreach($kernelModuleProperties as $kernelDependency) {
            $kernelDependency->setAccessible(true);
            if(empty($kernelDependency->getValue($kernelModule))) {
                $kernelModuleName = $kernelDependency->name;
                $kernelDependency->setValue($kernelModule, $this->KernelModuleLoad($kernelModuleName));
            }
            $kernelDependency->setAccessible(false);
        }
        return $kernelModule;
    }

    private function PropertyRecursiveLoad(Array $propertyValue, String $moduleName) {
        $propertyClass = new \stdClass();
        foreach($propertyValue as $moduleCaller => $moduleSubname) {
            if(is_array($moduleSubname) === TRUE) {
                $propertyClass->$moduleCaller = $this->PropertyRecursiveLoad($moduleSubname, $moduleName."\\".$moduleCaller);
            } else {
                $propertyClass->$moduleSubname = $this->KernelModuleLoad($moduleName."\\".$moduleSubname);
            }
        }

        return $propertyClass;
    }
}

First of all, Kernel grabs everything from $_REQUEST - all the Get and Post data. It then sanitizes it for further use.

Next, the first key/value output module is loaded and instantiated. The Kernel checks what private properties it has and tries to load the respective components from Kernel modules, Dev modules or Output modules. In the end, each instantiated class has private properties containing objects.

Finally, Kernel looks at what parameters a called public function has. It then sets each parameter to it's value.

An example

An example to show you how it's all being processed. This goes to Modules/Test/Test.php.

<?php

namespace Modules\Test;

class Test {

    public function printWorld() {
        return 'World!';
    }
}

And this goes into Output/Example/Example.php:

<?php

namespace Output\Example;

class Example {
    private $Database;
    private $Test;

    public function runExample($hello) {
        echo $hello. ', ';
        echo $this->Test->printWorld();
        echo '<br><pre>';
        print_r($this);
    }
}

Now, private $Test in class Example hold the instantiated object of class Test.

If you query for "index.php?Example&hello=Hello", the output would be Hello, World! with printing out the database connection info afterwards.

The final statement

I've been working on this for over a year, polishing the idea, thinking through the structure etc. I would like to publish the whole framework as open-source on a new domain, qobox.net.

But I am still not sure whether I achieved perfection (which is a very relative thing) or there are still things to improve (of course there are). I'd be grateful if any of you could point out what could be improved before the first release of Qobox Framework ever.

\$\endgroup\$
1
\$\begingroup\$

I haven't looked at all the code in-depth, but here are some first thoughts about it:

Error Handling

Neither printing exceptions nor ignoring them is a good approach, especially for a low-level class like yours, because you don't allow the calling code to handle the exception like it wants. For example, users will probably want to create a custom error page.

Input Sanitation

It seems that it is impossible to distinguish between GET and POST variables, which is not ideal.

HTML encoding should conceptually happen when outputting, as that is where XSS actually happens; that is when you know the context you are inserting into, which means that that is when you know how to properly defend against XSS (imagine eg inserting into <style> or <script> tags, or into attributes such as onMouseOver).

Encoding directly on input also means that you get unclean data, which may matter if you process the input somewhere (this may eg matter for passwords, or for external functions such as mail).

In your case, you also miss quite a lot of input, such as from _FILES and _SERVER.

For some (legacy) applications, it may still make sense to encode on input, and remember that in specific situations the input must be decoded. But since you are writing your code from scratch, it would be a good idea to use a proper approach. Something like a templating engine which by default encodes its variables might be an idea. For input sanitation, you still might want something restricting your input, eg like this.

Structure

Your kernel class seems to do too much. It handles and sanitizes input, loads modules, processes modules, seems to be responsible for routing, etc. Most of this work happens in a large code block in the constructor, which really hurts readability.

At a minimum, I would expect classes such as Input and ModuleLoader, and methods such as loadModule, instantiateModule, etc.

Routing Whitelist

Maybe I overlooked it, but it doesn't seem that you have a whitelist that checks if a method is actually allowed to be called with the given parameters (such as here).

As your class also does routing, I would expect something like this to exist (it would also make it a lot easier to use the router, as it would give an overview over what routes exist). Sure, you could just say that all public methods are, well, public. But I don't think that this is an ideal approach for a framework, as you want to make it difficult for your users to mess up.

Documentation

It would greatly increase readability if you would use something like PHPDoc comments for your classes and methods.

Comments

You have quite a lot of comments, which on the one hand is good (it makes your code easier to understand), but on the other hand it's also a sign of unclear code.

For example:

  • // Load is the name of the module now: Why not rename the variable $loadedModule, and save the comment?
  • // Or just load the first in list module, // and it removed the name of the module from $request, etc: If you would wrap the modules array in a Modules class, you could have methods such as popModule, etc, saving these comments.
  • // Now let's load the output modules, // Initialize the module with all it's parameters, // Initialize the needed method, etc: These comments are really just there to structure your code into blocks. A better approach would be to extract these blocks into methods, and add PHPDoc comments to those. Something like // Done is also always a sign that your code blocks are too large, and that more methods may be a good idea.

Misc

  • Because the code block in the constructor is quite large, you have quite a lot of variables in it, some of which are quite similar. This makes your code hard to read. For example, you have $load, which is assigned a value, then you save this value in $this->moduleLoaded, and then you keep on processing $load, which is a bit confusing to read.
  • You also reuse variables for different purposes, which is also a bit confusing. For example, $module first holds a string, and then later an instance of some object.
  • try to be consistent with your variables names. Most of yours start with a lower-case character, but eg $Parameter does not. It would also increase readability if you used the same name for the same concepts. Eg here: foreach($moduleMethodArguments as $Parameter) it seems that you use argument and parameter interchangably. But for a reader, this is not clear, leaving them wondering what the difference is.
\$\endgroup\$
  • \$\begingroup\$ Thank you for the useful advice. I will edit the code and, as meta suggests, will open a new question for a new review. \$\endgroup\$ – Nordenheim Jan 23 '16 at 3:50

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.