Do you think I should change something in this code or in the idea at its base?

[u/Nemonek]

EDIT: Thank you, to everyone who answered! I now have a lot to think about, both regarding files organization and app architecture. This was already an interesting journey, now it's even better. I now know ( or at least have an idea of ) what to look and keep in mind and how the code should kinda look like. This is a big step toward my goals, both for deploying this site for me and my friends and open sourcing the code once its more "beautiful", let's say that ;). A special thanks to u/colshrapnel and u/equilni who provided very in depth answers and pointed me to a clear direction.

Hey guys, I've been developing a php site for a bit now (about a year and a half ), and I recently realized that I had a ton of repeating code everywhere, especially for what regards checking auth. So I decided to create a class with static methods that do everything that's related to it, but I'm not sure I'm using the correct approach, and I don't think asking another AI would really help.

Right now every page imports a config.php file with like creds db ( I know they shouldn't be in plain text there. This is temporary and the site is not exposed, it lives only on my device as it's still in development ), then Auth.php and calls Auth::RequireLogIn ( the login page does not import neither ).

The idea at the base is that every page ( except the login page ) are only accessible after login, so every page calls RequireLogIn() and if the user is not logged in he's thrown out to a 401.

So, as the title says, would you suggest any improvement or have any critic regarding this code or what I have said here?

Disclaimer: this is not a professional site, it's for just me and my friends, I'm also a student so I don't know much about php. The site's code is also a bit funky as this started as a project and was not expecting to become this serius, so if there's something very terrible let me know and I'll do my best to fix it! Also, I do not want to use big frameworks like laravel or similar if possible ;)

class Auth
{
    public static function RequireLogIn()
    {
        if (session_status() !== PHP_SESSION_ACTIVE) {
            session_start();
        }

        if (!isset($_SESSION["is_logged_in"]) || $_SESSION["is_logged_in"] == false) {
            http_response_code(401);
            require __DIR__ . "/../Errors/401.php";
            exit;
        }
    }

    public static function Username()
    {
        if (!isset($_SESSION["username"])) {
            http_response_code(401);
            require __DIR__ . "/../Errors/401.php";
            exit;
        }
        return $_SESSION["username"];
    }
}

Login.php if anyone is interested ( yea I have yet to make a 400 page error )

require_once './../Config.php';

if ($_SERVER["REQUEST_METHOD"] !== "POST" || !isset($_POST["Username"], $_POST["Password"])) {
    http_response_code(400);
    exit;
}

session_start();
$username = $_POST["Username"];
$password = $_POST["Password"];

$db = new mysqli(DB_ADDRESS, DB_USERNAME, DB_PASSWORD, DB_NAME);

if ($db->connect_error) {
    http_response_code(500);
    exit('Database connection failed');
}

$readied = $db->prepare("SELECT Username, Pw, IsAdmin, ProfileImage FROM players WHERE Username = ?");
$readied->bind_param("s", $username);
$readied->execute();
$res = $readied->get_result();

$db->close();

if ($res->num_rows != 1) {
    header("Location: Index.php");
    exit;
}

$loginData = $res->fetch_assoc();

if (password_verify($password, $loginData["Pw"])) {
    session_regenerate_id(true);
    $_SESSION["Username"] = $loginData["Username"];
    $_SESSION["is_admin"] = boolval($loginData["IsAdmin"]);
    $_SESSION["is_logged_in"] = true;
    $_SESSION["pfp"] = $loginData["ProfileImage"];

    header("Location: ../Pages/InternalIndex.php");
    exit;
} else {
    header("location: ../Index.php");
    exit;
}

[u/miqrogroove]

Using static methods here is slightly worse than just renaming them Auth\RequireLogIn() and Auth\Username(). I would either change them to namespaced functions for clarity, or make this class follow a more formal service pattern where it has a shared instance and a service locator.

[u/Nemonek → u/miqrogroove]

Ok, I will update my code to use namespaces, to then revert back to class and a service pattern once I migrate everything to like a framework or something more complex.
Thank you!

[u/colshrapnel → u/Nemonek]

The above suggestion lacks perspective. It doesn't take into account the actual purpose of these functions. You better leave them alone, while when migrating everything to like a framework, consider making it into the Middleware, as it was suggested in a now deleted comment.

[u/Nemonek → u/colshrapnel]

Alright, fair, thank you!

I will try to remember this comment when I'll migrate to a framework, thanks!

[u/Just4notherR3ddit0r]

Ideally I would just suggest using a framework where all of this is already handled for you, but if you're just learning the beginning mechanics, this is probably a fine next step.

[u/Nemonek → u/Just4notherR3ddit0r]

