Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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']);
share|improve this question
    
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. – svick Apr 27 '13 at 20:50
    
Adding it right now, thanks you. – user24522 Apr 27 '13 at 21:03
    
"Do you guys think that it's uniform?" By uniform, do you mean consistent? – MarcDefiant May 3 '13 at 14:00

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.
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.