Jacques1

It doesn't matter which particular variation of that friend-selection code throws the exception, because it's always the same two errors: You try to emulate basic SQL feature like joins in PHP, which leads to lots of unnecessary queries and overly complex string constructions. If you had one simple query, I'm sure you'd be done already. Your code is wide open to SQL injections and syntax conflicts, because you've hastily given up prepared statements instead of actually fixing the problem. Read the previous replies and consider brushing up on your SQL skills. This will help you much more than chasing syntax errors.

In case you have trouble selecting the friends “in both directions”, this can be done with a UNION. For example, the friends of “joe” can be selected with SELECT friend_two AS friend_name FROM friends WHERE friend_one = 'joe' AND accepted UNION SELECT friend_one FROM friends WHERE friend_two = 'joe' AND accepted ; Or even shorter: SELECT DISTINCT IF(friend_one = 'joe', friend_two, friend_one) AS friend_name FROM friends WHERE (friend_one = 'joe' OR friend_two = 'joe') AND accepted ; Now all you need to do is join this result set with the user table as suggested by mac_gyver.

“Doesn't work” doesn't really tell us anything. What exactly is the problem? Taking a wild guess: You've forgotten the “u” (UTF- modifier in your regex. It should be “~...~u”.

So what exactly is the problem? If you expect somebody to wade through your entire codebase and implement the feature for you, I don't think that will happen. By the way, you should consider hashing the activation codes instead of storing them as plaintext. While the codes are not as critical as, say, the passwords, it would still be annoying if some joker finds them out and activates all accounts at once. A very simple hash algorithm like SHA-256 is sufficient for the codes, since they're purely random. So upon registration, you generate a code, send the plaintext code to the user and store the SHA-256 hash of it in the database. Upon verification, you hash the user-provided token and check if this hash is present in your database.

There's nothing wrong with closures themselves, but yours are way too complex and have unexpected side effects. The fact that you had to open a thread just for the implementation kind of proves that. Closures should be used as auxiliary functions performing a single simple task. For example, they're perfect for functions like usort() or preg_replace_callback(). But when you start to move your application logic and database queries into a bunch of callbacks, that's too much. When I saw your code for the first time, it immediately felt weird, and it took me a while to figure out what this even does. The heavy lifting should be done by functions and methods, not callbacks.

You might have misunderstood my suggestion regarding the UNIQUE constraint: I'm not talking about two individual constraints for the two columns (that of course wouldn't work), I'm talking about a single constraint which covers both columns. So this constraint makes sure that the combination of regno and year is unique. That's the same check you currently do in your code, but without the problems of your approach.

I'll have to agree with benanamen that this code is poorly written and shouldn't be used at all. If you intend to use this for a real application, it's way too insecure and will put your users' data in jeopardy. And if this is just for learning, you're better off writing your own code with modern features like the password hash API. It's generally a bad idea to just copy-and-paste code you found somewhere on the Internet. Most of it was written by amateurs many years ago and hasn't been updated since. A better approach is to do your own research and look for projects which are actively maintained on a platform like GitHub (preferrably by more than one person). Do you need this for an actual application? Do you have the time to learn the basics and write this yourself, or do you want a premade solution?

