Christian F.

Psycho: Regarding the redirect, it's there to prevent F5/refresh-resubmitting. At least, that's why I'd add it.

First of all I'd just like to congratulate you too, you seem to be progressing nicely and taking heed of the advice given by others. Secondly: There is no point, what so ever, in writing this using OOP principles. Contrary to what Famer thinks, procedural (or OOP) is no indication what so ever on the skill level of the developer. It's the quality of the code itself, and whether or not the proper design principles for the given task has been followed. Using OOP only for the sake of OOP, is actually a sign of an inexperienced programmer. Then, let's get cracking with the critique: basename is not adequate protection for $_SERVER['PHP_SELF'], as evidenced by this snippet:php > echo basename ("test/index.php/test2/forall?test=jalla&true#bleh"); forall?test=jalla&true#blehNotice that the URL is valid, but the result is far from what you'd want. Not only does it strip out the path of the file, but it also incorrectly determines what the path is. To properly secure PHP_SELF, please see this thread. As Famer already mentioned, your input validation isn't much of an actual validation.Not only do you use output functions in it, which should only be used when sending content to the relevant third party system, but you're also allowing the users to post just about anything they like. Including content which would mess up the structure of your message file, and as such break your "database". You're also failing to validate that the REMOTE_ADDR is actually an IP-address. Seeing as this value is populated from the value that the client sends, it can be subject to injection attacks. What you should have done, is to actually validate the content that the user sends. To make sure that it conforms to a set of rules, that is (generally) universally true for the content you expect. Names, for instance, does not (normally) contain hash tags, colons or any other non-alphanum character (except certain punctuation characters). This is an abbreviated version of the function I use for validating names: function validate_name ($name, $maxLength) { if (!preg_match ('/^[a-zA-Z\\pL][\\w\\pL\' \\.\\-]{1,'.$maxLength.'}\\z/u', $name)) { return false } return true; }The message content is a bit special, as you are expecting your users to be able to use all (or just about all) printable characters. Still, that leaves some non-printable characters out in the cold, which you should test for. Looking at an ASCII table should tell you what those are. The next item on the list is how you do these tests, and the current way of doing it is sub-optimal. What you've done means that the user has to submit the form multiple times, to get all error messages about any invalid/missing content. Instead of wrapping all of the tests in an else-if block, do each test individually. Then, after all validation tests have been completed, check if any errors were raised. If they were, then you know that you should abort parsing the input and show the form anew. Preferably with the previous data filled out (do not forget output escaping), and all of the errors that caused the submission to be declined. In short, something like this (pseudo code): if (submit) { if (!val_name ()) { $error[] = 'name'; } if (!val_message ()) { $error[] = 'message'; } if (!val_email ()) { $error[] = 'email'; } if (!empty ($error)) { show_errors (); add_escape_output ($name, $email, $message); show_form (); return; } save_data (); redirect (); } Also, never ever manipulate the user-data outside of purely escaping. If the content violates some of the rules then you need to alert the user about it, and let him/her fix it. Not just silently drop something, as that is extremely annoying. Imagine spending one hour on a very thought out thread, where you're asking for help on a very elusive, but critical, problem. Only to discover that the forum-software decided to silently drop half of your post, forcing you to rewrite all of that from memory. If that half contained only code, it wouldn't be much of a problem, but if it contained 1000 lines of explanations on the problem and what you've tried... I think you see my point. As for the actual saving of the data, there you've missed out on one crucial piece. Namely, output escaping. Seeing as you've opted to go for a semicolon delimited values file, you need to take care to ensure that any semicolons that the user adds to his message are escaped. If not, they will wreak havoc on your internal structure, as your parser will treat them as content separators instead of actual content.For this I'd advice you to use the built in CSV functions in PHP, as they should handle the escaping automatically. Your error listing needs to be updated to handle multiple errors, otherwise it's fine. You need to use the CSV functions for reading the message content as well, otherwise it'll treat semicolons improperly even with escaping. You really shouldn't use tables for formatting, as this is not tabular data. Instead use fieldsets, labels and CSS, as is proper and semantic HTML. Lastly, you really should consider using a proper database for this. Files doesn't lend themselves to dealing with concurrent writes, and you have a hell of a race-condition build into your script here. A race conditions, which will lead to lost messages if you get two people that chats at the same time. (Yes, only two people are enough.) Also, contrary to what Famer wrote, I see that you have indeed separated business logic and presentation logic. Only within the same file. Preferably speaking, though, this should have been in two files. With the HTML part being in a file of its own, which is included at the very end of all of the business logic. However, seeing as this is a code review request, I'm going to assume that this is what you've done already. PS: No need to use the ### in the comments, they're actually more of a distraction than help. Especially if you use an editor which uses syntax highlighting, which you should do.

