PHP + MySQL beginner, any tips appreciated on the approach

Question

Any tips or review points are appreciated, I'm not much familiar with server-side web-dev (PHP) and DB.

Few things of note:

I see no reason to separate codebase into different files and bother with imports - provided code is ALL the project and some paradigm boilerplate approach makes no sense. Also saves few disk reads. But if you disagree I'm all ears.
ASCII for DB was deliberate, I knew the data I will be using and storing beforehand, I also wanted to see if I can make it work under the narrow and defined conditions. Yes, I know, I could just set utf8mb4 and forget. If it was more complex and public project with variety of data that would be it.

<html>
 <head>
  <title>Video Mirror</title>
 </head>
 <body>
 <?php
    ini_set('display_errors', 1);
    if (!$_GET || empty($_GET['path'])) { die("No video ID specified"); }

    //
    $vKey = $_GET['path'];
    $db_user = "linus_torvalds";
    $db_pass = "kernel_overflow";
    $db_conn = "mysql:host=localhost;dbname=streamab_vdata;charset=ascii";
    
    if (!ctype_alnum($vKey) || (mb_strlen($vKey) > 6 || mb_strlen($vKey) < 5)) { die("Ivalid video key format"); }

    $PDO = new PDO($db_conn, $db_user, $db_pass);
    $SQL_QUERY = "SELECT * FROM cache WHERE id = ? LIMIT 1";
    $queryStmnt = $PDO->prepare($SQL_QUERY);
    $queryStmnt->execute([$vKey]);
    $data = $queryStmnt->fetch();

    if(!$data || ($data && $data['timestamp'] < time()))
    {
        $data = fethRemote("https://kino.com/$vKey");
        $SQL_QUERY = "INSERT INTO cache (id, timestamp, url, width, height, type) VALUES(:id, :timestamp, :url, :width, :height, :type) ON DUPLICATE KEY UPDATE timestamp = :timestamp, url = :url";
        $queryStmnt = $PDO->prepare($SQL_QUERY);
        $queryStmnt->execute($data);
    }

    echo buildBody($data);

    //
    //Helpers
    function doSQL($con, $query, $values)
    {
        $queryStmnt = $con->prepare($query);
        $queryStmnt->execute($values);
        //uhh conditional return?
    }

    function buildBody($values)
    {
        $vStyle = 'style="width:' . $values['width']. 'px;height:' . $values['height'] . 'px;\"';
        $vSrc = 'src="' . $values['url'] . '"';
        $vType = 'type="' . $values['type'] . '"';
        return "<video controls $vStyle <source $vSrc $vType></video>";
    }

    function fethRemote($url)
    {
        $html = file_get_contents($url);

        if(!$html || empty($html)) { die("Failed to fetch any data"); }

        $dom = new DOMDocument;
        @$dom->loadHTML($html);
        $metaTags = $dom->getElementsByTagName('meta');
        $data = ['id' => $vKey, 'timestamp' => '', 'url' => '', 'width' => '', 'height' => '', 'type' => ''];

        foreach ($metaTags as $tag)
        {
            $c = $tag->getAttribute('content');
            $p = $tag->getAttribute('property');
            if(empty($c) || empty($p)) { continue; }
            switch ($p)
            {
                case 'og:video:width':
                    $data['width'] = $c;
                    break;
                case 'og:video:height':
                    $data['height'] = $c;
                    break;
                case 'og:video:type':
                    $data['type'] = $c;
                    break;
                case 'og:video:url':
                    $data['url'] = $c;
                    break;
            }
        }

        if(empty($data) || empty($data['url'])) { die("Fetched page data does not contain valid matching tags"); }

        $url_components = parse_url($data['url']); 
        parse_str($url_components['query'], $params);
        $data['timestamp'] = $params['Expires'];
        return $data;
    }
 ?> 
 </body>
</html>

Few direct questions:

Do people still put MySQL credential into the plain php-files or env. variables? Are we still in the stone age of security?
PDO handles most of the query sanitation, is there something more I'm missing?

Does the code work as intended? Would you mind describing what the code is supposed to do? — slepic, Commented Jul 13, 2020 at 3:38
The title of your question is meant to express what your script does. It should not express your experience level or your concerns pertaining the script. @Kir — mickmackusa, Commented Jul 13, 2020 at 5:26
#1 is why it's generally considered prudent to have a separate include/config.php file that contains stuff like database credentials, which can then be set to be readable only to the user itself, with access to the include/ directory blocked by server configuration. But the core problem is that php needs to know those credentials in order to be able to authenticate to the database server, so they're going to have to be in plaintext somewhere. — Shadur-don't-feed-the-AI, Commented Jul 13, 2020 at 18:31

Your Common Sense · Accepted Answer · 2020-07-13 15:12:43Z

Although the preface is quite disheartening and basically says "I don't want this code to be improved or evolved into something bigger or better", but we can consider this notion as short-sighted and discard it for sake of providing a good code review.

The code is overall more good than bad, especially from the security standpoint.

However it could greatly benefit from a better structure:

Move database connection in a separate file
Move service functions in another file
Create a bootstrap.php file to put all the bootstrapping (includes, settings) together
Separate the business logic from the HTML output. Right now your die() statements just leave torn HTML. I don't think it's acceptable even for a beginner's web page.

Start any HTML only when the business logic is finished and the end result is indeed HTML (not some HTTP header or another format).
- it will also make the buildBody() function obsolete. Above all, it will make your HTML look like HTML, not some ragged zombie.
- however, we will have to find a way to make die() not to kill the script entirely, and it is called a user-defined exception.
- for the time being, HTML can be kept in the file but as soon as the project grows, it could be moved into two separate files as well.

There could be also many other improvements.

For example, if you add just return $queryStmnt; to your doSql function it will make it fantastically versatile, allowing to write concise PHP code that allows to run any SQL query and return the results in dozens different formats.

bootstrap.php

<?php
error_reporting(E_ALL);
ini_set('display_errors', 1);
include __DIR__."/pdo.php";
include __DIR__."/helpers.php";

helpers.php

<?php

class UserErrorException extends Exception {};

function doSQL($con, $query, $values)
{
    $queryStmnt = $con->prepare($query);
    $queryStmnt->execute($values);
    return $queryStmnt;
}

function fethRemote($url)
{
    // many things can be improved here as well
    // but I believe this function deserves a distinct review
}

index.php

<?php
include 'bootstrap.php';

try {
    if (empty($_GET['path'])) {
        throw new UserErrorException("No video ID specified");
    }
    $vKey = $_GET['path'];
    if (!ctype_alnum($vKey) || (mb_strlen($vKey) > 6 || mb_strlen($vKey) < 5)) {
        throw new UserErrorException("Invalid video key format");
    }

    $sql = "SELECT * FROM cache WHERE id = ? and timestamp < curdate()";
    $data = doSql($pdo, $sql, [$vKey])->fetch();
    if(!$data)
    {
        $data = fethRemote("https://kino.com/$vKey");
        $sql = "INSERT INTO cache (id, timestamp, url, width, height, type)
            VALUES(:id, :timestamp, :url, :width, :height, :type)
            ON DUPLICATE KEY UPDATE timestamp = :timestamp, url = values(url)";
        doSql($pdo, $sql, $data);
    }
} catch (UserErrorException $e) { // errors shown to the client
    $error = $e->getMessage();
} catch (Throwable $e) {
    error_log($e);
    http_response_code(500);
    if (ini_get('display_errors')) {//all other errors
        die($e); 
    } else {
        $error = "An internal error occurred, please try again later";
    }
}
?>

HTML part that can be moved into a separate file or kept in place:

<html>
<head>
  <title>Video Mirror</title>
</head>
<body>
  <?php if ($error): ?>
    <?= $error ?>
  <?php else: ?>
    <video controls style="width:<?= $data['width'] ?>px;height:<?= $data['height'] ?>px;">
      <source src="<?= $data['url'] ?>" type="<?= $data['type'] ?>">
    </video>";
  <?php endif ?>
</body>
</html>

Hmm, nice, what is __DIR__. for? And why extend Exception if 0 functional added? Also, what do you think about this approach: paste.gg/p/anonymous/6624c3142bf34012871b14ab10c4eef0 — Kirikan, Commented Jul 13, 2020 at 12:43
__DIR__ because working directory can be different. Extends because the name is the functionality added. Later the processing for different exceptions is different. Now it's my turn to ask why? what benefit it would give over the current approach? — Your Common Sense, Commented Jul 13, 2020 at 12:58
i would say such a function could make sense in this form: 3v4l.org/kZs4A — Your Common Sense, Commented Jul 13, 2020 at 13:22
My bad wrong link: paste.gg/p/anonymous/7c59204cfc4c4373be31051d4e0d6cc7 I was wondering if you could also give some insight into input data charset handling: stackoverflow.com/posts/comments/111185356?noredirect=1 — Kirikan, Commented Jul 13, 2020 at 13:24

mickmackusa · Accepted Answer · 2020-07-14 05:18:42Z