imkesaurus, it might make sense to forget about the user interface for now and just look at the logic: You want to be able to obtain one of three values: the transfer costs, the bond costs or the ancillary costs. All of those depend only on the purchase price, and the last one is derived from the other two. So why not create one function for each value? The function takes a purchase price (and a PDO object), selects the required data from the database, calculates the cost and then returns the result. For example: function calculate_transfer_costs($purchase_price, PDO $database_connection) { $transfer_costs_stmt = $database_connection->prepare(' SELECT transfer_duties, transfer_fee, deeds_office_fee, vat, total_transfer_costs FROM transfer_cost ORDER BY ABS(purchase_price - :purchase_price) ASC, purchase_price ASC LIMIT 1 '); $transfer_costs_stmt->execute(['purchase_price' => $purchase_price]); $transfer_costs = $transfer_costs_stmt->fetch(); // TODO: use constants instead of hard-coded numbers $vat = $transfer_costs['vat'] + 238.6; $total = $transfer_costs['total_transfer_costs'] + 1697.4; return [ 'transfer_duties' => $transfer_costs['transfer_duties'], 'transfer_fee' => $transfer_costs['transfer_fee'], 'deeds' => $transfer_costs['deeds_office_fee'], 'vat' => $transfer_costs['vat'], 'total_transfer_costs' => $transfer_costs['total_transfer_costs'], 'total' => $total, 'total_costs' => $vat + $total, ]; } Now you can call those functions and calculate the costs at any time in any order, no matter which button your user clicked.

Actually, this whole approach of checking for duplicates is unnecessarily fragile and cumbersome. If you want the pairs (regno, year) to be unique, simply add a UNIQUE constraint to your table: ALTER TABLE validatedparticipants ADD CONSTRAINT UNIQUE (regno, year); It might even be appropriate to use this as the primary key. Now the database system will actually enforce unique combination. Your application no longer has to worry about duplicates. It can simply try to insert the row, and if that fails due to a constraint violation, MySQL will trigger an error which can be handled the application. I'm not familiar with the old mysql_* functions, but if you follow scootstah's advice and switch to PDO, the code will look like this: Setting up the database connection: <?php const DB_HOST = 'localhost'; const DB_USER = '...'; const DB_PASSWORD = '...'; const DB_NAME = '...'; const DB_CHARSET = 'UTF8'; // MySQL error codes const MYSQL_ER_DUP_UNIQUE = '23000'; // violation of UNIQUE constraint $dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET; // set up the database connection $databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [ PDO::ATTR_EMULATE_PREPARES => false, // activate prepared statements PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, // activate exceptions PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, // fetch associative arrays by default (this isn't critical) ]); Then you try to insert the row and handle possible errors: <?php require_once __DIR__.'/database.php'; $participantRegistrationStmt = $databaseConnection->prepare(' INSERT INTO validatedparticipants SET camp = :camp, hostel = :hostel, ... '); // Try to insert the row; if the participant is already registered, this will trigger an exception try { $participantRegistrationStmt->execute([ 'camp' => $_POST['camp'], 'hostel' => $_POST['hostel'], // ... ]); } catch (PDOException $participantRegistrationError) { // If the exception was caused by a constraint violation, tell the user about the duplicate name; otherwise propagate the exception. if ($participantRegistrationError->errorCode() === MYSQL_ER_DUP_UNIQUE) { echo "You're already registered."; } else { throw $participantRegistrationError; } } This will work reliably under all circumstances, and you only need a single query. Sure, you could try to emulate this behaviour in the application by first checking if the row already exists, but this has a lot of problems: There's a certain gap between your two queries. If a duplicate request arrives within this time frame, your application will accept it, because it doesn't “see” the previous row yet. In the end, you'll have the same row twice despite your check. Applications have bugs (as you already saw), so your check may simply fail to work and leave you with tons of garbage data. If the data is edited manually (be it by you, your coworker, you client, whatever), there are no checks whatsoever. So there's a fairly big risk that duplicate data will be introduced through human error. Two queries instead of one are unnecesary bloat.

Judging from your code example, you want to dynamically add relatively simple actions to the standard update/delete procedure (hence your idea to use a callback). It would be a bit of an overkill to extend the entire class multiple times only to add trivial stuff like a database query. You also won't be able to combine the actions (like updating a database and sending out an e-mail). With the observer pattern, you can keep each extra functionality in a very simple class of its own and simply plug it into the main upload procedure. But you know your application best, so if you find subclasses more appropriate, that may very well be the case.

Observers are, roughly speaking, “callback objects” which are notified by the basic class when a particular action happens (e. g. an image upload) and may then add their own functionalities. In your case, the interface for an upload observer would look something like this: interface ImageUploadObserver { public function onUpdate(array $imageData); public function onDelete(array $imageData); } Now you can implement a specific observer which saves the image data in a database: class ImageArchiver implements ImageUploadObserver { public function onUpdate(array $imageData) { echo "Updating image $imageData[image_id] in database.<br>"; } public function onDelete(array $imageData) { echo "Deleting image $imageData[image_id] from database.<br>"; } } The image uploader class maintains a list of observer objects which are notified when an image is updated or deleted: class ImageUploader { protected $observers = []; public function registerObserver(ImageUploadObserver $observer) { $this->observers[] = $observer; } public function update(array $imageData) { echo "Updating image $imageData[image_id].<br>"; // notify observers foreach ($this->observers as $observer) { $observer->onUpdate($imageData); } } public function delete(array $imageData) { echo "Deleting image $imageData[image_id].<br>"; // notify observers foreach ($this->observers as $observer) { $observer->onDelete($imageData); } } } And a complete example: $imageUploader = new ImageUploader(); $imageArchiver = new ImageArchiver(); $imageUploader->registerObserver($imageArchiver); $imageUploader->update(['image_id' => 12]); $imageUploader->delete(['image_id' => 42]);

Consider using the observer pattern instead of callbacks to add arbitrary actions on top of the standard update() and delete() action.

But you already have figured out the code for filling $children (as you demonstrated in reply #17), and you even showed us a complete example list. So what's the problem? My test data was really just test data, it doesn't matter if the IDs are sequential or not. It was just easier for me to type.

What exactly is the problem with the code you already have? If you're running at least PHP 5.4, this should behave just like you want it to. In earlier versions you have to explicitly import the reference from $this into the closure's scope: <?php class A { public $foo = 123; public function getClosure() { $definerInstance = $this; return function() use ($definerInstance) { var_dump($definerInstance->foo); }; } } $a = new A(); $closure = $a->getClosure(); $closure(); Note the use keyword. But what are you even trying to do? This approach looks a bit odd.

You echo $name twice, once in a strong element and then again in a li element. The element structure also isn't valid HTML yet. The display_category_level() function already yields a ul element, so you mustn't put this into another ul element. Simple create a single li element on each iteration: <?php // test data $children = [ 1 => [ 2 => 'Main 1', 3 => 'Main 2', ], 2 => [ 4 => 'Sub 1', ], 3 => [ 5 => 'Sub 2' ], 4 => [ 6 => 'Subsub 1', 7 => 'Subsub 2', ] ]; display_category_level($children); function display_category_level($children, $parent = 1) { if (isset($children[$parent])) { echo '<ul>'; foreach ($children[$parent] as $id => $name) { echo '<li>'; echo '<h2>' . htmlspecialchars($name, ENT_QUOTES | ENT_SUBSTITUTE | ENT_HTML5, 'UTF-8') . '</h2>'; display_category_level($children, $id); echo '</li>'; } echo '</ul>'; } }

Create one function for each cost calculation. Then your calculate_ancillary_costs() can simply call calculate_transfer_costs() and calculate_bond_costs() and add up the values. By the way, you don't need a while loop when there's only one row like in this case. Just call fetch_assoc() once.

There's a dot instead of a comma after CloseTue in your query.

In PHP, that would be a foreach loop. See Barand's code in reply #10: foreach ($children[$parent] as $category_id => $category_name) { ... }

I'd add two columns to the table: sent (a boolean) and sending_failures (an unsigned integer). Each time the script executes, you pick a fixed number of records which haven't been sent yet and had less than, say, 10 failed attempts. Order them ascending by the time they were entered into the table so that you get a queue. If the mail was sent successfully, you mark it as sent, otherwise you increment the number of failures.

The code is working fine on my machine, so this looks more like a permission/configuration issue.

The problem could be pretty much anything, maybe an application bug, maybe an issue with the PHP settings, maybe incorrect permissions on the server. So you'll have to narrow it down yourself. Check the error log (if it's disabled, turn it on now and try again). What do you see? You also need to learn about using functions. Copy-pasting of large bocks of code is not how programming works and will make debugging much harder.

To fix the error handling issues now and in the future, you should enable exceptions: <?php $mysqliDriver = new mysqli_driver(); $mysqliDriver->report_mode = MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT; $databaseConnection = new mysqli(...); Whenever a query fails, PHP will automatically throw an exception with all relevant information. So you don't have to manually check every function call and extract the error message from some attribute.

Really all you have to do is put the mail sending logic into the loop rather than after it. Right now, you just keep overwriting the same set of variables in your loop, and then you send a single mail with the data that happens to come last. However, you still need to fix some things: You should send the e-mails in a controlled manner with an upper limit per script execution (e. g. at most 100 mails). Clearing your entire database table at once is not a good idea, because it may overload your server or even be abused for DoS attacks. Never insert raw strings into an HTML context. This applies to both websites and e-mails. While most e-mail clients won't execute any injected scripts, this is still very unprofessional and may be flagged as malicious content. Use HTML-escaping with htmlspecialchars().

I suggest you try it for yourself. If you cannot possibly get it to work, show your attempts and explain the problem. This is really just a small adjustment of the code you already have.

It's not valid to have an ul element as the child of another ul element. That's why I proposed putting the child-ul into the li element in my pseudo-code: <ul> <li> <h2>List heading</h2> <!-- sub list --> <ul> <li> ... </li> <li> ... </li> ... </ul> </li> <li> ... </li> </ul>