2
\$\begingroup\$

I've been working in PHP since almost three years ago. In this time I built a framework as base for my projects.

I want it to be improved, and which other best option there are that use StackExchange?, so I decided to post this here.

The framework is hosted in GitHub: https://github.com/CPedrini/Delos

Now, the problem: I think that there is a mess in the Input class, particularly in how the base's query string is parsed, I have problems to make a definitely decision of how manage URI's parameters.

I've created a .htaccess, which rewrites the URI like this:

From this: 127.0.0.1/home/initialize/param1:value1/param2:value2

To this: 127.0.0.1/index.php?p=home/initialize/param1:value1/param2:value2

Obviously, this is not needed, it's just a fancy stuff. If mod_rewrite is disabled then all the links on the web must be changed to the rewritten form.

So, in input class constructor the query string p is parsed.

I explode the $_GET['p'] var with separation slashes and delete empty values.

Then if first value doesn't contain a : char I store it in $_GET['section'] var, it will be the control to be instantiated.

Then, if section was there, in the second exploded value, if there isn't a : char then this is the method of the control to be called. I store it in $_GET['method'].

Finally, all the others stripped vars are parsed and stripped with the : char. In the previous example, to this:

$_GET['param1'] = value1

Do you guys think that it's uniform? What changes do you suggest?

EDIT: Added code in question.

$a = explode('/', $_GET['p']);

foreach ($a as $key => $value) {
    if($value == ''){
        unset($a[$key]);
    }
}

$b = 0;

if(isset($a[0]) && !strstr($a[0],':')){
    $_GET['section'] = $this->security->sanitize($a[0]);
    $b+=1;

    if(isset($a[1]) && !strstr($a[1],':')){
         $_GET['method'] = $this->security->sanitize($a[1]);
         $b+=1;
    }
}

if(isset($a[$b])){
    for($b = $b; $b < count($a); $b ++){
        if(!strstr($a[$b],'method') && !strstr($a[$b],'section')){

            $c = explode(':',$a[$b]);
            $_GET[$this->security->sanitize($c[0])] = isset($c[1]) ? $this->security->sanitize($c[1]) : '';
        }
    }
}
unset($_GET['p']);
\$\endgroup\$
2
  • \$\begingroup\$ Sorry, but a whole library is too much for us to review. Per the FAQ, you need to include the code you want reviewed in your question (not just link to it). And it should be a reasonable amount of code. You also might want to have a look at this meta question: How to get feedback on whole projects. \$\endgroup\$
    – svick
    Apr 27, 2013 at 20:50
  • \$\begingroup\$ "Do you guys think that it's uniform?" By uniform, do you mean consistent? \$\endgroup\$ May 3, 2013 at 14:00

1 Answer 1

1
\$\begingroup\$

Instead of this block:

foreach ($a as $key => $value) {
    if($value == ''){
        unset($a[$key]);
    }
}

You could simply use array_filter() without the second parameter:

$a = array_filter($a);

And also your method system/core/Security::sanitize (I suppose that would be $this->security->sanitize in your code above):

  • strip_tags does simply nothing after htmlspecialchars, because all the tags like <b> got replaced with &lt;b&gt;.
  • If that should secure you from cross site scripting it's definitely applied at the wrong place. You should only escape the html tags before outputting them, not directly after retrieving them, because some methods (like HTML parsers) would then parse the string wrongly.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.