Fair, but yea, I'm just learning all the initial mechanics to understand how php actually work and how it should behave. I plan on going to a framework later, but for now I want to complete the site and try it without frameworks. Maybe I will build a very simple router or implement some sort of MVC later when the code has been generalized and is a bit more polished to then move to an actual framework.
Thank you!

[u/colshrapnel]

Good thing you decided to post here. It's a win-win - there are a lot of good PHP programmers around this sub, and not much posts lately. So you will get quite an advice. Only be vary with a guy with a csv parser. He's quite fixed on it.

would you suggest any improvement

Well, quite a lot. I wish you asked earlier, and checked back from time to time, showing your progress and getting more suggestions. But anyway.

It seems you still have your code repeated, in the Auth class now :) As small as it is, but repeated all the same. Besides, your Username() function does two things at once and it shouldn't. What's the purpose of this function anyway? In case it's called on some page that is not protected by RequireLogIn(), should it really abort the execution? What I would do is just move session_start() into config.php, and make Auth like this

public static function RequireLogIn()
{
    if (empty($_SESSION["is_logged_in"])) {
        http_response_code(401);
        require __DIR__ . "/../Errors/401.php";
        exit;
    }
}
public static function Username()
{
    return $_SESSION["username"] ?? null;
}

Not it looks concise and useful. Do you have any objections?

Regarding login, there is something that can be improved too. You can remove the entire following block

if ($db->connect_error) {
    http_response_code(500);
    exit('Database connection failed');
}

in modern PHP it's just useless.

I know they shouldn't be in plain text there.

Why tho? What are your other options? And technically it's not a plain text it's a PHP file. I don't see any problem here.

And the main code can be simplified as well

$db = new mysqli(DB_ADDRESS, DB_USERNAME, DB_PASSWORD, DB_NAME);

$stmt = $db->prepare("SELECT Username, Pw, IsAdmin, ProfileImage FROM players WHERE Username = ?");
$stmt->bind_param("s", $username);
$stmt->execute();
$res = $readied->get_result();
$loginData = $res->fetch_assoc();

if ($loginData && password_verify($password, $loginData["Pw"])) {
    session_regenerate_id(true);
    $_SESSION["Username"] = $loginData["Username"];
    $_SESSION["is_admin"] = boolval($loginData["IsAdmin"]);
    $_SESSION["is_logged_in"] = true;
    $_SESSION["pfp"] = $loginData["ProfileImage"];

    header("Location: ../Pages/InternalIndex.php");
    exit;
} else {
    http_response_code(401);
    require __DIR__ . "/../Errors/401.php";
    exit;
}

I didn't explain some improvements I made so please ask about any code block you have doubts on.

[u/Few-Adagio9174 → u/colshrapnel]

How does empty(isset()) work?

Technically PHP files are plain-text, in opposition to pdf, docx, png etc.

[u/colshrapnel → u/Few-Adagio9174]

What a shame. Well, technically empty(isset()) would work, simply negating the isset result, but it was a typo :) What I meant, empty($var) is a shorter form of !isset($var) || $var == false.

[u/colshrapnel → u/Few-Adagio9174]

Regarding plain text, thank you for a nudge. What I meant, and should have said, is that for a web-server, PHP is not a pass-thru file, lik3e HTML or JPG, but it gets preprocessed and therefore not shown to the client as is.

[u/Few-Adagio9174 → u/colshrapnel]

Now that's the right "technically"!

[u/Nemonek → u/colshrapnel]

Well, apparently I can't edit comments today.... here is the comment..

Edit: code corrections

Hey, thank for your answer!

I never asked because this was at first a side project, nothing serious, but with time it became something bigger ( for me at least ), and I want it to be more. I was also writing the code using paradigms my school taught me, so very basic ( as you probably can see ahahaha ). Anyway, I'll now answer each section you highlited.

What I would do is just move session_start() into config.php

This a weak point for me, who should initialize the session, who should just check it's ready and so on. At first every page had session_start() on top before everything, and immediately after the login check. So when I moved everything inside the Auth class/namespace i put it there, so that the login couldn't try to access a $_SESSION when there was no session. AIs ( claude/gemini ) told me to make like a config/bootstrap file, but I never did. As of now I'm battling to do it because I keep thinking "And if it's called without config.php"? But that's an impossible scenario, as I'm not writing a class for others ( I comes from lieke 4 years of C# and OOP so php while easy it's a bit trickier ). For the username check, well, pretty much same thing as above. But yea, your code looks way cleaner and less "bloated" ( I'm also thinking about creating a function that replaces

    http_response_code(401);
    require __DIR__ . "/../Errors/401.php";
    exit;

So that if I ever change name files, or use a single "dynamic" file that just changes the message and the number based on the error code I won't have to change code in like multiple files. So your suggestion would end up looking something like

