I'll stick to reviewing your PHP code, as it doesn't look like it's recieved the attention it deserves!
I'll do what I can to find faults, but I won't be testing anything. Alright, we ready?
- Nice! First line and we've found something! What kind file is
functions.php!? Filenames should be one of the clearest names in your project. If you can't come up with a good filename, then that's a sign you need to break apart the contents within.
- I'm assuming the initial conditional
empty() is for testing only? I hope so, because automatically updating data on page load is a disaster waiting to happen. Send a couple hundreds requests to a page that's doing resource heavy database access and you'll happily be sitting in a DoS attack.
It's a little hard to tell, but it looks like those variable sets in that conditional are longer than any desired line length. It makes it much harder for the reader to effienctly read your code if they have to do a lot of horizontal scrolling.
You can reduce this a couple ways. In this case, break down your array over multiple lines:
$_POST['UPDATE_HEADERS'] = array(
'table' => 'polisy_oc',
'id' => 'numer_polisy_oc',
'id_value' => '72451'
This way is substantially easier to read in my opinion.
For other really long lines, you may be able to put data in variables, and then reference those variables.
- @200_success had a good point:
UpdateRecord isn't a very good name. Although it's hard to rename this class because it really doesn't have a single responsibility. That in itself contradicts the principles of SOLID. You might look towards making a
__construct($db_connection) should really be
__construct(Database $db_connection) to better implement dependency injection practices.
- I'd avoid automatically calling
update() from the constructor. Make
update() public and call it where the object is initialized.
update() were in a
Record class, that'd be OK. But here, I don't think the name is right. For all I know, this could really be updating the record updater!
If you don't change the class name, I suggest this be a method called
- Where do we get
$_SESSION from? The user. And what's our golden rule? Never trust the user. I think it's a bad idea to check the POST data against session data. Make a concrete white-list somewhere. Either that, or eliminate the need to dynamically reference a table. I'm sure with enough whiteboard space you'll be able to figure it out!
- Avoid using the default
$value. Give some more descriptive variable names.
- What is
Alright! One file down, now the next?
Database is a very vague name. You got the noun part down, but seeing something like this would kill me!
- Something like
$connection doesn't need to be
public. I'd make that private/protected and encapsulate it.
- Add a visibility to your constructor.
- Your constructor has too much business logic in it! It should be for constructing the object, nothing more. Break the logic into other methods.
- Oh goodness! I do not like how you're setting up the database via the session! That is asking for a lot of trouble! Why can't the credentials be hardcoded? Or maybe put in a config file and access that way? The user has your database credentials!
- Your DBAL shouldn't have any type of business logic in it what-so-ever. It defeats the point of having it in a separate class.
And lastly, you mention your code bottlenecking, but you're not sure why. Profile your code, get some concrete numbers, and then work from there. Is it that
foreach that's causing the lag, or it say the query? You'll never know unless you test it!
In the comments a couple questions were asked. I'll do my best to answer them:
functions.php is a file which contains database class and functions that are used by more than one class; shouldn't I use such resolution?
It's vague. What kind of functions are in this file? What kind of functions should go in this file? When is it appropriate to have a new class, versus just add functions into here. It's a problem of extending functional programming along side OO programming.
Making a Record class would mean that it would have methods like updateClient(), insertClient() etc. inside, which I thought would make connecting app to database harder - wouldn't it be simpler for an app just to invoke desired class which is in separate .php file?
Those methods you list might actually be a better fit in a
Client class. But having CRUD operations in a single class is not a bad thing. It keeps all "record" things together, and all "client" things together.
Why adding class name in __construct parameters implements better dependency injection?
It ensures that the type hinted class is being introduced into the object. If I say
__construct(Database $db), I know that a database will be passed. It also helps inheritance efforts. I could code something like
__construct(API $api), and then if I have
ServiceAPI classes, both of which extend
API, then I can pass either of those, too.
in update class, I check provided table and column with [the $_SESSION global] white-lists, why is it bad?
You don't have control over the session. It's so easy to take advantage of vulnerable sessions, it just takes the right mind. Here's just the a beginner's answer to session hijacking/fixation.
I want an user of app to be the user of the database either (docs.oracle.com/cd/B28359_01/network.111/b28531/…), that's why I've come up with creating connection by $_SESSION.
From the Oracle link you provided, right off the bat I'll give you text from that page:
You can store password credentials for connecting to a database by using a client-side Oracle wallet. An Oracle wallet is a secure software container that stores the authentication and signing credentials needed for a user to log in.
I point this out because of several reason:
- Oracle isn't advocating PHP sessions. They want you to use their secure software to authenticate. These are two different realms here!
- Application users should really only need to be database users if you have that much data. I don't see any reason to do this if you are the average web developer. If you are scaling up to this approach, you will want a team. Things will become very messy if each user get's their own set of databases!
How can I connect to database by mysqli using password hashes?
You can't. Not hashes anyways. Know the differences between hashing, encrypting, etc.
You may need to rethink your program architecture before getting into this. I don't have the experience to tell you how it should be done.