9
\$\begingroup\$

I've started learning PHP for university-related projects today. I tried implementing a basic blog/forum. Here's the full source code.

It has the following features/design-decisions:

  • There are posts, which have the following fields: title, author, date and contents.
  • Posts have comments, which have the following fields: author, date, contents.
  • There is a basic login system: users can register/login with an username and password.
  • JSON files are used for data storage instead of a MySQL database.
    • There is a JSON file for posts, posts.json, which contains posts and comments.
    • There is a JSON file for users, which contains usernames and password hashes.

I'm looking for a full code review: I've only read the PHP manual today and while I understand most of its features I have no idea what the best practices are and how things should be implemented.

I will list below some of my main concerns with the code/design.


I have a core.php file which is included with require in every other PHP file.

It links the stylesheet at the beginning (and it could also import JS files):

<head>
    <link rel="stylesheet" type="text/css" href="./CSS/style.css">
</head>

Is this the correct way of adding the same <head> to every file?


Every time I want to access the user database or the posts database, I call these functions:

function getUsersDB() { return new UsersDB(); }
function getPostsDB() { return new PostsDB(); }

However, creating a new UsersDB or PostsDB instance forces a read from the .json file. I need to access these databases in multiple PHP files.

Is there a way to "cache" these UsersDB and PostsDB instances? Something similar to C++'s static, that can be shared between all PHP scripts?


When I have to generate something between HTML tags, I just call print like this:

print("<strong>");  
print($mPost["author"]);
print("</strong>");

Is there an easier/better way of generating open/close tags? Should I create a function that takes a lambda like:

function doBetweenStrong($lambda)
{
    print("<strong>");  
    // execute $lambda here
    print("</strong>");
}

Every page of my website has the same menu and "login status" bar. I simply call Builder::printCommon(); at the beginning of every page. Is this the best way to do it?


I keep track of the current logged-in user in $_SESSION.

class Credentials
{
    static public function isLoggedIn()
    {
        if(!isset($_SESSION["currentUser"])) return false;
        if($_SESSION["currentUser"] == "") return false;
        return true;
    }

    static public function login($mUser)
    {
        $_SESSION["currentUser"] = $mUser;
    }

    static public function logout()
    {
        unset($_SESSION["currentUser"]);
    }

    static public function getCurrentUserName()
    {
        return $_SESSION["currentUser"];
    }
}

Is this safe? What is the correct way to keep track of the currently logged-in user?


I find it hard to show the user error/success messages (for example, after registration, or after login, or after posting something). The current solution is creating a new page like registerSuccess.php or registerFailure.php.

Should I create a general message.php and pass the desired message to it using $_SESSION?


Paths. Is there a reliable way to get the server's root folder?

I'm currently using ./, ../ and ../../ to get the files I want from scripts. What if I want to move a script in the future? I would have to change all paths manually.

What if I include a script into another? Wouldn't the paths be resolved incorrectly?

\$\endgroup\$
  • \$\begingroup\$ I'd usually make a constant to hold the script path, and make everything relative to that. If the script path changes, you'd only have to change it in one place. \$\endgroup\$ – Ivo Coumans Oct 14 '14 at 12:56
4
+50
\$\begingroup\$

First of all, it's good that you try to implement something like a blog/forum on your own. It will teach you a lot of good/bad practices. In one comment you said, that you are trying to understand how these frameworks could be impemented, so it would be very good, if you take a look at them. Check out their APIs and see what they did.

