5
\$\begingroup\$

I want users to enter their code into my blog and keep original styling tags intact. So I had to develop a function that extracts user's code (between two tags) and convert special characters to HTML entities, then remove unwanted tags using purifyHTML.

I want to know if it is safe to use it like that, and if there are any bad issues with this function.

public function extracter($string, $start, $end){
    function get_string_between($string, $start, $end){
        $delimiters = array();
        $ini = strpos($string,$start);
        if ($ini == 0) return FALSE;
        $delimiters['start'] = substr($string, 0, $ini);
        $ini += strlen($start);
        $last = strpos($string,$end,$ini);
        $len =  $last - $ini;
        $delimiters['inside'] = substr($string,$ini,$len);
        $delimiters['end'] = substr($string, $last+strlen($end));
        return $delimiters;
    }
    function reconstruct($strings = array(), $filter){
        if( count($strings) == 0 || !$strings )
            return false;
        $count = count($strings);
        $i = 0;
        $str = '';
        $code = array();
        $uniq = uniqid();//i create a unique key to prevent confusion if users insert the same token i'm using
        foreach ($strings as $key => $val) {
            $i++;
            foreach ($val as $k => $v) {
                if( $k == 'start' )
                    $str .= $v;
                if( $k == 'inside' ){
                    $str .= '['.$uniq.$i.']'; //creat a token in the string
                    $code[$i] = htmlspecialchars($v);
                }
                if( $i == $count )
                    if( $k == 'end' )
                        $str .= $v;
            }
        }
        $purify = $filter->purifyHtml($str);//i'm using purifyhtml to remove undesirable tags and leave TinyMce tags
        for ($j=1; $j <= $i; $j++) {
            $purify = str_replace('['.$uniq.$j.']', $code[$j], $purify); //replace the token with the escaped code
        }
        return $purify;
    }
    /* main */
    $string = " ".$string;
    $components = array();
    $exists = TRUE;
    while ( $exists ) {
        $st = get_string_between($string, $start, $end);//decompose if token found
        if( !$st )
            return $string;// return the original string if nothing found
        $string = $st['end'];
        $components[] = $st;
        $exists = strpos($st['end'], $start) !== FALSE;//check if another code exists in the string
    }

    return reconstruct($components, $this->InputFilter);
}
\$\endgroup\$
1
\$\begingroup\$

"Extracter" should be spelled "extractor".

It's nice that you broke up the functionality into smaller functions.


In get_string_between(), you don't appear to be properly handling the $start and $end strings not being found. Consider:

$ini = strpos($string,$start);
if ($ini == 0) return FALSE;

Did you mean to write

if ($ini === FALSE) return FALSE;

instead? strpos() is tricky, in that you have to carefully distinguish between 0 and FALSE return values.

For $last = strpos($string,$end,$ini), you don't check the return value at all, and just use it.


Flag variables are an ineffective way to control execution flow. If possible, restructure the code to avoid them. For example,

$components = array();
$exists = TRUE;
while ( $exists ) {
    $st = get_string_between($string, $start, $end);//decompose if token found
    if( !$st )
        return $string;// return the original string if nothing found
    $string = $st['end'];
    $components[] = $st;        $exists = strpos($st['end'], $start) !== FALSE;//check if another code exists in the string
}

… should be written as an equivalent do-while loop:

$components = array();
do {
    $st = get_string_between($string, $start, $end);//decompose if token found
    if( !$st )
        return $string;// return the original string if nothing found
    $string = $st['end'];
    $components[] = $st;
} while (strpos($st['end'], $start) !== FALSE); //check if another code exists in the string

However, even that is dissatisfying, since the strpos() test is redundant with the strpos() operation within get_string_between().

$components = array();
while (($st = get_string_between($string, $start, $end))) {
    $components[] = $st;
    $string = $st['end'];
}
return empty($components) ? $string    // return the original string if nothing found
                          : reconstruct($components, $this->InputFilter);

In reconstruct(), your loops are too complex. The original code…

$count = count($strings);
$i = 0;
$str = '';
$code = array();
$uniq = uniqid();//i create a unique key to prevent confusion if users insert the same token i'm using
foreach ($strings as $key => $val) {
    $i++;
    foreach ($val as $k => $v) {
        if( $k == 'start' )
            $str .= $v;
        if( $k == 'inside' ){
            $str .= '['.$uniq.$i.']'; //creat a token in the string
            $code[$i] = htmlspecialchars($v);
        }
        if( $i == $count )
            if( $k == 'end' )
                $str .= $v;
    }
}

… could be simplified to

$str = '';
$code = array();
$uniq = uniqid();
foreach ($strings as $i => $component) {
    $str .= $component['start'];
    $str .= "[$uniq" . ($i + 1) . "]";  // Create a token in the string
    $code[$i + 1] = htmlspecialchars($component['inside']);
}
$str .= $component[count($component) - 1]['end'];

It's unconventional that $code is a 1-based array, and there doesn't seem to be any advantage to starting from 1 rather than 0, so just use $i instead of $i + 1 everywhere. Just adjust $j accordingly.


Nobody likes to read low-level string manipulation code, especially when it's not clear what the code is supposed to do. Now that I've understood it, I would write a comment on the function like this:

/**
 * Runs HTML purifier on $string.  Sections of the string between $start
 * and $end delimiters are taken literally: instead of being filtered through
 * the HTML purifier, they are escaped using htmlspecialchars() instead.
 * The $start and $end delimiters themselves are discarded.
 */
\$\endgroup\$

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.