Jacques1

The first rule of PHP debugging: When you get The White Screen Of Death, that means you need to check your error reporting. But your code really doesn't make any sense at all: You pretend to use a prepared statement, but then you somehow change your mind and just drop all values straight into the query string. The whole purpose of a prepared statement is to not use this extremely insecure practice and instead pass the values through parameters. You use PDO(?) to fetch all rows of a query and return them. But then you somehow change your mind and decide to use an entirely different database interface to fetch the rows. It's too late for that, the function is already terminated. And you can't just switch from PDO to the odbc_* functions in the middle of it, anyway. And on top of that, you don't even fetch the result set in your odbc_* part (which doesn't get executed). You just keep overwriting the same variable. It seems to don't really understand what all those methods/functions do and which API they belong to. You seem to have copied and pasted code from two entirely different tutorials and then somehow tried to merge everything it into one application. This does not work. The first thing you need to do is make a definite decision about the database interface. You cannot mix and match. It's either PDO or the odbc_* functions, not both. Then you need to learn the interface you've chosen. If you choose PDO, I recommend this tutorial.

None of this makes any sense. When you're using $this, then you're in the instance, which means visibility is irrelevant. Visibility defines the external access. So whatever code you're having trouble with, it's not the one you posted above. It might be a good idea to actually investigate the bug and fix it rather than abusing public visibility as a cheap workaround. But I guess you're happy with “It works” (whatever that means).

OK.

What you're saying simply doesn't fit together. In the original post, you confused static and non-static attributes. This is a simple problem with a simple solution. You should actually have gotten an error message. Now you describe an entirely different situation: You say that you understand the difference between static and non-static and still got a bug. So we're talking about different code and a different problem? Then post it. We can't help you based on code which you don't even use.

Polymorphism does not magically make all switch statements superfluous. If you want to instantiate different classes depending on a parameter, well, then you need to refer to those different classes. The only way to avoid a switch statement would be to make the class name dynamic: $some_class = 'A'; $a = new $some_class(); But that's not exactly pretty, either. If you take this route, you should definitely check if the parameter indeed refers to a subclass of the product class. Otherwise, you may end up with any class.

self and $this are two entirely different things. self refers to the class, whereas $this refers to the current object. The objects do not have any property named “subtotal”, so there is no $this->subtotal. However, the CoreCartFunctions class has a subtotal property, so you can use self::$subtotal.

I realize that, and the bottom line is: This is nonsensical. I don't know why exactly you need to save database columns, but this results in a much higher complexity, much more work and much more problems. That's hardly a good decision. The user ID is not a secret. Those are just sequential numbers, right? Then it's not exactly hard to guess the ID of other users, is it? Even worse: This “token” is valid for the entire lifetime of the user account. Once an attacker has gotten hold of it, they can use it as often as they want. And hashing the user ID does not help you at all. If an attacker knows the user ID, they can simply calculate the MD5 hash from it. Nothing prevents them from doing that. And if they know the hash, well, then they already got the token. So the hash does absolutely nothing. You might as well append the letter “x” to all IDs. Long story short: This “token” does not work. It did not suggest encryption. You're putting words in my mouth. I specifically adviced against encryption and explained exactly why it does not work for your case. It's as useless as that MD5 hash. Sorry, but I think this discussion is pointless. You don't even seem to read the replies, let alone take the advice. We posted two valid solutions, one of them involving database columns, one of them not. If you stick to your approach no matter how broken it is, go ahead. But you don't need us then.

Do not store the plaintext token in the database. It's equivalent to a password, so you may only store its hash. But apart from that, yes, this is the standard way as used by hundreds of thousands of sites around the world. And only to clarify: The token must be unpredictable. Anything other than a bunch of random bytes does not work.

As I've already said, this makes no sense. You can't just throw a bunch of cryptography at the problem and hope that it will somehow magically make the site secure. You need to use the right tools in the right way. Hashing the username and the user ID is utterly useless. What is this supposed to do? And again: Encryption does not help you at all in your case. The purpose of encryption is confidentiality. It generally does not prevent people from altering the data. So what are you trying to hide from whom? And how does this solve the problem?

Hi, a hash of the username and the user ID? How is this secret data? And what is the hashing supposed to do? You need a secret token. That means you need to read, say, 16 bytes from the random number generator of your operating system: $raw_token = openssl_random_pseudo_bytes(16); $encoded_token = base64_encode($raw_token); If the OpenSSL extension isn't available on your system, you can also use mcrypt_create_iv() or read directly from the system device (typically /dev/urandom). Encryption doesn't help, no. This is not about keeping the timestamp secret. You need to prevent people from altering it, so this is a problem of integrity and authenticity. The theoretical solution is a message authentication code (MAC). But, frankly, don't do it. Getting MACs right is very difficult, and it will make your system much more complex and fragile. For what? Only to save two database columns? The proper solution is to send the encoded token to the user, store the timestamp and a hash(!) of the raw token in the database and then check them on request.

You use a wild mixture of the old mysql_* functions and the new MySQLi extension. This is not possible. They're two entirely different extensions, and the functions are not interchangeable. Why do you even do this? Did you try to update this old code and stopped in the middle of it? Then the first step you should do is finish the transition to MySQLi.

@ QuickOldCar: The code doesn't check password hashes at all. It's the registration procedure, and this simply compares the password field with the “Please retype your password” field. @ TomasV: First of all: Congratulations for being one of the very, very few members who actually uses up-to-date code with PDO and bcrypt. Normally, we have to start every reply with a long explanation of why the mysql_* functions shouldn't be used, why plaintext passwords or MD5 hashes are not appropriate etc. Looks like this is one of the rare occasions where we can skip that part. There are several problems, though. Are you sure you want to query a table called “movies” to check if the e-mail address is in use? Why movies? Also note that you do a COUNT(*) query and then check if the result set is empty. A COUNT(*) query always yields a row, so this check doesn't work and might be the bug you're looking for. But this isn't really the right approach, anyway. You cannot enforce unique database entries using PHP. Just because there were no rows when you checked doesn't mean this is still true when you actually insert a new row. What if two people try to register with the same name at almost the same time? Then they're both allowed to have it: user A | user B -------------+------------- | check name: | not used yet | | | check name: | not used yet | insert name | | | insert name The solution is to let the database system take care of uniqueness checks: Add a UNIQUE constraint to all columns that should have unique values. In your application, simply try to insert the row, and if that fails due to a constraint violation, you know the name is already taken. You shouldn't establish a new database connection for every single query. This makes no sense and is extremely inefficient. Simply connect to the database once on top of the script and then use this connection for all queries. It's also important that you configure PDO correctly. Currently, you have no error reporting at all. You'd have to manually check for errors, but you don't do that either. The default configuration is actually downright dangerous, because PDO will use client-side escaping instead of actual prepared statement. So start by creating a separate script for the database connection (and one for configuration values): config.php <?php define('DATABASE_HOST', 'localhost'); define('DATABASE_USERNAME', 'myapp_default_user'); define('DATABASE_PASSWORD', '823da81d4ce2b7c4a0c11bbc9343aa73'); define('DATABASE_CHARSET', 'utf8mb4'); define('DATABASE_NAME', 'myapp'); database.php <?php function connect_to_database() { static $connection; // Make sure to only connect once, even if the function is called multiple times if (!$connection) { $db_options = array( // use actual prepared statements instead of client-side escaping PDO::ATTR_EMULATE_PREPARES => false, // throw an exception when a query fails PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, // optional: fetch associative arrays by default PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, ); $connection = new PDO('mysql:host='.DATABASE_HOST.';dbname='.DATABASE_NAME.';charset='.DATABASE_CHARSET, DATABASE_USERNAME, DATABASE_PASSWORD, $db_options); } return $connection; } Include those files and establish a database connection on top of your script: <?php require_once __DIR__.'/config.php'; require_once __DIR__.'/database.php'; $database = connect_to_database(); Then add the UNIQUE constraints and check for violations as explained above: // MySQL error codes define('MYSQL_ER_DUP_ENTRY', 1062); $regstration_stmt = $database->prepare(' INSERT INTO users (uname, pword) VALUES (:username, :password) '); /* * Try to insert the user. If that fails, check if it's due to a violation of the * unique constraint. In that case, we know that the username is already taken. */ try { $regstration_stmt->execute(array( 'username' => $username, 'password' => $password, )); } catch (PDOException $registration_error) { // check the error code to see what went wrong if ($registration_error->errorInfo[1] == MYSQL_ER_DUP_ENTRY) { echo 'Sorry, this name is already taken.'; } else { // nothing to do for us, pass on the exception throw $registration_error; } } There are some other things, but I guess you do it step by step.

You should take the form offline. It took me 2 minutes to find your website, and you've just invited all script kiddies to give it a try. Start by learning to use the PDO interface. This is the “new” database interface for PHP. It also supports parameterized queries as a solution to the dreaded SQL injection problem. It's also crucial that you understand the basics of web security, in particular how to prevent cross-site scripting and how to store passwords. Escape everything, including variables like $_SERVER['PHP_SELF']. This will already be a huge step forward.

I wouldn't waste my time trying to debug this. The code is at least 10 years behind and full of much worse issues. You're riding a dead horse. I don't even know where to start: The mysql_* functions are obsolete since more than a decade and will be removed in one of the next PHP versions. The ereg* functions are even older. They were replaced with the preg_* functions somewhere around the year 2000, I think. That's a damn long time. MD5? I guess it was acceptable back in the 90s, but current hardware can break this is a matter of minutes. You have SQL injection vulnerabilities via the password parameter. You have cross-site scripting vulnerabilities via $_SERVER['PHP_SELF'] The e-mail check is ... weird. Should “0@0” really be accepted? The check if the e-mail address is already registered doesn't work for simultaneous requests. I understand that this might be legacy code. Or maybe you've just used some really, really bad tutorials or books. So I'm not blaming you. But this definitely needs an update. PHP today is very different from the PHP of the 90s.

We could speculate all day long about what might be the problem. But it makes much more sense to actually investigate it. So if the arguments are not valid, what exactly are they? var_dump() will help you find out.

Make a separate table for the servers and have the server column point to that table. Otherwise, this column is unrestricted and may contain any number.

No, you associate a serial number to a server with a row. You make a table for your servers. And then you make a table to associate serial numbers with servers. So there's a column named “serial_number” and a column named “server” pointing to the server table. If the serial number “FOOBAR” should be associated with server “30”, you insert a row into the association table: ('FOOBAR', 30)

No, this has nothing to do with visibility. Those are all public attributes, and you don't even try to access them outside of the class. The problem is the static modifier. You've found that already, but you somehow didn't draw the right conclusions. A static method belongs to the class itself. A non-static attribute, on the other hand, is associated with a particular instance of the class. That code is trying to mix the two levels: It calls a static method, but then it tries to access a non-static attribute within the method. This is invalid and simply makes no sense. The class simply doesn't have a $_value attribute (but its instances have).

So what is the mistake in your opinion?

First and foremost, this is a matter of understandability. The names are supposed to describe the content of the field. A bunch of cryptic numbers doesn't describe anything. What is “30”? The price of 30 apples? This may not seem important to you as long as you're working alone. You surely know what those magic numbers mean. But as soon as other people need to work on this, or if you resume the project after a longer pause, bad naming becomes a serious problem. The table is simply impossible to understand without detailed background knowledge. And, yes, there's also a technical reason: You can't have numbers as identifiers. The only workaround is to wrap the numbers in backticks, but that's really awful. Just use proper names. No, you should make a row for each. That's how data is stored in the relational model: as a row in a table. How exactly the table should be designed depends on your data. I have no idea how that looks like. Then don't use the LIKE operator. The sole purpose of LIKE is to search for partial matches (hence the name). I assume you've simply merged two pieces of code you've found somewhere on the Internet? Well, it's better to actually learn PHP and write your own code. Get rid of the old mysql_* functions. Personally, I'd also get rid of MySQLi and use the PDO interface instead. It's much more convenient. See this PDO tutorial. It explains everything you need to know to properly query a database.

The “<?=” syntax isn't considered a short tag since PHP 5.4 and is always available. If you do have short tags (actual ones), you're better off killing them altogether.

Calling parse_url() function will not return the expected result. The problem is that most users and many programmers don't understand how an URL actually looks like. Contrary to popular belief, “www.google.com” does not refer to the website of Google. It's a relative URL consisting only of a relative path. For example, if you're currently on https://yoursite.com/some/path/, then “www.google.com” refers to https://your-site.com/some/path/www.google.com/ And that's why parse_url() will interpret the string as a path. If you want to interpret the URLs in a different way to match popular misconceptions, you'll have to take a different approach. First of all: What do you do with URLs which use a scheme other than HTTP or HTTPS like mailto:foo@example.com or ftp://some-site.com/some/file? Leave them as they are? Remove them? Rewrite them? What about URLs which are clearly supposed to be a relative path like /images/kitten.jpg? Leave them as they are? Resolve them relative to the current site? Rewrite them?

Um, what? Did you literally name your columns “30”, “31” and “32”? Those are far from valid idenfifiers. How about choosing sensible names?

I have no idea how that is a “fix”. Who says that the primary key of the image is the filename? Who says that all images have a “.png” extension? In fact, the table entries you've shown so far all ended with “.JPG”. If you like to deal with the problem again in a few days when people complain about broken images, go ahead. But personally, I'd do it now.

Doing the error configuration at runtime doesn't help if the error happens before the script runs. This is not the case here, but you're likely to run into more errors in the future. So the proper solution is to open your php.ini and fix your global configuration: display_errors must be On, error_reporting should be -1. Of course you only do this in your local test environment. On a live server, you log the errors, not display them. Note, however, that PHP does not automatically display MySQL errors. If the query itself fails, then you will not see that by default. How to get the error message depends on the database extension you're using. As trq already said, your question is very vague.