Jacques1

The code has so many problems that I would just throw it away and start over, this time with the technology of the 21st century. The mysql_* functions are dead. They've been obsolete for more than a decade and have already been removed from the current PHP version. We use PDO now. stripslashes() is even more dead. The last time it made sense was somewhere in the 90s, but even that it was an awful hack to work around design errors of PHP. Plaintext passwords? C'mon, this isn't 1980. Nowadays, we use password hash algorithms like bcrypt. Stop telling your users about your database errors. They cannot do anything with this information. So wherever you've learned PHP, you definitely need a better resource.

Don't. There are a few validators which make sense (like the e-mail pattern), but most of the filters are nonsense or even harmful, especially the “sanitizers”. All they do is damage your data. I've also suggested PDO, but more important than choosing an interface is using it correctly. Your example above doesn't do that and isn't very useful for anybody, especially not for beginners. I wouldn't use w3schools as a resource either. It's known to be bad and spread a lot of wrong information. The link shows that: They also print error messages on the screen, they use insecure emulated prepared statements, and their way of using prepared statements is needlessly cumbersome. Use tutorials from people who actually know what they're doing. See my PDO link in #5, for example.

That's not really better. You're also doing this strange mysqli_error() stuff, you're relying on obsolete SQL-escaping, and the htmlspecialchars() calls before the query will screw up all the data. HTML-escaping is strictly for HTML output. You must not use it in any other context.

Yes. This is actually a good example for a highly specialized class which you may very well have to replace at some point. The hard-coded queries won't even work for all SQL database systems, let alone any non-SQL storage. I don't, and I personally don't really care about project-specific conventions unless I'm actually working on the project. Suffixes can make sense if you don't have an IDE or just like to be super-explicit. Some people also like to prefix all names. I do have an IDE, so this is all just clutter to me.

There are Markdown parsers which can convert the markup to HTML. Markdown itself is very simple to compared to HTML (and you don't have to care about it anway as it's processed automatically).

Note that associative arrays have a couple of quirks which can lead to confusing behavior. Depending on the exact type of the elements (strings? floats? objects?), you may need a different implementation. The above works best for integers and strings.

So the goal is to get your server compromised, yes? In that case, ask somebody else.

The code makes no sense. It's generally a bad idea to imitate other languages. Instead, you should specifiy the goal and then implement it with the features of your current language. So what is the goal? A mathematical set which can contain a value exactly once? Then the right implementation is an associative array where the keys are the set elements and the values are arbitrary: <?php $set = []; // store 42 (the value is irrelevant) $set[42] = true; // store 42 again (this has no effect, just like you would expect from a set) $set[42] = true; // check if the set contains certain elements $contains_42 = isset($set[42]); $contains_43 = isset($set[43]); var_dump($contains_42, $contains_43); // delete element unset($set[42]); $contains_42 = isset($set[42]); var_dump($contains_42); Putting this into a class shouldn't be too hard now. I'm sure there are also existing libraries. <?php class Set { private $elements = []; public function add($element) { $this->elements[$element] = true; } public function remove($element) { unset($this->elements[$element]); } public function contains($element) { return isset($this->elements[$element]); } } $set = new Set(); $set->add(42); $set->add(42); var_dump($set->contains(42)); var_dump($set->contains(43)); $set->remove(42); var_dump($set->contains(42));

Storing and displaying dynamic HTML is inherently dangerous and should be avoided at all costs. If the WYSIWYG editor can export a simplified format like Markdown, that's definitely preferrable. Raw HTML only makes sense if the markup is so complex that it cannot be represented in any other way. The workflow is as follows: The WYSIWYG editor exports the document in a certain format (the simpler, the better). You store this document in the database using a prepared statement. To display the document, you convert it to HTML (if necessary), filter it with the already mentioned HTMLPurifier and then include it on your page. Additionally, you send a Content Security Policy header to reduce the risk of cross-site scripting attacks.