The questions is, how many hits does this page get? The CPU time without knowing how often the script is executed is rather meaningless. If the page has been accessed a 100 000 000 times, then 144 minutes is nothing. On the the other hand, if it has been accessed only 100 times, you have a serious problem. MySQL fetches the data from an internal cache, if it doesn't change, so that helps keep the CPU time low for it.

For this you'll have to use imagecopyresampled, which will allow you to copy and scale the image. Prior to this, however, you'll need to calculate the correct new dimensions of the scaled down version. Normally this is done by finding which of the dimensions has the greatest difference, in percent, compared to the maximum size for that particular side. This you figure out by taking the actual size and dividing it by the maximum size, for both dimensions. Then compare the results against each other, and select the biggest number out of the two. Next you'll need to divide the actual dimensions on this number, to get the target dimensions for the new image. At which point you're ready to use the above function, to get a nicely scaled picture, with the same aspect ratios as the original. PS: If you only need it to be less than 750 pixels wide, you can just check if the width is bigger. If it is, calculate the difference ratio for the width, using the same method as described above, and use it straight away. No need to calculate the ratio for the height.

You need to edit the wpsc_cart_item_price () function, I suspect. In any case, the code you've posted above is not quite the code you want to look at, as it is only responsible for showing the HTML page. Not calculating anything. Also moved this to the correct section, as this is more a Wordpress question than it's about mathematics in PHP.

nmap!

Not to mention it makes connecting to more than 1 database at the time impossible. Granted, not a problem for the vast majority of cases, but it is worth taking note of it anyway. This is true for all classes you make "static", after all. Since you can't create objects from it, to hold separate data in. While there can be many objects of the class, there is only one class.

I'd simplify the logic a bit, and generate the timestamp using DateInterval and DatePeriod. // First set up the starting day, ending day (+1 day) and the interval to use (1 day). $start = new DateTime ('next monday', new DateTimeZone ('UTC')); $end = new DateTime ('next monday +7 days', new DateTimeZone ('UTC')); $interval = new DateInterval ('P1D'); // Then create the period range, including the starting day and up to the ending day. // Doesn't include the last day, which is why we added one day extra when creating the ending timestamp. $range = new DatePeriod ($start, $interval, $end); // Run through the days of the next week, and build up the output string. $output = ''; foreach ($range as $date) { $output .= $date->format ('Y-m-j')."\n"; }Then it's just to echo out $output wherever you want the dates to be displayed.

Glad I could help, and thanks for sharing the solution.

Also, I would just like to point out that input validation doesn't necessarily help against SQL injections. For that you have to use the proper output escaping method, which is either *real_escape_string () or Prepared Statements. I recommend the latter, using the PDO or MySQLI libraries, as that handles the escaping for you. Secondly: htmlspecialchars and htmlentities should never be used prior to adding data to the database. They are HTML escaping functions, which means that you should only use them immediately before sending content to the browser. Also, htmlentities escapes far more than necessary for HTML, and as such htmlspecialchars is the one you should use. The correct order of processing data from a user is as follows: Validate data. Show validation errors, if necessary. Process the data (business logic). Escape and send to the correct third party system (browser or database, most likely).

You forgot the delimiters, which is what PHP is complaining about. The old ereg_* () family of functions didn't require the delimiters, which is the most common cause of this issue amongst people who're either used to working with them or looked at (too) old guides. The easiest way to fix the problem, is to add a pair of slashes to the start and end of the regular expression. Check the examples for preg_match for more information.

Another tip: You should not add HTML tags to the values returned by the class. Adding markup is solely the view's (template) responsibility. That way, you only need to replace the view to output your data to a different system, instead of rewriting the entire application. Embedding presentation logic into the business logic, as you've done above, means you've just about ensured that you'll have a major rewrite on your hands. Every time your want to change how things look.

Ah, wonderful, it worked. Thanks!

Am I correct in my suspicion that you're trying to build up a dynamic SQL query with the item IDs? If so, then you should really be looking at JOINs. If not, then just ignore me.

Got a rather odd issue: I'm missing most of the text on the forum. Except for the dynamic data (forum names, breadcrumbs, etc), and the header/footer text, I have no text on the forum. Everything comes up as blank, and the search fields show only "Placeholder". I haven't changed anything since the last time I logged in, but it seems like the forum can't find the language file. I suspect that either I'm using an old, and now deleted, file or that somehow the record in the DB has been altered. Tried to fix it myself, but could not find the edit profile link without the actual link text. Searching the DOM didn't provide any useful results either, unfortunately.