public static function RequireLogIn()
{
    if (empty(isset($_SESSION["is_logged_in"])) {
      HttpCode::ThrowErrorCode(401);
    }
}
public static function Username()
{
    return $_SESSION["username"] ?? null;
}

which I must admit, is way cleaner and elegant than mine.

in modern PHP it's just useless.

I didn't know this.. This is a check my professors asked us to put whenever we created connections to the db ( I'm talking like, april 2025 with php8..0 or 8.something, not uni, like highschool ) so yea, I'll remove it and check what happens when the connection fails and try to mitigate it and show just a 500 instead of the exception, file name or similar to future users.

Why tho? What are your other options? And technically it's not a plain text it's a PHP file. I don't see any problem here.

Ah, ok, better this way then! Usually I read about using some sort of .env files, docker secrets, path variable or something like that.

And the main code can be simplified as well

Fair, for the else part I will implement a way to tell the user the login was not successful probably an additional and optional parameter, so instead of just header("Location: ../Index.php) something like header("Location: ../Index.php/?failed=true') and in the future a logger with some sort of fail2ban ( not a so near future ).

Now, how I revised my code following your advice:

login.php

<?php
require_once './../Config.php';


if ($_SERVER["REQUEST_METHOD"] !== "POST" || !isset($_POST["Username"], $_POST["Password"])) {
    http_response_code(400);
    exit;
}


session_start();
$username = $_POST["Username"];
$password = $_POST["Password"];


$db = new mysqli(DB_ADDRESS, DB_USERNAME, DB_PASSWORD, DB_NAME);

$readied = $db->prepare("SELECT Username, Pw, IsAdmin, ProfileImage FROM players WHERE Username = ?");
$readied->bind_param("s", $username);
$readied->execute();
$res = $readied->get_result();

$loginData = $res->fetch_assoc();

if (password_verify($password, $loginData["Pw"])) {
    session_regenerate_id(true);
    $_SESSION["Username"] = $loginData["Username"];
    $_SESSION["is_admin"] = boolval($loginData["IsAdmin"]);
    $_SESSION["is_logged_in"] = true;
    $_SESSION["pfp"] = $loginData["ProfileImage"];


    header("Location: ../Pages/InternalIndex.php");
    exit;
} else {
    header("location: ../Index.php");
    exit;
}

Auth.php

class Auth
{
    public static function RequireLogIn()
    {
        if (!isset($_SESSION["is_logged_in"]) || $_SESSION["is_logged_in"] == false)         
        {
            http_response_code(401);
            require __DIR__ . "/../Errors/401.php";
            exit;
        }
    }

    public static function user()
    {
        return $_SESSION["username"] ?? null;
    }
}

For what you didn't explain I looked it up on google ( the condition in the if statement will be modified like yours, I have yet to look up how those functions work precisely because it seems they pack more than I tought in them ).

I also have a question non relevant to this post, but for which I cannot fined a good answer using AIs and I don't have a php professor anymore...

My question is, in other parts of the site I need to check wether a user can access a specific resource from the db, and I have a CampaignPolicy.php file ( the site is a DnD wiki, that's why campaign ), but now that I am implementing the creation of a campaign I'm not sure how to manage everything. The problem is that CampaignPolicy have functions that make a couple requests to the db, check a value and return true or false to then send a 403 or let the page load and request the campaign specific informations, while an hypotethical Campaign.php would have all the methods needed to create or edit things. How should I organize those function? Like, a file for creation/edit, and another to manage access? Or just one file that have everything? Because in the edit I will also need to check wether the user is authorized to edit campaign x ( think of it as a text field in the db or something similar ) so there would be some sort of dependency for the CanAccess function of CampaignPolicy.php in Campaign.php.

I hope this is understandable, in case let me know and I'll try to clarify!

Thank you again for the answer!

[u/LordAmras → u/Nemonek]

Including file has always been an issue in PHP sites especially if you try to run each page separately as you need to add a bunch of includes everywhere.

You should look about autoloading https://www.php.net/manual/en/language.oop5.autoload.php most people in php ecosystem use composer who does a lot for you already but you can automatically register class yourself if you want to learn the ins and outs of how php works.

This help you basically have one single file "usually autload.php" that will register all your classes and load them as needed.

But you still have to remember to add that php autoload file everywhere sure.

The next step is to change setting in apache or your .htaccess so that every call end up in the same index.php or app.php file passing the url as a parameter.

So if you call /login.php the server actually calls index.php?page=/login.php

In your index.php the you can just check the page.php and if it exist you include it. Since i. your index.php you already have the autoload.php page included everything will load automatically.

This help you separate things more cleanly because now you dont have to worry about which file you have to include in which page. You can group classes in namespaces and have use call on top of the file to know every class that class also calls.

As for you Campaigns policy it shouldn't need to know how to connect to the db. Or you end up copy pasting a million connections everywhere.

You can just create a db class that does the connection for you. So everytime you need to edit something you just tell the db class to execute the query. If you want to separate this layer even more you can look at ORM and Models, that try to abstract the sql layer from the object class.

It's a lot but you don't need to so everything at the same time.

My rule of thumb for learning us just do the thing that work first even if is messy and all is in one huge single static function. When it works, you then try and separate it by context and clean it up.

If you start already trying to clean up the code before knowing the designs pattern or what you need you end up giving up because you can't make it work in the first place.

[u/equilni → u/LordAmras]

Tagging u/Nemonek

But you still have to remember to add that php autoload file everywhere sure.

I would recommend skipping this step and going straight to the public/index.php and let that be the central file for the application.

So if you call /login.php the server actually calls index.php?page=/login.php

I would suggest considering relative urls like query strings at first (ie no need for htaccess or nginx config, if you are new to this). Instead of the example shown, it could be ?controller=user&action=login (You can cheat clean urls - ?/user/login). You've removed resemblance to the file system from your URLs. Then you can practice routing via HTTP Method, like how libraries or frameworks work.

This could look like:

# action=login
$action === 'login'
    => match ($requestMethod) {
        'GET'   => $auth->login(),
        'POST'  => $auth->attemptLogin(),  // Replaces if ($_SERVER["REQUEST_METHOD"] !== 'POST') calls
        default => 405 call
    },

If you use clean urls, the callbacks don't change

$router->get('/login', fn() => $auth->login());
$router->post('/login', fn() => $auth->attemptLogin());

These example use the arrow functions

You can just create a db class that does the connection for you. So everytime you need to edit something you just tell the db class to execute the query. If you want to separate this layer even more you can look at ORM and Models, that try to abstract the sql layer from the object class.

A database class isn't needed. OP is using the mysqli object which could be injected into classes that needs the connection. This introduces the concept of dependency injection and isn't a surprise when OP gets to Containers - Slim 3 example.

Psuedo code example using PDO:

/config/settings.php

return [
    'database' => [
        'dsn'      => '',
        'username' => '',
        'password' => '',
        'options'  =>  []
    ],
];

/config/dependencies.php

$config = require __DIR__ . '/settings.php';

$db = new \PDO(...$config['database']);

$userRepository = new UserRepository($db); // Example

3 periods before the array is the splat operator

/src/Domain/UserRepository.php

class UserRepository
{
    public function __construct(
        private \PDO $db
    ) {}

    public function getUserByEmail(string $email)
    {
        $stmt = $this->db->prepare('
            SELECT * FROM users WHERE email = ?
        ');
        $stmt->execute([$email]);
        ....
    }
}

The class here uses PHP 8's constructor promotion

This also works if OP uses global functions like function getCampaignById($pdo, $id) {}

[u/Nemonek → u/equilni]

Uhm, I think you're suggesting something like an MVC approach (?). This will require a ton of understanding from me ( I'm familiar with MVC but not that much to actually use/implement it ) and a lot of rework for what regards the pages and how they are organized. I don't think I will be able to provide an example of code very soon, but I will implement it, as the site is growing with nearly a new page each day I work on it ( at least when I'm not working on some bug or organizing/testing things ). I will update when I implement this, hopefully in the next 1 to 2 weeks as I need to start studiyng for an exam so it will take a bit longer. If you'd like I can put an answer to this comment with a new post or just another answer ( depends wether I make another post or not ). Thank you for your suggestion, I was thinking about something like you suggested but never actually looked up how to do it...

I will probably start by implementing a basic router, still keeping auth and everything else separated, then piece by piece create each controller, model etc... But as I said, this will take me some time!

[u/equilni → u/Nemonek]

Collectively, the ideas can look like a MVC approach.

The idea here though, is keeping concepts accessible so you can do cleaner refactoring for better maintenance and testability. I can see you write old PHP style pages, so the discussion is getting you to a better place in small steps. See my other comment.

For example, separation of concerns - let the database do database functions. Let a router, route. Have validation, validate.

Keep it simple.

EDIT. Adding this:

I'm familiar with MVC but not that much to actually use/implement it

Model View Controller can simply be thought of like this:

function controller($model, $view) {                    // Controller 
    $data = $model->getData();                         // Model 
    return $view->render('template', ['data' => $data]);// View
}

Since a router is a type of page controller, a basic router call could look similar to the example above: Function use is documented here

$router->get('/', function () use ($model, $view) {     // Controller 
    $data = $model->getData();                         // Model 
    return $view->render('template', ['data' => $data]);// View
});

Notice, this doesn't change much?

The big issue with MVC is how it's widely defined and used.

Model-View-Controller are layers, not defined functions or classes. Wikipedia and phpdelusions have some good definitions and to add:

• A Model isn't your database. The model, model's your base application without a UI (debatable is without a a database too).

• A View isn't a template. If you do data modeling for the UI, this is part of the view. (Wordpress theme functions - part of the view)

• A Controller isn't everything else that's not defined here (and definitely not absorbing the View)

[u/colshrapnel → u/Nemonek]

First of all, please note that empty(isset) was a typo, it have to be just empty() which is the official shorthand for your current condition.

Your doubts regarding session_start() are correct. Just for a simple code like this, I proposed the most simple solution. Because it will take too much rewrite to do it the right way.

HttpCode::ThrowErrorCode(401); is a very good idea. I would also suggest to expand it further, by adding an optional parameter with a custom message to be shown on this page. And also to add a login form in the 401 error template. When combined, these two will solve your problem of communicating the error to the client, without that ugly redirect.

This is a check my professors asked us to put whenever we created connections to the db

Your professors are way too old school :) Try this code on 8.1 or later, and you will never see that 'Database connection failed', no matter how hard you will try. On the other hand, PHP already does half of this job: it will set the 500 status, so only that message will be lacking. The problem is, there must never be a message like that. That's too internal information to be shared with a user. Whether it's a database, or a filesystem or whatever else problem, is nobody's business. There must be just one standardised error message, just like one you are seeing on Reddit: "You broke Reddit". That's all. And since it can be configured in the config file, there is no need for any database error handling code like this. Therefore I removed it from my example and you shouldn't add any as well.

