4
\$\begingroup\$

I'm looking for feedback about anything and everything to do with this short program. For example, is there a risk in not sanitising the data, and if so, how would I do that given the need for non-alpha-numerics? How is the logic? Is my html up to date? Suggestions for styling? All input appreciated. I'm on a learning curve to become a professional, so need to be producing quality code.

<?php
/*
PHP Regex tester
Author: Robin Andrews 2016
*/

// Has form been reset?
if (!empty($_POST['reset'])){
    $_POST = array();
    $msg = "";
}

// Check if fields completed and process if so
if (!empty($_POST['pattern']) && !empty($_POST['string'])){
    $pattern = $_POST['pattern'];
    $string = $_POST['string'];

    if (preg_match_all('~'.$pattern.'~', $string, $matches)){
        $groups = implode("; ", $matches[0]);
        $count = count($matches[0]);
        $msg =  $count . " Matches present: " . $groups;
    } else {
        $msg = "Pattern is not present";
    }

    // clear $_POST data
    $_POST = array();

}

?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Regex Tester</title>
    <style>
        body{
            font-family: arial;
            font-size: 18px;
        }
        #wrapper{
            width: 400px;
            margin: 20px auto;
            border: 2px solid blue;
        }

        h3{
            text-align: center;
        }

        input, textarea{
            font-size: inherit;
            font-family: inherit;
            box-sizing: border-box;
            margin: 5px;
            width:300px;
            border-radius: 10px;
            padding: 5px;
        }

        form{
            width: 300px;
            margin: 5px auto;
        }

        input[type=submit], #reset{
            font-family: inherit;
            font-size: 16px;
            box-sizing: border-box;
            margin: 5px 0;
            width:100px;
            border-radius: 10px;
            padding: 2px;
        }

        #submit{
            margin-left:5px;
        }

        #message{
            margin: 10px;
        }

    </style>
</head>
<body>

    <div id="wrapper">
        <h3>Regex Tester</h3>
        <form action="" method="post">
            Enter a string: <textarea name="string" rows="4" autocomplete="off"><?=isset($string)? $string : ''?></textarea><br>
            Enter a pattern: <input type="text" name="pattern" autocomplete="off" value="<?=isset($pattern)? $pattern : ''?>"><br>
            <input type="submit" value="submit" id="submit">
            <input type="submit" value="reset" id="reset" name="reset">
        </form>
        <div id="message">
            <p><?=isset($msg)? $msg : '&nbsp'?></p>
        </div><!-- message -->
    </div><!--wrapper-->
</body>
</html>
Improve this question
\$\endgroup\$
1
  • 2
    \$\begingroup\$ What if $pattern has ~ character? \$\endgroup\$
    – hjpotter92
    Jun 12, 2016 at 14:43

2 Answers 2

Reset to default
2
\$\begingroup\$

If you don't want to extend this site and you will not change the current page at all, then your code is completely ok. But you probably will therefore need to make it more flexible.

  1. Put css style and php code to different files. (see require_once)
  2. Build a class to sanitize _POST data and provide necessary data (and the reset mechanism)
  3. Build a class for executing preg_match_all() and processing the output (use spl-autoload for your classes)

The next step is to learn the MVC pattern...

Improve this answer
\$\endgroup\$
1
\$\begingroup\$

The reset mechanism can probably be simplified. Instead of making it a submission button within your form, you can enjoy the same effect, by creating an <a href="">Reset</a> tag and styling identically to the other submit button. This way, when Reset is clicked, there will be no POST request and you can safely omit the 4 lines of code starting with if (!empty($_POST['reset'])){.

Next is the handling of the POST data. My advice is not to declare a new variable unless beneficial. If you are not mutating the submission data at all, then there is no benefit in creating redundant variables which are already accessible in the $_POST superglobal array.

count($matches[0]) is not necessary. You should be assigning the returned value from preg_match_all() to $count.

Should you be sanitizing the incoming data? Yes, but because the idea of your script is to allow special characters in the regex string to be respected, it would be a bad idea to implement preg_quote(). Escaping special characters in the pattern will severely limit the functionality of your application. When you want to display user data in an HTML document, you need to html encode characters that may foul with your intended HTML markup. Imagine if $string was </textarea><div>See what I did here</div><textarea> -- you would be inviting people to fundamentally change your document. Begin looking at https://stackoverflow.com/q/1873793/2943403 and https://stackoverflow.com/q/43813500/2943403 for starters, but there are hundreds of demonstrations on the web. Make yourself familiar with all of the regex pattern modifiers and decide how far down the rabbit hole you'd care to go.

Further about the regex handling. You will either need to make an authoritative judgement call about the pattern's flags/modifiers, or you'll need to add more utility to your UI. Should your tool allow case-insensitive matching? Should it process strings as individual bytes or by characters?

And finally, instead of isset($v) ? $v : '', in modern php, you can use the null coalescing operator -- $v ?? ''.

Improve this answer
\$\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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.