Jacques1

Not really. There's a fundamental problem with the way you design your database tables. Instead of storing records as rows, you've somehow decided to copy and paste the corresponding set of columns over and over again. Now you have a gigantic list of numbered columns, most of which problably don't even contain any data. That is not how relational databases work. When you want to add, say, 100 skills to a users, you do not duplicate the skill-related columns in the user table 100 times. That would be insane. You create a separate table for the skills, add 100 rows and link each row to the particular user in the table of users. A clear indicator for design problems is when you start numbering your columns. That doesn't happen in a proper table. So one user has multiple experiences, educational accomplishments and skills? Then you need four tables: users (user_id, name, email, ...) experiences (experience_id, user_id, title, company, ...) educational_accomplishments (educational_accomplishment_id, user_id, institution, degree, field, ...) skills (skill_id, user_id, ...) The experiences, skills etc. are linked to the users via the user_id column.

The getAll() method should be public function getAll() { return $this->databaseConnection->query('SELECT name FROM users')->fetchAll(); } A query doesn't need to be executed, and PDO::FETCH_ASSOC is your default fetch mode.

A PDOStatement implements the Traversable interface. It does, at least partially. The mysqli_result class implements Traversable as well. You get a mysqli_result either through the mysqli::query() method or by calling mysqli_stmt::get_result() in the case of a prepared statement (this requires the native MySQL driver, though, so it's not available on all systems). mysqli is a more low-level interface, so it tends to be harder to use.

You certainly don't want to open a new database connection for every instance of your classes. In fact, you're currently opening a new connection through the inherited constructor and then yet another connection whenever the getAll() method is called. That means a single object will flood the database server with lots of useless connection requests when it really just needs one connection. Instead, create exactly one PDO instance outside of the objects and pass it to the constructor: <?php class Model { protected $databaseConnection; public function __construct(PDO $databaseConnection) { $this->databaseConnection = $databaseConnection; } } <?php class User extends Model { public function test() { var_dump($this->databaseConnection); } } <?php $databaseConnection = new PDO(...); $user = new User($databaseConnection); $user->test(); And again: Don't use prepared statements for purely static queries. That's what query() is for. You must disable emulated prepared statements when connecting to PDO, otherwise you're not safe from SQL injection attacks.

You can actually omit the fetch() calls altogether. PDO is rather “clever”: If you simply iterate over the PDOStatement object with a foreach loop, you automatically get the rows as associative arrays. Also note that prepared statements are useless when there's no external input. Simply use a static query: <?php const DB_HOST = '...'; const DB_USER = '...'; const DB_PASSWORD = '...'; const DB_NAME = '...'; const DB_CHARSET = 'UTF8'; $dsn = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET; $databaseConnection = new PDO($dsn, DB_USER, DB_PASSWORD, [ PDO::ATTR_EMULATE_PREPARES => false, // important: Disable emulated prepared statements PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, ]); $userStmt = $databaseConnection->query(' SELECT name FROM users '); foreach ($userStmt as $user) { echo 'The name is '.htmlspecialchars($user['name'], ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8').'<br>'; }

It's perfectly fine to use different HTML structures for different themes. A theme engine which can only swap CSS/JavaScript files is actually very limited and will require hacks as soon as the layouts become more complex. But then you still don't copy-and-paste hand-written HTML files. A theme is a graphical representation of a particular content. When you have to create 100 themes, it would be insane to duplicate the entire content 100 times. What you do instead is create the content once and then insert it into theme-specific templates. Anyway, I'm starting to think this is hopeless ...

The code makes no sense to me. You want the lowest, highest and average tempatures of what? Of each sensor individually? Of all sensors together? The problem is that your database layout is rather poor. Instead of storing the measurements as rows, you've added a separate column for each sensor. This makes it extremely difficult to do any calculations across the sensors, and adding a new sensor requires you to alter the entire table and append yet another column. It also seems you've stored dates and times as strings with custom formats instead of using the proper MySQL data types. This will again make calculations very difficult. So I suggest you fix your table first. The columns should be as follows: measurement_id (INT, AUTO_INCREMENT, PRIMARY KEY) sensor (VARCHAR) measurement_time (DATETIME) temperature (DECIMAL or FLOAT) A single row would look like this: (1, "Walk-in 1 Front", "2016-07-03 00:01:01", 5.00)

