patternphpMinor
Request handler based solely on reflections
Viewed 0 times
solelyreflectionsrequestbasedhandler
Problem
The code itself is divided into 3 logical parts:
I am showing only the
```
$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
- 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),
Userclass 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,DeveloperorOutputmodules.
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 `
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.