Declaring ini_set('display_errors', 1); while in the development stage is fine, but when you move to production, you mustn't be presenting error messages to the end user.
if (!$_GET) isn't of much use given the following expression.
Just use if (empty($_GET['path'])).
ctype_alnum() is locale specific and can possibly let you down. (Here's a demo I prepared a little while back.) \p{L} will match your multibyte letters and \p{N} will match your multibyte numbers. You can bake the required length of the string into a single regex pattern as well. /^[\p{L}\p{N}]{6}$/u

This means your entire validation check can be done in one hit.
```
 if (empty($_GET['path']) || !preg_match('/^[\p{L}\p{N}]{6}$/u', $_GET['path'])) {
     die("Missing/Invalid path value");
 }
```
If you aren't going to move your database connection credentials into a separate config / .env file, then I don't see any advantage in declaring single-use variables either. Just write the literal strings into your pdo connection call.
I think you should try to avoid using ALLCAPS variable names so that they are instantly differentiated from constants. (Yes, of course, the dollar sign is another clue, but this is still my advice for improved readability.) Conversely, you should always use ALLCAPS when you using sql keywords this will improve readability as well.
Assuming the id column of the cache table is the Primary/Unique Key, there is no benefit in declaring LIMIT 1.
Your row "freshness" condition can be written into your sql instead of checking the timestamp in php.
doSQL() looks like unfinished noise. You should remove it until it is something that you are going to finish and incorporate.
In buildBody(), I think the argument should be named $attrs since it is an array of attributes. You might even like to signify the data type with some typehinting. As for the building of the new html, I think using sprintf() will go a long way to clean up the syntax and make the code easier to read/maintain.
if(!$html || empty($html)), again, can be simplified to if (!$html)) -- the variable will always be set, so you only need to check if it is falsey.
I despise the use of @ (error suppressor / "stfu" operator) in code and cringe whenever I see them. There are ways to handle errors generated by invalid html being passed to DOMDocument. For instance: libxml_use_internal_errors(true);
~~Using XPath to compliment DOMDocument is going to allow you to express all of your requirements in a tidy expression so that you don't need to iterate non-qualifying nodes.~~ I overlooked what you were extracting from the DOM. Just use get_meta_tags() for the cleanest extraction.
I don't think I would bother to declare the empty strings in $data. You are already making empty() checks later in the code, so if these keys are not set, then empty() will handle things just fine.
If I am not mistaken, your $metaTags loop is only expecting to find one match. So that your loop is not performing unnecessary iterations, I recommend that your case statements break 2 so that the loop is killed. ~~That said, if you set up XPath properly, you won't need a loop or break 2 -- your code will either find 0 or 1 qualifying node.~~
if(empty($data) || empty($data['url'])) can be simplified to if (empty($data['url'])) because if the url key is set and truthy, then of course $data is not empty.
Finally, as a general rule, whenever your script's intention is to write to the database, then the script's incoming data should come from $_POST. Scripts that merely intend to read from the database should be relying on $_GET data. If you have a valid overriding reason to do it your way, okay, but this is a line that I toe in my own projects.

I disagree about xPath, this is what I had before pastebin.com/raw/kRYLpUqR It is not just more code and ugly, I also need to check each xPath query if it has results before calling getAttrivute or I get error. The loop is very tidy and it just one pass over all tags instead of "4 passes until first math as xPath". Does break behave differently in PHP as opposed to other languages? ctype_alnum() was temporary, ideally i wanted to limit to ascii only but setlocale() has issue with threads. Timestamp is external and managed by 3rd party so I need to store it and update from them. — Kirikan, Commented Jul 13, 2020 at 9:55
I overlooked what you were extracting. Just use get_meta_tags() — mickmackusa, Commented Jul 13, 2020 at 11:22
Any suggestion for the charset handling for input? Regex is always slower in every language I've known so far, I dont believe PHP is exception. Is there really no other choice but regex? See stackoverflow.com/questions/62874128 — Kirikan, Commented Jul 13, 2020 at 12:02
Such a small regex is not going to be the bottleneck for your script. This is such a succint and direct single-call technique. This is exactly how I would validate the input in my own my own project. Of course you have other choices, but they will be more verbose and if they are faster, the performance gain will be utterly undetectable by humans. — mickmackusa, Commented Jul 13, 2020 at 13:10
Do you have any examples that expand on 11) and 0|'s sprintf()? — Kirikan, Commented Jul 13, 2020 at 14:10

slepic · Accepted Answer · 2020-07-13 07:28:40Z

Do people still put MySQL credential into the plain php-files?

Yes. Whether it is ok is a different question...

Do people still put MySQL credential into the env. variables?

Yes. That's probably the preferred way.

You shall prevent access to files containing secrets outside of PHP. A PHP script is executed from scratch for every request. You don't want to decrypt your secrets again and again on every request. And anyway if you did that, you would be storing the encrypted secrets and their decryption keys on the same place which leads you to a neverending problem that now you have to secure your decryption keys as well. Just secure your secrets and that's enough.

Don't put secret files to git. Set file system secrets read access only to the application user. Recipe for creating the application container (the one who sets env. variables) may contain encrypted secrets that can only be decrypted (using, possibly 3rd party, service) during the execution of the recipe using credentials provided by the initiator of the recipe execution.

PDO handles most of the query sanitation, is there something more I'm missing?

Yes. You should

$pdo->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);

or handle PDO::errorCode() after executing queries.

@Kirikan Well, exceptions force you to consider possible runtime errors and halt execution if they are not caught. Right now you just assume that the entry was inserted and ignore the situation when the operation fails. — slepic, Commented Jul 13, 2020 at 9:55
@Kirikan it's one option actually. it simply tells PDO to reports database errors. — Your Common Sense, Commented Jul 13, 2020 at 10:39

Stack Exchange Network

PHP + MySQL beginner, any tips appreciated on the approach

3 Answers 3

bootstrap.php

helpers.php

index.php

Your Answer

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

Hot Network Questions

PHP + MySQL beginner, any tips appreciated on the approach

3 Answers 3

bootstrap.php

helpers.php

index.php

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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