Right now, your project is really too trivial for proper feedback. You have models, controllers and views. Yeah, that's the basic structure of MVC – but nothing more. All I can tell you is that you have several security vulnerabilities other issues in your code: The views and models are loaded from arbitrary paths without any kind of validation. This will quickly lead to local file inclusion vulnerabilities. Dynamic file paths must be validated and ideally whitelisted (compared with a fixed set of acceptable paths). Do not load random scripts into your application. (Ab)using PHP as a template engine is generally questionable. PHP templates are inherently insecure (because they may do absolutely anything), they often lead to spaghetti code, and they are incredibly ugly and limited. PHP is a template engine of the 90s. You should be using a modern library like Twig or Smarty. In your database class, you're using the default emulated prepared statements, which is a security problem and can lead to SQL injection vulnerabilities. It's also impossible to set the character encoding of the database connection (unless one accesses the underlying PDO instance). Disable emulation by default and add an encoding parameter. You shouldn't extend internal PHP classes like PDO. You may accidentally override a feature and screw up the inner workings. It also makes the framework very inflexible. What if I don't have PDO, only mysqli? You catch exceptions, print the error message on your site and then continue the script. That's a very bad idea. Not only will you leak internal information and irritate legitimate users with cryptic messages. You also put your program into an erroneous state. If the database connection just crashed, how is the application supposed to continue? Leave exceptions alone so that PHP can send the message to the correct(!) destination and abort the script in a controlled manner. Don't catch exceptions unless you know exactly why you're doing it. Your classes are currently hard-coded everywhere, making it impossible to really extend or test anything. Once you understand the basics of MVC, you should look into dependency injection. No. See above. No.

Literally duplicating every single HTML file for every single theme is the worst approach you can possibly take. Whenever the content changes, you'll be busy wading through all of your themes and adjusting them. Writing a new theme is also a pain in the ass, because the files are cluttered with text and images and other content that has nothing to do with a theme. Have you considered solving your problem with PHP? I'm talking about a dynamic website where the content is stored in a database and then inserted into HTML templates. An even better approach is to combine PHP with a modern template engine like Twig. This gives you advanced features like template inheritance: Instead of writing every about page, contact page etc. from scratch, you can define a base template which already contains a default page structure. Within each theme, you only need to override the parts which are actually different. This again avoids duplication and makes the themes very compact. I recommend you look into existing projects and see how they do it (content management systems, blogs, forums, ...). The concept of themes isn't exactly new in web programming.

If the project is mostly object-oriented, consider moving the logic from the main script into a separate class. This will immediately solve the scoping issues and is generally cleaner. Or use closures as explained above. But I feel this is more like a hack.

