steve228uk/CandyCMS — pull request #70 by ringmaster (2012-08-21 17:44:13+00:00)
Title: Don't pull this

Hello! You probably don't know me, but I stumbled upon your project yesterday or the day before, as it was pointed out you have a nice site design. I was intrigued and looked into your code. Uh-oh.

This pull request should not be pulled, as it is incomplete. Instead, you might consider some of the ideas I've spent a couple of minutes on and provided in code. I don't have the time to finish these revisions, but I think they will be of use to look at.

In particular, you need to **stop inserting raw data into queries**. It's imperative. There are ways to basically trash your database *right now* just by screwing around with the login form submissions. (Input this as a username, for example: `"; create table foo(id int);`) Instead, use PDO's named parameters. Look at any of the revised queries in the code in this request to see named parameters at work.

You should also consider using DRY (don't repeat yourself) principles in your coding. You're creating instances of database cursors all over the place. Not only is it multiple lines of code for each query, but depending on how PHP is doing garbage collection, you could be leaving dozens of cursors open to the database, resulting in connection exhaustion under load. In this pull request, I've centralized the database calls and added commonly used functions so that you can do basically anything with the database and get useful results in one command.

PDO supplies a lastInsertId() method. Instead of querying for the highest id of the table, use the PDO method. There is only one reason I can think of for why you might not want to do what I've done in the code in this pull request, but you're not using Postgres (as far as I can tell), so that reason is irrelevant.

Having to set the $Candy variable as global everywhere is a real downer. Use a static class to store that stuff. I've done some crazy stuff with the CandyCMS object (which, if you're going to use it like I have, should probably be named just "Candy" for brevity, because it's used everywhere) that I don't even recommend unless you know what you're intent is with the object. My thought was to allow extensions on that core class using __invoke() and the $properties array, but I don't know if that aligns with your intended future use of that global. Regardless, the global feels wrong.

There are a handful of other revisions I haven't made. That the admin uses some separate files from the main view is slightly perplexing. I can think of some reasons why this might be the case, but once again, I'm not seeing any evidence of those needs in the project.

The installer also failed for me, I think because I'm running PHP under FastCGI. The location of the application was not found correctly with `$_SERVER['DOCUMENT_ROOT']`. It ends up pointing to my /etc/apache directory for some reason. Anyway, there are probably better ways to determine this.

There's a handful of other stuff I'd like to mention, but I'm not sure of the best method to do it. This certainly isn't it. I added an issue on your repo for the SQL Injection thing, which is super-important. The rest of the stuff you can probably take or leave. Sadly, I don't see a great way for people to do code reviews over the web yet, especially uninvited. And really, you didn't ask for this, so you know... Strange, right? I just figured you might benefit from someone who's done this before, instead of having to learn the hard way, or never learning at all.