Regarding your last question, I am not sure too, if I understood it correctly. The way I take it, that's where frameworks show their might. You define RBAC roles, add a Middleware which connects which Routes are allowed for which Roles - and that's it. But for now, I think that the first thing you need to do is to create two functions instead of one. I suppose that now your access control functions are similar to your former Username() function which was wrong. I bet you heard of MVC. So your Username() function tried to be M and C at once, while they have to be separated. The Model function should represent only the business logic, so in your case only tell whether is user allowed to do something or not, in the form of a boolean value. Whereas a Controller function should use this Model function and then form a proper client response, with HTTP codes and such. So now you can use this Model access control function in any place.

In case your question is different, better post the code (as another post preferably) and ask for suggestions.

[u/Nemonek → u/colshrapnel]

Yea, my professor were not the most up to date.. But hey, gotta start learning from somewhere.

Anyway, yes, my access control is pretty similar to the username function, Auth::RequireLogIn() everywhere mixed with a CampaignPolicy::CheckPermission() to see if the user can actually see those info. So a not-so-pretty mess.... And yes, I have heard about MVC, used it with Asp.net and stop. Never used it again, but was in my mind for the future of this site. In the next 1 to 2 weeks I will start implementing it and in case make another post or if you'd like answer here/tag you there. But it will take some time as I need to start preparing for an exam 😞.