The code is fundamentally wrong. It seems you've taken a really old, really bad script with mysql_* function calls and just added an “i” everywhere. This doesn't work. You need to actually learn the mysqli interface (or rather: database programming in general). Get rid of this or die(mysqli_error()) stuff. Why on earth would you want to show your database errors to your users? What are they supposed to do with this message? It only helps attackers interested in gaining information about your system. The proper approach is to enable exceptions so that the PHP error handler can take care of the problem (assuming you've configured it correctly): // make mysqli throw an exception whenever it encounters a problem $mysqli_driver = new mysqli_driver(); $mysqli_driver->report_mode = MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT; $database_connection = mysqli_connect($host, $user, $password, $database); Stop putting PHP values straight into query strings. Never heard of SQL injections? The proper approach is to use prepared statements: // use a prepared statement with placeholders to safely pass data to MySQL $member_stmt = $database_connection->prepare(' INSERT INTO members (username, email, password) VALUES (?, ?, ?) '); $member_stmt->bind_param('sss', $_POST['username'], $_POST['useremail'], $_POST['password']); $member_stmt->execute(); Apart from that, I strongly recommend you use PDO instead of mysqli. It's a lot more comfortable, and since you have to rewrite your code anyway, you might as well choose the best interface available.

As I already said in #11, it's good practice to always start with an interface and program to that interface rather than any concrete class. The only time you ever reference a class is when you instantiate it to provide an implementation. Or when you use a PHP core class. Personally, I would never mention the term “interface” in the name or filename of an interface, because this is redundant information. It's like starting every function name with “function”. If you instantiate the class at a central location and then pass it to any object which needs a variable dumper, yes, this is a valid approach. There are more advanced concepts like DI containers, but I wouldn't do too much at the same time. If you understand why interfaces are useful, you're definitely ready to write proper OOP code.

Just dropping a class into your code may be fine for one-off jobs where you don't give a shit about quality. But if you want a proper OOP infrastructure for a long-term project, you want to program against interfaces rather than concrete classes. This has lots of benefits: It separates the API from the implementation. Most of the time, it's completely irrelvant how exactly you've written your code. The HTML elements or echo statements are just trivial details. What matters is the API, and that's what the interface conveys. It forces you to actually design your API instead of just writing ad-hoc code. It makes your code modular and extensible. I understand that you only need this one implementation right now, but things change. A lot of people thought that they'd only ever need the mysql_* functions to query their database, now they have to rewrite their entire code. It's the basis for testing. If you have hard-coded class references everywhere, there's almost no chance for testing an object in isolation. With an interface, you can just plug in a dummy object. This may seem very theoretical to you right now. But once you start using OOP code from other programmers (libraries, frameworks, ...), you'll quickly see that there's a big difference between good infrastructures and bad infrastructures. A good infrastructure can easily be adjusted and just works. A bad infrastructure gets in your way if you don't do exactly what the author expects you to do.

The code doesn't make much sense. First off, nobody would ever use the IP address as a customer identifier in reality, because IP addresses are commonly shared and reused among thousands of people. Haven't you been introduced to the concept of sessions? Secondly, your entire code is essentially a (bad) reinvention of basic SQL operations. Have you heard of joins? Aggregate functions? SELECT SUM(cart.qty * product.product_price) AS total_price FROM cart JOIN product ON cart.p_id = product.product_id -- you should be consistent when naming columns; either p_id or product_id, not both WHERE cart.ip_add = :ip_add -- this is a parameter for a prepared statement; do not inject PHP values directly into query strings

Every class and every interface goes into its own file. So there are three separate files in this case. There is no special treatment for the CLI class. It's just one of currently two implementations of the interface. You don't and cannot put it into a separate folder, because autoloaders expect a specific file system structure (folders typically correspond to namespaces).

The LIKE fragments are also badly broken. For some reason, you replace all percent characters and underscores in the text parameter with literal backslashes. That's not what I told you in your last thread. And the author parameter isn't escaped at all. Why do you even use LIKE for a numeric ID? Shouldn't that be an exact match? A lot of this doesn't make sense, and it seems you're randomly typing code on your keyboard with no real concept. That's not a good approach. Make sure you understand what you want and then implement it systematically. For example, start with a proper search form just for the joke text. Also write a function which correctly(!) escapes a string for a LIKE context. function escape_mysql_like($input, $escape_char = '\\') { return str_replace(['%', '_'], [$escape_char.'%', $escape_char.'_'], $input); }

You can fill the superglobals with whatever test data you need. If you don't like this, that is a valid reason for a wrapper. My point is that you shouldn't write wrappers just because some scanner said so.

And what are the specific arguments for that statement? The superglobals are a core feature of PHP, so it's normal to use them in an application. Wrapping them in custom classes only makes sense if you have a concrete reason (extra features, access control, ...). Otherwise you're just making the application more complex. I know that some automated scanners and IDEs complain about superglobals, but that doesn't mean you should blindly follow this advice.

Yes, but you also need to understand what to replace the percent character with. Right now, you're replacing it with an underscore which stands for “exactly one arbitrary character”. Obviously that doesn't help. If you want percent characters to be interpreted literally, you need to escape them with backslashes: $search = '%'.str_replace('%', '\\%', $_GET['text']).'%';

You document the interface methods. The methods of the implementing classes automatically inherit this documentation, unless you override it. The explicit public modifier should be used for clarity. Interfaces are an exception, because they can only specify public methods. <?php /** * A debug tool to print the content of variables (e. g. $_POST) */ interface VarDumper { /** * Prints a value * * @param string $title the title to be printed * @param mixed $data the value */ function dump($title, $data); }

The class name is fine. You just shouldn't claim in the description that the class is written specifically for the PHP superglobals. Yes, the @param tag belongs to method descriptions. An interface can be used as an abstraction layer. When a task may be implemented in different ways, you first define an interface which merely contains the method signatures (i. e. the names and parameters). The implementation is then left to the concrete classes below the interface, which provides a lot of flexibility. This is what my example does: The interface merely says that a VarDumper has a dump() method with two parameters. How exactly the method works is not specified yet. This is up to the classes which implement the interface (in my case HTMLVarDumper and CLIVarDumper). Using the code is simple: You choose the appropriate class for the current context and instantiate it. The application can then use this instance and be sure that it will always get the right formatting. If you always want HTML output: $varDumper = new HTMLVarDumper(); // ... $varDumper->dump('POST', $_POST); If you want HTML or CLI output depending on the server API: <?php if (substr(php_sapi_name(), 0, 3) == 'cli') { // CLI output $varDumper = new CLIVarDumper(); } else { // default: HTML output $varDumper = new HTMLVarDumper(); } // ... $varDumper->dump('POST', $_POST); // the concrete VarDumper implementation is irrelevant at this point

It doesn't really make sense to instantiate a class just for the trivial purpose of making a fancy variable inspection. This should be a function or, if you insist on OOP, a static method. The hard-coded HTML output violates the reusability principle. If I want to have debug information in CLI mode, I cannot use the class at all. Even a different formatting isn't possible. Method names are supposed to use camelCase according to PSR-1, not PascalCase. The class description is confusing. It claims that the class is meant specifically for the PHP superglobals, but the actual implementation has no reference to the superglobals at all. It's a general-purpose debug display which can for example be used for the superglobals. What's the @param doing in the class description? A better approach would be to first define a generic interface for debug information and then subclass it for different formattings. This also allows custom formattings: <?php interface VarDumper { function dump($title, $data); } class HTMLVarDumper implements VarDumper { function dump($title, $data) { // output for HTML contexts } } class CLIVarDumper implements VarDumper { function dump($title, $data) { // output for CLI contexts } } // the class user is free to define custom classes as well

Simply pass the entire $json_ary (you might want to choose a better name) to your template and then access whatever data you need. I also strongly recommend you learn the basics of PHP before jumping into this project. Otherwise you'll waste a lot of time on imaginary problems like this one and wrong approaches like in your other thread.

Who said anything about columns? I'm talking about rows. If, for example, you want to assign keywords to articles, you'd create a table article_keywords with an article_id column and a keyword column: article_id | keyword ------------+------------- 1 | programming 1 | php 1 | sql 2 | javascript 2 | html (You can also store the keywords in an extra column so that they can be reused) Now it's trivial to look up the articles belonging to one or many keywords. Your “needle” table seems a bit odd, though. I would expect the keywords to come directly from the user input.

No, there is no sane way to work with the data structure you have. You need to learn the basics of normalization and then fix your tables accordingly. For example, values in relational databases are supposed to be atomic, that is, comma-separated lists are almost always wrong. What you do instead is store each keyword individually. Then you can easily search them without any explode() hacks.

You cannot put the reload() call after the post() call, because the post() method returns immediately without waiting for the response. Depending on timing conditions, you may very well end up reloading the page before you've even received the data. That's why there's a success callback: Anything that should happen after the request has been successful must go into that function.