Im used to Symfony (http://symfony.com/) so my advice will mostly use their architecture as an example. However, be sure to remember that there are many ways to success and the things I describe are my current point of view.

Your major problems (maybe not now, but in a few days/weeks/months) are the structure of your project, the structure of your source code and the mixing of php/html.


Project Structure

Currently, you have a few php files in your root directory and a few php files in your PHP directory. On a first glance, it seems, that the PHP directory contains your framework and the root folder contains the views, which use this framework. If that's the case, make it obvious.

Advice #1: Give your framework a name and rename the folder accordingly.

That way, you will have an easier time to find the place you are looking for. Which leads to the next problem: Your core.php contains multiple class definitions. If you continue doing this, your file will grow exponentially and it will take you a long time to find the line you want to modify.

Advice #2: Put each php class in a separate file.

The next step is to use namespaces to further structure your classes and files. It might be more difficult to begin with, but will help you in the long run. Additionally you might want to look into autoload. Used correctly, this should solve your troubles with include and require.

Advice #3: Use Namespaces and autoload

Another possible improvement are the files in your root directory. Currently you probably access each file directly via www.whatev.er/viewPost.php or www.whatev.er/createPost.php. I highly recommend to take a look at something like a controller/routing mechanism and only use ONE file (index.php) to access your website. Take a look at .htaccess and URL Rewriting. This is actually a pretty powerful and extensive technique. So just a short explanation: Every (nearly every) call to your website is redirected to your index.php. So www.whatev.er/blub/bla/1234 or www.whatev.er/post/create all call your index.php. In the index file, you can parse the requested uri and call the appropriate actions.

Advice #4: Use .htaccess and URL Rewriting and only one file as an entry point to your website.

Advice #5: Use some kind of controller/routing mechanism to handle actions depending on your route.


Source Code Structure

As already mentioned, only define one class per file. I like it, that you encapsulate the fields of your classes. However, you have many static functions, which basically degrades your classes to some kind of namespaces. There is nothing wrong with static functions, but be careful not to overuse them.

Advice #6: Don't overuse static functions (and statics in general)

As you mentioned, getUsersDB() and getPostsDB() is suboptimal. You have pretty much three options:

  1. Use global objects
  2. Pass a reference where it is needed
  3. Use dependency injection

The use of global objects is generally discouraged, but migh be okay given your current experience. Another (slightly) better option is to instantiate the database once and pass it to the method that needs the database. The imho best option is to use dependency injection. However, this is pretty advanced stuff and something you want to look into in the future.

Advice #7: Use dependency injection if possible. Globals might be okay for now, but lead to trouble in the future.

You say, that you have posts and comments. However, in your code you only deal with assoc arrays. Why don't you create classes for Post and Comment? This is the perfect place to use a class.

Advice #8: Use classes for entites (Post/Comment) and not assoc arrays.

If you use classes for Post/Comment, you might wonder how to write them to json. PHPs jsonSerialize will help you there. However, you probably have to convert json -> Post/Comment yourself.

Advice #9: Use jsonSerialize or something similar to serialize objects to json.


Mixing of PHP/HTML

This is probably the problem, that will annoy you the most in the future. As it will make dealing with outputting stuff way more complicated than it needs to be. I highly encourage you to use some kind of templating system to render templates and display html. Don't ever output or print html code in a php file. Take a look at twig (used in eg. Symfony). Something like that will extremely improve your codebase. Your problem with printing will vanish and you can use common parts of html code without any problems.

Advice #10: Use some kind of templating mechanism to output HTML.

\$\endgroup\$
3
\$\begingroup\$

Your full code would be a bit too much to review. I recommend to break it down to pieces and post each for multiple separate reviews.


Is there a way to "cache" these UsersDB and PostsDB instances? Something similar to C++'s static, that can be shared between all PHP scripts?

For one thing, you could use include_once or require_once to make sure that these objects are initialized once per client request (once per page load).

Also, you could implement these classes using the Singleton pattern, as explained very nicely in this article.


Is there an easier/better way of generating open/close tags? Should I create a function that takes a lambda like:

function doBetweenStrong($lambda) { ... }

That sounds better. But instead of a lambda, why not simply the content to be printed? I recommend to rename the method to simply strong, and make it a static method inside an HtmlHelper class.


Every page of my website has the same menu and "login status" bar. I simply call Builder::printCommon(); at the beginning of every page. Is this the best way to do it?

That seems quite fine. I suppose the printCommon prints more than just the menu and the login toolbar. Otherwise the name is too generic and it should be renamed to better reflect what it does, for example printNavbar.


As per your Credentials class, first of all it's not a very good name. The name suggests a simple object that contains only data, like the username and password. The class in your example has behavior: it is used to perform login, logout, and check session status. SessionManager would be a better name to reflect what it does.

Is this safe? What is the correct way to keep track of the currently logged-in user?

This seems safe enough, as long as you make sure that login($mUser) will only be called from well controlled places in the code. Ideally from only one place, right after the user is authenticated.


I find it hard to show the user error/success messages (for example, after registration, or after login, or after posting something). The current solution is creating a new page like registerSuccess.php or registerFailure.php.

Should I create a general message.php and pass the desired message to it using $_SESSION?

I don't think there's a silver bullet for this. If you want to show two different pages for success and failure, you'll need two pages. Of course, if the only difference in them is a short part in the middle, then the two pages should be derived from a common base template, and implement only the differences.

Using $_SESSION to pass internal details between requests for such mundane tasks is not a good idea.


Paths. Is there a reliable way to get the server's root folder?

This is always tricky. Are you sure you want to limit yourself to installing in the server's root folder? It's nice when a framework gives you the freedom to install inside a subfolder of a domain, not necessarily the root.

Either way, the problem is of course the same: your scripts need a fixed point in the filesystem shared by all of them, in order to make cross-references reliably.

As a simple solution, you could define a PROJECTPATH variable at the top of every script, where it's easy to see, containing the relative path to the project's root. Of course, if you move scripts to different directories you will need to update the variable in all those files. But I don't know a better way.

What if I include a script into another? Wouldn't the paths be resolved incorrectly?

It will work. The relative paths will be resolved relative from the current script. For example, if a script t1.php includes src/t2.php, which includes t3.php, that will look for src/t3.php, not t2.php in the current dir. That is, the current directory for src/t2.php is src.

\$\endgroup\$
2
\$\begingroup\$

You don't want to add the same head section to every file. You should have the ability to (for example) add more CSS or Javascript to a specific page. You also may want to add meta tags, etc. to your pages. At minimum, you probably want to add a title to your page, which should be different on each page.

All that said, no, you are not creating your common code correctly. Your core.php file does two things. One, it creates output. Two, it defines things like classes. You should pick one task for it. Either create HTML output or define functions/classes for later use. As a general rule, the function and class definitions should occur before you produce any output. Output should be close to the last thing you do. This allows you to do things like redirect to a different page. Your code would complain that you already produced output if you did that.

Your head section is also missing code. Prior to creating your head section, you should have a DOCTYPE and an html tag. Here's the code from this page:

<!DOCTYPE html>
<html itemscope itemtype="http://schema.org/QAPage">
<head>
    <title>php - Blog/Forum implementation - Code Review Stack Exchange</title>

Notice how the title is unique to this page. I'm not as sure of the DOCTYPE and html tag. That's not the way that I learned how to write them, but perhaps I'm not up on the latest.

I'd also tend to break the class and function definitions out of core.php. The usual rule of thumb is one class per file. More than one function can go into a file. You can include the files from core.php.


You can't share variables between different pages without storing them somewhere in between (e.g. the session). If you just want to share between all the scripts used by a particular page, you can just create global variables in PHP. In fact, any variable you create outside something with limited scope (e.g. a class or function) will have global scope. So you could replace those two functions with

$usersDB = new UsersDB();
$postsDB = new PostsDB();

Inside a function, you'd tell it you want to access globals by

global $usersDB;
global $postsDB;

Alternately, you could change your functions to look like

function getUsersDB() {
    static $usersDB;

    if ( ! isset($usersDB) ) {
        $usersDB = new UsersDB();
    }

    return $usersDB;
}

See static scope variables for more information.


There's no real advantage to the function that you define there. It's about the same amount of code either way. You may find other examples where there is a real advantage to defining a helper function like that. The big question is if it makes your code shorter and simpler.

Incidentally, I'm more used to writing that with echo. Sticking to your print notation, I might say

print('<strong>' . $mPost['author'] . '</strong>');

Note that I also changed the quotation marks. Double quotes allow variable interpolation, which you weren't using. Single quotes are actually slightly faster, so they are preferred for most strings.

You may want to add some sort of output protection too though. For example, if $mPost['author'] is based on user input, you might do

print('<strong>' . htmlspecialchars($mPost['author']) . '</strong>');

or define a function to do it for you.

print('<strong>' . Builder::protectedOutput($mPost['author']) . '</strong>');

Using the function can allow you to change how you protect the output in many places at once. Without the function, you'd have to change it everywhere it's used.


Builder::printCommon() is a class method. You can do that, but in PHP it's more common to include a module to do that and to put the HTML in the module. You could also put your head section in the same module. You'd call it like

include(DIR_WS_MODULES . 'theme_top.php');

And it would look something like

<!DOCTYPE html>
<html itemscope itemtype="http://schema.org/QAPage">
<head>
    <title><?php $builder->printTitle(); ?></title>
    <link rel="stylesheet" type="text/css" href="./CSS/style.css">
    <?php $builder->printExtraHead(); ?>
</head>
<body>
  <div class="mainMenu">
    <ul>
      <li><?php Builder::printLink('Index', './index.php'); ?></li>
      <li><?php Builder::printLink('Create post', './createPost.php'); ?></li>
      <li><?php Builder::printLink('Register/login', './registerLogin.php'); ?></li>
    </ul>
  </div>
  <div class="loginStatus"><?php Builder::printLoginStatus(); ?></div>

Note that some of the Builder accesses would not be static. These are the ones where different parts of the program may modify what they output.


The Credentials class is reasonably safe, although I am more accustomed to seeing the user ID saved than the user name. There could be issues elsewhere. For example, is the cookie info only passed via HTTPS? How secure is the session?


It used to be that there was often register_form.php, register_process.php, and register_success.php, but the more modern way is to do everything with register.php (or even index.php?page=register). The only time that I've seen problems with this is when you redirect after processing the input. Yes, in that case, you may have to store the message(s) in the session and create a module to display them.


Part of the problem is that you are running some of your pages out of sub-directories. It is more common to put all the externally accessible scripts in the same directory.

Then you can make constants that relate the sub-directory locations to the root directory. That way if you move your classes directory, you just have to change one constant.

As a general rule paths are determined based on the location of the script handling the page request rather than on the locations of any scripts included by that script. That way you don't have to worry about how any script is called except the one that is directly related to the page request (e.g. index.php or doRegister.php).

\$\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.