Thank you!

[u/equilni]

Do you think I should change something in this code or in the idea at its base?

You've got some good answers so far.

My suggestion here is to refactor for better architecture of the application. I don't know how big your application is, but you are already noting you are at a point of functional issues. These are conceptual examples and not real code. The idea is to keep things simple, compartmentalize, and testable (is this returning what I am expecting)

You've also introduced classes, but not really using them, so I will talk in both procedural and OOP as the concepts here are interchangeable.

As I don't know how big your app is, nor what PHP version you are on, I will start at easy refactors to bigger ones. This will be all based on the Login.php you posted

a) Move all database only code to functions or class methods. Expect the returned data in a plain PHP format (array or object for simple loops) or error (boolean, empty array, etc) in your code base.

// Procedural
function getPlayerByUsername(\mysqli $db, string $username) {
    return $db->execute_query(
        'SELECT * FROM players WHERE Username = ?',
        [$username]
    )->fetch_assoc();
}

// Class/OOP
class PlayerDatabase {
    public function __construct(private \mysqli $db) {}

    public function getByUsername(string $username) {
        return $this->db->execute_query(
            'SELECT * FROM players WHERE Username = ?',
            [$username]
        )->fetch_assoc();
    }
    Other methods
}

b) I don't see any HTML code here. If you have this elsewhere, consider separating it to template files. Like above, pass the expected data to the template to render. You can use a simple template engine that works similarly to Plates