Do the length check in the RegExp as well, to avoid unnecessary code and (not to mention) nesting. Also, I would recommend that you reverse the logic of your test a bit. So that you're checking for error conditions instead. This will not only make it a lot easier to read your code, as your error messages will immediately follow the test which produces them; But it will also help reduce the amount of nesting required, and thus further improve the readability. Lastly: Always use die after a header redirect.

My point was, as I wrote, to show that one can use objects in a purely procedural code. That was simply the first and best example I had available.

No, no and no. CI is one of the worst frameworks to use, especially for learning purposes. What I would recommend you do to, John, is to find a small and light-weight framework. One which you feel comfortable with, and then work to rewrite your logic to it. Laravel or Symfony 2 are some of the most commonly recommended frameworks, and Yii is another candidate. You can find a list of others at Wikipedia.

What problem? Lots of information in your post, which is great, but you seem to have forgotten to describe what your problem is.

Considering the fact that all PHP does, is to output HTML to the browser: Yeah, adding that menu shouldn't be a problem. Might take a bit to orient yourself in the code, but the adding itself should be trivial. That is, if the system (PHP script) you're using is any good.

Object Oriented Programming (OOP) definitely has its place. Even if you're not going to use it in your projects, learning how to think OOP will improve your code and coding style. Guaranteed. That said, as (almost) everyone above have said: Both OOP and procedural code has its place, and knowing when to use what is the real trick. Try to force something that should be procedural into being OO, and you've just added a whole lot of unnecessary complexity for no gain at all. Vice versa, and you've got a (too) inflexible solution which puts severe limits on future expansion. Both procedural and OOP can easily be utilized to create an unholy mess, avoiding that is entirely up to the programmer. Generally speaking though, the more complex the system the easier it is to create said unholy mess. Also, one thing that most people struggle to understand, is that using classes and objects is not equatable to OOP. You can use objects in strictly procedural code, like in this example. As you can write object-oriented code without using objects (albeit, that tends to be more difficult). It is all about how you structure your code, how it's abstracted out. PS: I wouldn't put too much stock in how Hall Of Famer extols OOP, stating that it is the end-all and perfect solution. He's a "bit" too biased, for some reason.

Follow the link, and you'll find lots of examples. Search the web, as I suggested, and you'll find millions of examples. Generally you'll find that the effort people put into helping you, is directly proportional to the (perceived) effort you've put into solving/finding the answer on your own. This includes your grammar and spelling.

They are quite similar yes, but as the PHP manual states htmlspecialchars escapes a bit more than just FILTER_SANITIZE_SPECIAL_CHARS. For a 1 to 1 comparison, you should use FILTER_SANITIZE_FULL_SPECIAL_CHARS. However, htmlspecialchars is both shorter to write, and more common, so I recommend using it. That brings us to the next point, SQL injection prevention. As stated htmlspecialchars is for escaping output to a HTML-parser, not a database engine. The DB engine doesn't understand HTML, and doesn't care about it either. What it does understand, is SQL queries. SQL queries and HTML use quite different meta-characters, with only a few in common: Quotes being the most obvious, and even that is somewhat conditional for HTML. However, due to the other meta-characters (which HTML does not share) using HTML escaping methods for SQL queries will not protect you. Those meta-characters will go through htmlspecialchars unscathed, and thus be able to cause SQL injections. Same the other way around, if you use SQL escaping methods to escape output going to a browser. It will not escape the < and > signs, meaning an attacker can easily perform HTML injection attacks (XSS etc). Not only that, but you'll suddenly have a lot of slashes in places where there shouldn't be any. Which is quite annoying, at best. This is why it's so important to know, and use, the proper method for the third party system you're sending the data to. If you don't, you are still vulnerable. Not only that, but you just mangled your data to boot. So, I'll repeat: htmlspecialchars only immediately before adding content to the HTML output, and SQL escaping only immediately before adding stuff to SQL queries. Without overwriting the original values. Examples: // Test data. $string = "Tim <3 Jenny O'Toole"; // SQL protection: $query = "INSERT INTO `test` (`string`) VALUES ('%s')"; $query = sprintf ($query, $db->real_escape_string ($string)); // HTML protection: $outputMessage = "<p>".htmlspecialchars ($string)."</p>\n";

Jeez... Talk about being ungrateful. I'm not here to do the research for you, I gave you a starting point the rest is up to you. I even verified that the results contained the knowledge you asked for, all of this for free, to be kind. What do I get back for it? "It's not good enough, do it for me" basically. If you want me to do it for you, then I can. I, like Jessica, only charge about $100 per hour for jobs like this.

You need to read up a bit on MySQL's LIKE function. Here's the official manual entry on it, and while it might be a bit confusing at first I recommend playing around with the examples. If you need more examples, there's plenty out there if you search for "mysql like".