What exactly are you trying to achieve? The only way to get a truly local function is to use a closure (anonymous function): <?php class Foo { public function bar() { // cannot be called here $baseFunction(); } } // closure $baseFunction = function () { echo 'Closure'; }; // can be called here $baseFunction(); $foo = new Foo(); $foo->bar(); Or you could use namespaces. This doesn't actually hide the function, but it makes it harder to access it.

As explained in the second answer, the mysql_* functions are ancient and have been removed from PHP. Manual SQL-escaping in general is deprecated, because it's far too error-prone (as you can see). Nowadays, we use PDO (or mysqli) together with prepared statements. The query string is supposed to be entirely static with no variable insertion of any kind. If you need dynamic input, you use query parameters and then bind the input to those parameters. This not only solves your current problem. It eliminates the risk of SQL injections altogether (as long as the query strings are in fact static).

This only displays the day difference.

The $intervals array is truly one of the weirdest pieces of code I've ever seen. Where do you got this from? It also seems you're trying to cast a timestamp string into a integer, somehow expecting it to (magically?) turn into a Unix timestamp. This makes no sense and will give you 2016 as Unix time, which was indeed 46 years ago. I'd scrap the code and do it properly with the DateTime class. Yes, PHP can do date calculations. <?php $timeIntervalAttributeNames = [ 'y' => 'year', 'm' => 'month', 'd' => 'day', 'h' => 'hour', 'i' => 'minute', 's' => 'second', ]; $testTimestamp = '2016-07-02 16:41:48'; $testDateTime = DateTime::createFromFormat('Y-m-d G:i:s', $testTimestamp); $now = new DateTime(); $diff = $now->diff($testDateTime); $formattedDiff = ''; foreach ($timeIntervalAttributeNames as $attribute => $name) { $val = $diff->$attribute; if ($val != 0) { $formattedDiff = $val.' '.$name.($val > 1 ? 's' : s); break; } } echo $formattedDiff;

The problem with an array is that it's rather cumbersome to turn that back into the corresponding rows. And the results are lost forever once the session has been garbage-collected, which means you don't really know what's happening on your site (no usage statistics, no overview, nothing). Storing the questions in the database will give you a lot more flexibility. I'd create one table for the quiz instances and one table for the randomly ordered questions of each quiz (together with their status). The session only needs to contain the quiz ID.

You have to go through the exact same prehashing procedure as above. function bcrypt_verify($password, $hash) { $binary_prehash = hash('sha256', $password, true); $encoded_prehash = rtrim(base64_encode($binary_prehash), '='); return password_verify($encoded_prehash, $hash); }

The only way to find out what's wrong is to actually check the PHP error log on your server. We can only guess. Maybe one of the functions is misspelled or not included in this particular script.

What I'm saying is that your (or Muddy_Funster's) database design doesn't implement any of the rules. Anybody can recruit anybody, regardless of what rank they have or whether the recruitment has been approved by their manager (they can simply approve it themselves). So where exactly are you planning to enforce the recruitment rules? In the PHP application? This doesn't really work (as explained above). With a database trigger as suggested by Muddy_Funster? That's theoretically possible, but it will be painful and difficult to understand. The layout of a database should reflect the actual data you're storing. That's currently not the case. The table says that anybody can recruit anybody and then get anybody's approval. In other words, the lowest staff member could recruit an assistment manager and simultanously approve their own decision. This is obviously nonsense. So it's not really about less tables vs. more tables. It's about the right tables.

As to the original question: While the salt isn't a problem, there are problems with the code. Do not rely on PASSWORD_DEFAULT. The whole point of modern password hash algorithms is to make the hashing as expensive as possible within the limits of your hardware. If you just go with the (low) default parameters, your hashes will be weaker than necessary. A better stratety is to choose a specific algorithm (PASSWORD_BCRYPT is the only one right now) and then increase the cost parameter until the duration is no longer acceptable. The previous value is the optimimum. Also don't hash passwords directly. The current bcrypt algorithm in particular cannot handle arbitrary input. Long passphrases (like the one by Muddy_Funster) and null bytes will lead to truncation, meaning the hash is again a lot weaker than expected. A common workaround is to pre-hash the password with another hash algorithm like SHA-256: <?php function bcrypt_hash($password, $cost) { /* * Pre-Hash the password with SHA-256 to obtain a 256-bit binary hash. Then encode the binary hash with Base64 to get a * string of 43 ASCII characters. This makes sure the string is within the length limit of bcrypt (56 bytes) and doesn't * contain any null bytes. */ $binary_prehash = hash('sha256', $password, true); $encoded_prehash = rtrim(base64_encode($binary_prehash), '='); return password_hash($encoded_prehash, PASSWORD_BCRYPT, ['cost' => $cost]); } $raw_password = 'I used to live at 221b Bakers Street, but got fed up with Danger Mouse ruining all the letters I put in the post box'; $cost = 12; // adjust this to your current hardware var_dump( bcrypt_hash($raw_password, $cost) ); --- 128 is the bit length of bcrypt salts. It has nothing to do with randomness. You'd have the same limit if you used sequential salts.

A random number generator doesn't guarentee anything – except the trivial fact that you're guaranteed to get duplicates when you've run out of salts. If that's what you wanted to know, the answer is 2^128 + 1 (the number of possible 128-bit salts and one more). A much more practical problem is to actually take the randomness into account and calculate the number of required users to reach a certain duplicate probability. That's what I was referring to in my reply: To get a 50% chance of identical hashes when the salts are random and the passwords are idential, you need around 2.2E19 users. The latter number is a lot smaller and can actually be alarming for shorter salts. For example, a 64-bit salt seems astronomical as well when you look at the number of possible values (~10^19). But it only takes a few billion users to end up with a 50% chance of a collision. This may not happen in a single application, but it will certainly happen accross multiple applications.

A collision probability of 0.5 would require ~2.2E19 users (see birthday attack).

Just skip the butthurt diva nonsense, and I'm sure we'll get along just fine.

PHP type comparison tables The attribute isn't limited to strings and integers, though. It can be set to absolutely anything. While this may be acceptable in small non-professional applications where bugs aren't a problem, I definitely wouldn't use public attributes for anything critical.

You're trying to compare a number (0) with a string (ufo::LARGE). In a strongly typed language, your code wouldn't even run. In a weakly typed language like PHP, the values get converted. And the integer value of "LARGE" is in fact 0: <?php var_dump((int) 'LARGE'); var_dump(0 == 'LARGE'); If you don't want this to happen, don't compare apples and oranges. I wouldn't even allow invalid values for the $mp_size attribute. Write a proper setter which rejects invalid values.

The problem of the above database layout is that it doesn't implement any of the rules, thus allowing nonsense data. For example, a single staff member with an arbitary rank (let's say the janitor) can recruit, pre-approve and approve an assistent manager. You can try to fix this with lots of application-side checks, but then you may still end up with nonsense data. What if the data is inserted or edited directly? What if one day there's a bug in the ever-changing application code? You'll never know if the data you're relying on is actually valid. I'd do this the other way round: Spend a lot of time on a proper database layout which will only accept correct data. This will in turn save you a lot of code and bugs. I see three different recruiting cases: The manager can recruit a person as an assistent manager or deputy manager An assistent manager can suggest a person as a deputy manager or executive, which must then be approved by the manager A deputy manager can suggest a person for an executive role, which must then be approved by both an assistent manager and the manager (or just the manager, I assume) Possible relations: staff(staff_id, last_name, first_name, ...) assistant_managers(staff_id) deputy_managers(staff_id) applicants(application_id, last_name, first_name, ...) manager_recruitments(applicant_id, role) assistent_manager_recruitments(applicant_id, role, approved_by_manager) deputy_manager_recruitments(applicant_id, role, approving_assistent_manager_id, approved_by_manager) You can simplify the relations with views. The physical layout isn't necessarily the layout you have to use when accessing the data. Note that the above data model allows an applicant to have multiple roles at once. If you want to prevent that as well, you'll need an additional table for all applications (which consist of an applicant and a role).