HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Request handler based solely on reflections

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
solelyreflectionsrequestbasedhandler

Problem

The code itself is divided into 3 logical parts:

  • 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.



  • Developer modules - the modules a web-developer might write for themselves.



  • 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.

```
$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_k

Solution

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 ` or 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.

Context

StackExchange Code Review Q#117485, answer score: 2

Revisions (0)

No revisions yet.