To me it also seems weird to use a failed insert to decide to do an update. I don’t think the one extra redundant database query for an update matters much except for high traffic systems. However, I wouldn’t choose this method because to me a database error logically is a sign of a problem to be fixed and not a normal application flow - unless I check specifically for the error code returned from the database and act accordingly. Usually, I have my db class set up to log all database errors to a file and in this situation the expected failed inserts (in case of edits) would pollute my logs unless I code an additional flag to prevent that from happening. Therefore, while I wouldn’t call this method wrong it is not what I would call logical or obvious.
I prefer to simply use a hidden field in a form to hold the record ID and have it empty for new records. Then deciding whether to insert or to update is a matter of checking for the existence of the ID. I suppose that’s what @benanamen had in mind, too. And this works very well also for natural primary keys which are user-editable as part of the form.
Also, while I understand there is ultimately a bit less code when it’s done @TomB’s way I find it’s not practical to make the code that universal for both inserts and updates because as the complexity grows there are often requirements for a slightly different logic for each of these actions. For instance, often there is a need to supply a current timestamp value to inserted rows while not to existing ones. When checking for an ID from the form it’s quite simple to do:
How would you do it when you try to always insert first? One idea:
$recordToInsert = $record; // clone entire record
$recordToInsert['created_at'] = date('Y-m-d H:i:s');
if (!$this->insert($recordToInsert)) {
$this->update($record);
}
To me it’s a little bit more convoluted (in terms of logic, not amount of code) and as requirements grow the complexities will also grow and I find it cleaner to be able to make the decision upfront whether an insert or update is needed. Sometimes I even keep them as separate methods in order to minimize the amount of if/else statements. It’s easier to follow the code of a method if it has a single purpose. But this is a matter of weighing pros and cons and obviously everyone may make different decisions.
This breaks for keys where the table is not auto-increment and requires adding an additional layer of logic to account for those situations.
And for composite keys requires more logic still.
if (empty($id)) {
$record['created_at'] = date('Y-m-d H:i:s');
}
$table->save($record);
No, why would it break for not auto-increment? It doesn’t matter if the PK is AI or not, the ID of the record is passed to the update function - it doesn’t matter what kind of key it is.
A bit - just one more field.
Maybe simpler but what if the PK is a text field and the user changes the PK? Your save/update method doesn’t event know how to find the record because $record doesn’t contain the PK before the change, unless you add a magic field into $record with the old PK value.
In this special case it needs the same amount of logic your approach needs for every case.
Because you need the logic:
if (empty($record['id']) {
$table->insert($record);
}
else {
$table->update($record);
}
If the table is not auto-increment then it breaks, it’ll always try to update because id has been supplied by the user on the form.
edit: regardless of the approach you use there will be cases where you need some special logic. However, I’d prefer a default approach that handles most cases with minimum code. In most cases I find that tables are auto-increment and the record needs to be updated if the PK is set.
It’s possible to move this code:
if (empty($record['id']) {
$table->insert($record);
}
else {
$table->update($record);
}
into a function so it’s reusable, but the primary key is not always ID and it’s not always a single column. In the interest of having some generic code that handles most use-cases, the try/catch approach works well.
Yes, you are right, in this case the if should be like this:
if (empty($old_id) {
where $old_id is passed from the form.
I see your approach looks good but still checking for a failed insert looks weird. If I were to do this at least I’d check for a specific exception, some db drivers like DBAL have exceptions for various database errors:
I’m still looking for someone to show a practical downside rather than “eww”. Out of interest how do you feel about REPLACE INTO or ON DUPLICATE KEY UPDATE or even a preliminary SELECT that checked to see if the PK was in use before doing an insert or update in the save function?
The save function’s API would look identical. If there was an issue with the data (e.g. trying to write to a column that doesn’t exist) it would throw a PDOException. Someone using the object and looking at the API alone would not know or care which of those three (or try/catch) was used.
I guess my question is if ON DUPLICATE KEY UPDATE is fine, but you can’t tell if it’s being used without looking at the code what difference does it make? What we’re really interested in is preventing negative side effects by choices the code makes. If, to an object using try/catch and another using REPLACE INTO are indistinguishable, why is one a problem and the other not?
There is none I know of - I said earlier that I don’t consider this incorrect, only to me it looks more logical to user other methods than database errors to determine whether to do an insert or update. In other words - subjective preference, so you won’t get anything more from me than “eww”
I think they are perfectly fine. But every time we use methods like this it’s important to think carefully about the execution flow to make sure we don’t run into dangerous race conditions, etc. For example, in your save method’s case it’s important that when an insert fails and you catch the exception and continue to update - that the update also fails but fails with an error, not silently. Otherwise, in some rare cases (like database connection errors, etc.) a user might try inserting a new record - the insert will fail, the update will fail - but if the update does not trigger a visible error the user might think the record was inserted. This might be a problem if your update method issued a well-formed query for an non-existent ID for an insert action like this:
UPDATE mytable SET name='Mark' WHERE id=NULL;
This is the reason why I prefer to use a single UPDATE query for an update - even if it takes a little bit more coding, it’s not that much effort but at least I’m free from thinking about potential problematic edge cases like the one above.
LOL! Your exactly right and that is exactly what I ripped out to provide the most minimal example I could show to get my point across. Really, go look at my repo, it’s been up exactly as you say for some time now. Excellent observation Mr. @rpkamp!
But that still requires the logic for choosing to update or insert on every single form you create, the whole thing I was avoiding with the save method.
It also mixes a lot of different concerns, most are irrelevant for this thread.
What are you even talking about? That is a very clean, sensible, logical solution to a single add/edit page. It either does the single task of an insert using a single method, or it does the single task of an edit using a single method, in a single simple line with YOUR exact code.
Ok, I am already reading the book so I will withhold any further comment until I finish the book.
While I was actually using the class outside your app I did find a small problem in the update routine. Allow me to suggest a minor tweak.
As is, you set a variable for the Primary Key which is great, but the routine requires that the key be named id. Since I use a contact_id type naming convention, I noticed it right off. Also there is a minor issue in the query that is generated. Right now, the query (minus the form fields) says (Example: UPDATE mytable SET id = 5 WHERE id=5. There is no need to update the key value with the exact same key value. The “fix” for both is quite minimal. Take not of the two changes I made.
public function update($fields) {
$id = array_pop($fields);// benanamen mod
$query = ' UPDATE `' . $this->table .'` SET ';
foreach ($fields as $key => $value) {
$query .= '`' . $key . '` = :' . $key . ',';
}
$query = rtrim($query, ',');
$query .= ' WHERE `' . $this->primaryKey . '` = :primaryKey';
//Set the :primaryKey variable
$fields['primaryKey'] = $id;// benanamen mod
$fields = $this->processDates($fields);
$this->query($query, $fields);
}
In post #35 I did acknowledge I saw some code that appeared to be similar. Since I was not and still not familiar with all the code I didnt know the context in which it was used so I left it at that.
I think it’s best I just finish reading your book first before addressing anything you asked. It will give me a better insight into you and what your are teaching. It’s been a really good read so far. I would definitely recommend people read it no matter what skill level they are at.
That fix assumes the key is the first entry in the array. Try putting your hidden id input at the bottom of the form and send it what happens
As we know the pk you could unset it, but in the interest of avoiding unnecessary complexity, I didn’t cover that in the book.
Please can you show how you would handle two different forms? Doing so will demonstrate to yourself where the repetition is and which concerns you are mixing.
Ultimately, using my approach I don’t even need to repeat the code calling the save function for each additional form (can post code later when I’m not on mobile).
No need, it would obviously not work. As your book says, the Class author gets to tell people how to use it which would include the hidden id at the bottom of the form. But I get a noob could stick it anywhere. In fact, in my noob days, I think I used to stick hidden fields at the top before anything.
So how would you address a DB key not named id then?
try putting it at the top of the form.