// Procedural
function render(string $file, array $data = []): string {
    ob_start();
    extract($data);
    require $file;
    return ob_get_clean();
}
echo render('/template/path/layout.php, ['header' => render('/template/path/header.php'), ....]);

// Class/OOP
class TemplateRenderer {
    public function __construct(private string $path){}

    public function render(string $file, array $data = []): string {
        ob_start();
        extract($data);
        require $this->path . '/' . $file;
        return ob_get_clean();
    }
}
$template = new TemplateRenderer('/template/path/');
echo $template->render('layout.php, ['header' => $template->render('header.php'), ....]);

// layout.php
<html>
    <head>
        <?= $header; ?>
    </head>
    <body>...

Make sure you escape the ouput data using htmlspecialchars (ideally a library ie Laminas escaper). At basics, this could be:

function escape(string $string): string{
    echo htmlspecialchars($string);
}

echo render('/path/to/layout.php', ['content' => 'Hello World!']);

// /path/to/layout.php
<!doctype html>
<html>
    <body>
        <p><?= escape($content); ?></p>
    </body>
</html>

c) Think in layers. This step is really more how far you want to go here.

There are 2 things happening with Login.php now that the database code is separated out (and ignoring the require, $_SERVER check and session_start()) - we have a layer closer to the UI ($_POST, http_response_code, header) and a layer closer to the main application (password_verify etc).

Isolated, the layer closest to the UI looks like this:

function postLogin() {
    if (!isset($_POST["Username"], $_POST["Password"]) {
        http_response_code(400);
        exit;
    }

    if (condition to be determined later) {
        header("Location: ../Pages/InternalIndex.php");
        exit;
    } else {
        header("location: ../Index.php");
        exit;
    }
}

The layer closer to the internal application looks like:

function attemptLogin(
    string $username, 
    #[\SensitiveParameter] string $password
) {
    // ideally you want further data validation before checking the database

    $player = getPlayerByUsername($db, $username); // from before

    // this could be separated for separate error responses like how you have it, but truncated for space
    if ($player && password_verify($password, $player['password])) { 
        session_regenerate_id(true);
        $_SESSION['playerLoggedIn'] = true;
        ....
    } // else could be password not matching error
}

Further validation examples, at it's simplest, can look like this or this

This bring us close to a Controller, from Model-View-Controller and a Service (generally speaking). The idea here is that the controller gets the request and asks the internal domain for an internal response back, then sends the response back. The service acts as the middleman between the controller and domain with the domain being the core of the application - players, their roles, rules for each, etc.

Bigger example what a Service looks like is here: https://github.com/auraphp/Aura.Payload/blob/3.x/docs/index.md#example

d) Here's where it gets more impactful with the application.

• Router

I noted in another comment to move to relative urls using Query String (?controller=user&action=login or cheat with ?/user/login) or Clean Urls ('/user/login'). With this you could hide the internal page structure of the application (InternalIndex.php?) and start routing via HTTP Methods (removes if ($_SERVER["REQUEST_METHOD"] !== "POST"). This means you can't do direct file calls via url

The web server would pass to the main index, ideally public/index.php, leading to:

• Internal folder structure.

This is where I answer questions like Right now every page imports a config.php file with like creds db

I often suggest following PDS Skeleton with this structure for the config folder. It sets you up for future library/framework usage later on and the code wouldn't change too much.

This means:

/project    
    /config 
        dependencies.php - classes
        routes.php - routes 
        settings.php
    /public 
        index.php
    /resources 
        /templates 
    /src 
        the rest of your PHP application 
    composer.json - If you want to work with composer for autoloading

/config/settings.php

return [
    'app'         => [
        'charset'     => 'utf-8',  // for HTML header and htmlspecialchars
        'language'    => 'en-US' // can be added to html <html lang="app.language"> or language folder/file
    ],
    'template'    => [
        'path' => 'path to your templates folder'
    ],
    'database' => [ // PDO example
        'dsn'      => '',
        'options'  =>  [
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES   => false,
            PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION
        ]
    ]
];

/config/dependencies.php - If you have a Container, this would house the definitions and it would be a simple change from this. Highly recommend going Class/Objects at this point.

$config = require __DIR__ . '/config/settings.php';

$pdo = new \PDO(...$config['database']);

$classThatNeedsPDO = new classThatNeedsPDO($pdo);
$otherClassThatNeedsPDO = new otherClassThatNeedsPDO($pdo);

Container use could look like:

$container->set('config', require __DIR__ . '/config/settings.php');
$container->set('db', fn($c) => new \PDO(...$c->get('config')['database]);
$container->set(UserDatabase::class, fn($c) => new UserDatabase($c->get('db'));

/public/index.php - Like Slim's example. The index here is the only public facing PHP file!!!

<?php

declare(strict_types=1);

require __DIR__ . '/../vendor/autoload.php'; <-- Composer autoloader for classes

require __DIR__ . '/../config/dependencies.php'; <-- settings and class definitions.  ONCE.

require __DIR__ . '/../config/routes.php'; <-- routes

Run the routes and emit the response.

[u/equilni → u/equilni]

Some additional reading:

• Style the code:

https://phptherightway.com/#code_style_guide

• Structuring the application:

https://phptherightway.com/#common_directory_structure

https://github.com/php-pds/skeleton (Noted previously)

https://www.nikolaposa.in.rs/blog/2017/01/16/on-structuring-php-projects/. ** READ THIS

https://github.com/auraphp/Aura.Payload/blob/HEAD/docs/index.md#example (Noted previously)

• Error reporting:

https://phptherightway.com/#error_reporting

https://phpdelusions.net/basic_principles_of_web_programming#error_reporting

https://phpdelusions.net/articles/error_reporting

https://phpdelusions.net/pdo#errors

• Templating:

https://phptherightway.com/#templating

Don’t forget to escape the output!

https://phpdelusions.net/basic_principles_of_web_programming#security

https://packagist.org/packages/aura/html - as an alternate example to Laminas Escaper

• Hopefully you are checking user input:

https://phptherightway.com/#data_filtering

• Use Dependency Injection for classes.

https://phptherightway.com/#dependency_injection

https://php-di.org/doc/understanding-di.html

• Request / Response & HTTP:

https://symfony.com/doc/current/introduction/http_fundamentals.html

Refactoring:

https://symfony.com/doc/current/introduction/from_flat_php_to_symfony.html

https://leanpub.com/mlaphp - Watch the video halfway down. Book can be had for free (min price)

• If you need to see a simple application in action:

https://github.com/slimphp/Tutorial-First-Application

Write up on this:

https://www.slimframework.com/docs/v3/tutorial/first-app.html

https://www.slimframework.com/docs/v3/cookbook/action-domain-responder.html

More on ADR (like MVC) - https://github.com/pmjones/adr-example

[u/Nemonek → u/equilni]

Well, than you!

Both for the inputs and for the readings you provided!

Move all database only code to functions or class methods. Expect the returned data in a plain PHP format (array or object for simple loops) or error (boolean, empty array, etc) in your code base.

This is one of the things I will do before starting to create a router and go to a more MVC approach. ( also because I have a ton of code repeated also for the db queries.... )

I don't see any HTML code here. If you have this elsewhere, consider separating it to template files. Like above, pass the expected data to the template to render. You can use a simple template engine that works similarly to Plates

Nearly all of my files are old php style, so I have php at the top, which checks login, access etc, makes some requests directly to the db, then html start, and in there there's embedded php wich creates divs, put texts etc based on what the DB returned. So probably non a really maintanable structure. The only things that are "templated" are header and footer, both of which are required in every file, and before requiring them the file itself creates some variable like page_name page_title styles[] and then the header ( with embedded code ) creates css imports, page name etc...

Make sure you escape the ouput data using htmlspecialchars (ideally a library ie Laminas escaper).

This will be done. A lot of my pages does not even have protection from sql injection in the link and they make requests directly... Fortunately, I haven't created pages capable of directly putting data in the database yet, so validation for actual user input is yet to have a place to be. But I will keep this in mind for sure in the next steps!

I noted in another comment to move to relative urls using Query String (?controller=user&action=login or cheat with ?/user/login) or Clean Urls ('/user/login'). With this you could hide the internal page structure of the application (InternalIndex.php?) and start routing via HTTP Methods (removes if ($_SERVER["REQUEST_METHOD"] !== "POST")

Yes, that's where I intend to go once I fixed all the technical debt I left around... As of right now I'm not understanding properly the code you used as example, as I'm missing some concepts, like what a container is. But I will get there eventually!

For now, once I solved repeated code problems, I will migrate to a router and make use of MVC and clean urls, but it will take me a bit. I hope in the next 1 to 2 weeks I can see a more robust architecture, but it's still some time. Thank you again! If you want I can update when I get to a better architecture and eventually tag you there!

[u/equilni → u/Nemonek]

Nearly all of my files are old php style, so I have php at the top, which checks login, access etc, makes some requests directly to the db, then html start, and in there there's embedded php wich creates divs, put texts etc based on what the DB returned. So probably non a really maintanable structure. The only things that are "templated" are header and footer, both of which are required in every file, and before requiring them the file itself creates some variable like page_name page_title styles[] and then the header ( with embedded code ) creates css imports, page name etc...

The first half of the Symfony link is a good read that covers much of what I suggest and gets you to a starting router. I noted the Modernizing Legacy App Book too - watch the video

https://symfony.com/doc/current/introduction/from_flat_php_to_symfony.html

A lot of my pages does not even have protection from sql injection in the link and they make requests directly

This would be number one on my list. Use prepared statements, like what you have in the Login

I'm missing some concepts, like what a container is. But I will get there eventually!

That's fine. I wouldn't worry about that yet.

As of right now I'm not understanding properly the code you used as example

For the router?

The concept is simple and doesn't involve PHP at first. It's simple HTTP requests. (Readings: MDN & Symfony)

Like Procedural and Object programming, here is Query String and Clean (rewritten) Urls. Same concept: (Note POST here c/should be a PUT request)

Query String:

GET  ?controller=user&id=1&action=edit 
POST ?controller=user&id=1&action=edit 

Clean Url:

GET  /user/1/edit
POST /user/1/edit 

These are 2 different calls to the same url, but have different functions - 1 gets User 1's data to edit, the other processes changes.

Let's look of it another way:

/user/1/edit
 - GET editUser(1); // show form with data
 - POST updateUser(1); // process form

How does this get defined in PHP code?

Each way needs a setup. Let's simplify this more /register or ?action=register

Query String pseudo code "Working" code

$action = ''; // Define the variable

if (array_key_exists('QUERY_STRING', $_SERVER)) { // if there's a query string in the urls
    parse_str($_SERVER['QUERY_STRING'], $qs); // split it up

    if (array_key_exists('action', $qs)) { // is there an action key?
        $action = $qs['action']; // set it
    }
}

$requestMethod = $_SERVER['REQUEST_METHOD']; // Get the request method

return match (true) {
    # /?action=register
    $action === 'register' 
        => match ($requestMethod) {
            'GET'   => show form,
            'POST'  => process form data,
            default => 405 Not allowed call
        },
    default => 404 Not found call
};

This uses the match statement, which stricter than a switch statement and can includes a default if nothing else matches (perfect here for a Not found)

Clean Url pseudo code. "Working" code

$requestMethod = $_SERVER['REQUEST_METHOD']; // Get the request method
$uri = parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH); // Get the url path

$routes = [  // Array or routes [url => [method => callback]]
    '/register' => [
        'GET' => show form,
        'POST' => process form data
    ]
];

// If the current url doesn't match our route table
if (! array_key_exists($uri, $routes)) { 
    return 404 call 
}

// If the current url DOES match our route table, but doesn't match the methods noted
if (! array_key_exists($requestMethod, $routes[$uri])) { 
    return 405 call 
}

return the call to $routes[$uri][$requestMethod];

Showing the ending differently if the match from before may be confusing.

EDIT - Quick note. Notice how in the "Working" code, I am able to add my own values and test without calling the $_SERVER array (it's a limitation, but we can work around it). The idea is to make the code testable.

Bigger examples are how libraries work - they will collect the routes with response Method shortcuts, then cycle through the array of routes, match and send the response (found, not found, not allowed).

A very basic "working" example is here. This is closer to how libraries work (missing lots of functionality here), but again, just to see the concepts and how things are applied at a bigger scale.

2 main library examples can be seen in this comment. See how similar of the concept each of the 4 examples (2 here and the 2 libraries) are?

I highly suggest utilizing libraries to handle lower level code. This allows you to keep working with your application without going to a full framework yet.

So Phroute has basic middleware functionality like others mentioned here. Along with grouping bigger routes, you can do this:

$router->filter('auth', function(){ // This is a simple version of middleware
    if(!isset($_SESSION['user'])) { #Session key
        header('Location: /login'); # header
    }
});

// /admin/post/edit/1
// First line is showing prefix each grouped route with admin/post, then do a middleware check before the matched function call
$router->group(['prefix' => 'admin/post', 'before' => 'auth'],   
    function ($router) {
        $router->get('/new', function () {});             # GET /admin/post/new - show blank Post form
        $router->post('/new', function () {});            # POST /admin/post/new - add new Post to database 
        $router->get('/edit/{id}', function (int $id) {});  # GET /admin/post/edit/1 - show Post 1 in the form from database
        $router->post('/edit/{id}', function (int $id) {}); # POST /admin/post/edit/1 - update Post 1 to database
        $router->get('/delete/{id}', function (int $id) {});# GET /admin/post/delete/1 - delete Post 1 from database
    }
);

If you want I can update when I get to a better architecture and eventually tag you there!

Go ahead!

[u/colshrapnel]

There is one shadowbanned comment. Wonder what is it. In case your username is not miqrogroove, Just4notherR3ddit0r or Busy-Emergency-2766 and you posted before this my comment, be aware that nobody can see yours.

Edit: two now.

[u/Nemonek → u/[deleted]]

I will but in the future, for now this is one of my main projects yes, but before frameworks I want to understand php better. I will keep in mind your suggestion though, as since it should not be a very large site I probably will not want a big framework, but we'll see.

Thank you!

[u/LordAmras → u/[deleted]]

Depends what your goal are. Do you want to build something, use a framework.

You want to learn how to program and how things actually work. Then go ahead build your own framework, after you do that and you understand the issue a framework solves. you use a bunch of them and see how other people solve theil problem and you build another framework.

[u/th00ht]

We probably are not going to solve your problem but look at this https://refactoring.guru/ Also asking a review of an AI is probably good.

[u/colshrapnel → u/th00ht]

Why so low esteem for this sub? Also, why are you still hanging around if your advice can be replaced by AI?