CyberShot Posted December 19, 2018 Share Posted December 19, 2018 (edited) I created an htlm form using $_SERVER['PHP_SELF'] to update my database and I am calling the form like so <?php if( $_POST['update']){ $unit = $_POST['unit']; $date = $_POST['date']; $amount = $_POST['amount']; $notes = $_POST['notes']; $york->update($unit, $date, $amount, $notes); } ?> in my database class, I have created an update function public function update($unit, $date, $amount, $notes){ $conn = $this->link; $sql = "UPDATE amount SET dues = ' . $amount . ', notes = '. $notes .' WHERE duesID = '. $unit .'"; if (mysqli_query($conn, $sql)) { echo "Record updated successfully"; } else { echo "Error updating record: " . mysqli_error($conn); } } $this->link connects to the database in my __construct and there are no errors as far as that goes. When I hit the update button in the html form, the page reloads and there is a message saying "Record updated successfully" but when I check the database, there isn't any new record. Looking at it again, I have realized that the query is looking for duesID to match the $unit number which in this case should be 10 but the amount table doesn't have a record for that. so what I need is a query that will select the address table and amount table then update the amount table where the address table unitID = $unit which is passed through my html form. How do I do that? The Address table does have a 14 entries in it with just to columns. Unit and unitID. Is this where a join comes into play? Edited December 19, 2018 by CyberShot Quote Link to comment Share on other sites More sharing options...
benanamen Posted December 19, 2018 Share Posted December 19, 2018 There are several issues with the code you posted. $_SERVER['PHP_SELF'] is vulnerable to an XSS Attack. You need to check the REQUEST METHOD, not rely on the name of a button or hidden field in order for the script to work. Do not create variables for nothing Your code is vulnerable to an SQL Injection Attack. Never ever put variables in your query. You need to use Prepared Statements. Do not output internal system errors to the user. 1 Quote Link to comment Share on other sites More sharing options...
CyberShot Posted December 19, 2018 Author Share Posted December 19, 2018 all of this is done on localhost. I would assume it's not as dangerous? I have updated the code to use the if($_SERVER['REQUEST_METHOD'] == 'POST') How do you perform an update that doesn't have variables? are you saying that they are ok to pass if I use prepared statements but not just in an mysqli->query? I do remember reading that PHP_SELF was vulnerable attacks. What should I put there then? I don't want to separate my update function out of the db class. What is the best method for this? why can't you output internal system errors? how do I get my errors if I don't output them? Quote Link to comment Share on other sites More sharing options...
benanamen Posted December 19, 2018 Share Posted December 19, 2018 (edited) Quote How do you perform an update that doesn't have variables? You already have the POST variables, just use them. Quote are you saying that they are ok to pass if I use prepared statements Yes Quote I do remember reading that PHP_SELF was vulnerable attacks. What should I put there then? Nothing. Just leave the action= out completely. Quote why can't you output internal system errors? how do I get my errors if I don't output them? Because it leaks sensitive info that is only good to hackers and useless to the user. You turn on error reporting in the local dev php.ini and you also log the errors. Edited December 19, 2018 by benanamen Quote Link to comment Share on other sites More sharing options...
Barand Posted December 19, 2018 Share Posted December 19, 2018 5 hours ago, CyberShot said: Is this where a join comes into play? yes UPDATE amount am INNER JOIN address ad ON am.duesID = ad.unit SET dues = ?, notes = ? WHERE ad.unitID = ? Quote Link to comment Share on other sites More sharing options...
CyberShot Posted December 20, 2018 Author Share Posted December 20, 2018 Barand. I am trying to get your query working but I am having trouble. It is close though but I am getting an error at the last line. Here is what my database looks like table address unit ( varchar(20) ) unitID ( int, primary key, auto increment ) table amount autodate (timestamp) dues (int(6) ) late (varchar(1) ) paid (varchar(1) ) notes ( varchar(500) ) duesID ( int, primary key, auto increment ) Right now, I am just trying to get a test query in so that I can get a "scaffold" to work off of. so I thought the way this would work is that the query would check the unitID in the address table to see if it matches the "unit" number passed through the post variable from the html form. Benanamen said not to create useless variables, so I took them out and just used the post variables as suggested. So my html form has a text box for unit #, date, amount paid and notes Right now, I am just trying to get the unit # to match the primary key in the address table and then put the amount, and notes into the amount table. After I get that working, I will work off of that to get the rest of the fields to populate. I have 14 entries in the address table and 1 in the amount table. As the query is now, it's says I have an error in my sql syntax "WHERE ad.unitID = '10' The ten should match the primary key in the address table and put the record into the amount table to match. Here is what I have been trying with my query $sql = "UPDATE amount am INNER JOIN address ad ON ad.unitID = '{$unit}' SET dues = {$amount}, notes = '{$notes}', WHERE ad.unitID = {$unit}"; Quote Link to comment Share on other sites More sharing options...
Barand Posted December 20, 2018 Share Posted December 20, 2018 Can you post a dump of the data in those two tables? Quote Link to comment Share on other sites More sharing options...
CyberShot Posted December 21, 2018 Author Share Posted December 21, 2018 (edited) here are my two tables. you will see in the below in the amount table, there is one record that has a duesID of 5 which does correspond to the address table unitID of 5. This is the correct record to show for unit 4109. I am trying to get a query that will choose the unitID and put the proper record down below so that when I query the database, the correct record will show for the correct unit. Right now, I have a form with unit #, date, amount paid and notes. My thought was that I could put in the unit number and then have the query select the matching number from the unitID in the address table and then insert the record into the amount table with the matching duesID number so that when I want to select a single record, I could just choose the unit number which will match the unitID and that record will populate. mysql> select * from address; +--------+----------------+ | unitID | unit | +--------+----------------+ | 1 | 4101 | | 2 | 4103 | | 3 | 4105 | | 4 | 4107 | | 5 | 4109 | | 6 | 4111 | | 7 | 4113 | | 8 | 4115 | | 10 | 4119 | | 11 | 4121 | | 12 | 4123 | | 13 | 4125 | | 14 | 4127 | | 9 | 4117 | +--------+----------------+ mysql> select * from amount; +--------+------+------+------+---------------------+------------+---------------------+ | duesID | dues | late | paid | notes | date | autodate | +--------+------+------+------+---------------------+------------+---------------------+ | 5 | 210 | 0 | 1 | paid through paypal | 2018-12-04 | 2018-12-10 20:54:07 | +--------+------+------+------+---------------------+------------+---------------------+ Edited December 21, 2018 by CyberShot Quote Link to comment Share on other sites More sharing options...
Barand Posted December 21, 2018 Share Posted December 21, 2018 (edited) It's unusual to join the primary key of one table to the primary key of another. Do you only ever want one amount record for each address? Try UPDATE amount am INNER JOIN address ad ON am.duesID = ad.unitID SET dues = ?, notes = ? WHERE ad.unit = ? Edited December 21, 2018 by Barand Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted December 21, 2018 Share Posted December 21, 2018 you are going about this in the hardest way possible. even the naming of your tables and columns is not helping. your address table appears to be a rental unit table. it should be named that - rental_units. the id column in this table defines the unit id, the other column in that table is apparently the unit name. it should be named so - name. i'm assuming it is a varchar column because rental unit names can include alphabetic characters, A, B, C...? the amount table is some sort of unit dues payment table. it should be named to indicate the meaning of the data in the table - due_payments or similar name. the id column in this table defines the payment id, not the unit id. this table should have a separate unit_id column to relate the payment record back to the unit it corresponds to. the form to insert a new payment record should have a way of selecting from the existing units, such as a select/option menu. this will reduce typo mistakes and only allow valid units to be chosen. this select/option menu would display the unit name, but the value that is submitted would be the unit_id. the INSERT form processing code would insert a record into the payment table with the submitted unit_id, amount, and other fields you have defined. if you are editing an existing payment record, such as when correcting a typo in a value, you would reference the specific record using its payment id, not the unit_id. to display the corresponding unit name as part of this process, the SELECT query that retrieves the existing payment record would JOIN with the units table to get the unit name. the UPDATE form processing code would update the record indicated by the payment id value. Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted December 21, 2018 Share Posted December 21, 2018 (edited) as to displaying/logging the database statement errors. use exceptions to handle all database statement errors (connection, query, prepare, and execute) and in most cases let php catch the exception where it will use its error_reporting, display_errors, and log errors_settings to control what happens with the actual error information. the exception to this rule is if your application needs to detect and handle duplicate values for an insert/update query. your code would catch the exception in this case, detect if the error was for a duplicate index, set up and output an error message to the user telling them what is wrong with the data they submitted, or if error is not for a duplicate index, re-throw the exception and let php handle it. when learning, developing, and debugging code/queries, you would display all errors, which now includes the database statement errors, and when on a live/public server, you would log all errors, just by changing the php display_errors and log_errors settings. this will allow you to remove the existing error handling logic in your code, thereby simplifying it. to enable exceptions for the php mysqli extension, add the following line of code before the point where you are making the database connection - mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); lastly, since you need to change to use prepared queries when supplying external/unknown values to the sql query statement, you need to switch to the much simpler php PDO extension for all the database statements. the amount of code you have to write to accomplish a prepared query using the mysqli extension is an error prone waste of time (and the code needed to write a mysqli general purpose prepared query function/method is complicated and slow.) Edited December 21, 2018 by mac_gyver Quote Link to comment Share on other sites More sharing options...
CyberShot Posted December 22, 2018 Author Share Posted December 22, 2018 20 hours ago, Barand said: It's unusual to join the primary key of one table to the primary key of another. Do you only ever want one amount record for each address I only need one record per month per unit. each unit could actually make multiple payments per month though. My plan is to output each month into a table and each month the data in that table would reset to zero to show the new payments for each unit for the current month. Then the database would hold all payments made over time. I would like to query the database to show units that have not paid, how much they owe and when the last payment was made and what months are missing. You said it's unusual to join the primary key of one table to another. I guess I don't understand because that is how I thought it was done. What is the right way to do it as that is the way I would like it to be. Quote Link to comment Share on other sites More sharing options...
CyberShot Posted December 22, 2018 Author Share Posted December 22, 2018 12 hours ago, mac_gyver said: as to displaying/logging the database statement errors. use exceptions to handle all database statement errors (connection, query, prepare, and execute) and in most cases let php catch the exception where it will use its error_reporting, display_errors, and log errors_settings to control what happens with the actual error information. the exception to this rule is if your application needs to detect and handle duplicate values for an insert/update query. your code would catch the exception in this case, detect if the error was for a duplicate index, set up and output an error message to the user telling them what is wrong with the data they submitted, or if error is not for a duplicate index, re-throw the exception and let php handle it. when learning, developing, and debugging code/queries, you would display all errors, which now includes the database statement errors, and when on a live/public server, you would log all errors, just by changing the php display_errors and log_errors settings. this will allow you to remove the existing error handling logic in your code, thereby simplifying it. to enable exceptions for the php mysqli extension, add the following line of code before the point where you are making the database connection - mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); lastly, since you need to change to use prepared queries when supplying external/unknown values to the sql query statement, you need to switch to the much simpler php PDO extension for all the database statements. the amount of code you have to write to accomplish a prepared query using the mysqli extension is an error prone waste of time (and the code needed to write a mysqli general purpose prepared query function/method is complicated and slow.) I agree with you 100%. I have managed to create an html select that does display each unit from the database where the value of the select is the address table primary key. This is a home owners association and what I am trying to keep tack of is the HOA dues. Those dues pay the water, sewer, garbage, landscaping and electrical for the complex. There are 14 units. Each unit pays monthly. There is no good cheap software out there that does what I want so I was trying to make something myself but as you can tell, I don't know what I am doing. I made the database, inserted some records and have managed to display them in a table. The html form was at this point was just to get a working query that I could work off of and then make more correct with prepared statements at a later time. Once the basic structure was up and running. The update queries are not working. I can get it to tell me that the database has been updated but the new record doesn't actually show up. Maybe I should just scrap the project and go back to watching TV and playing games. Quote Link to comment Share on other sites More sharing options...
Barand Posted December 22, 2018 Share Posted December 22, 2018 (edited) Store payments as individual transactions. When a payment is made, insert a new record in a payment table Table: unit +---------+--------------------+ | unit_id | address | +---------+--------------------+ | 1 | 2 Effin Close | | 2 | 9 Letsby Avenue | +---------+--------------------+ | | +---------------------------------+ | Table: payment | +------------+------------+------------+----------+ | payment_id | unit_id | date_paid | amount | +------------+------------+------------+----------+ | 1 | 1 | 218-08-15 | 250.00 | | 2 | 2 | 218-08-17 | 250.00 | | 3 | 1 | 218-09-15 | 250.00 | | 5 | 1 | 218-10-15 | 250.00 | | 4 | 2 | 218-10-28 | 150.00 | | 6 | 1 | 218-11-15 | 250.00 | +------------+------------+------------+----------+ Now you can see what they have paid, and that Unit 2 missed payments in September and November and didn't pay the full amount in October. Edited December 22, 2018 by Barand Quote Link to comment Share on other sites More sharing options...
CyberShot Posted December 23, 2018 Author Share Posted December 23, 2018 for the unit table. I set the unit_id as the primary key and auto increment and for the payment set the unit_id as primary key auto increment? Quote Link to comment Share on other sites More sharing options...
CyberShot Posted December 23, 2018 Author Share Posted December 23, 2018 (edited) Ok, I have remade my table to match what you have posted.I had to alter the tables in order to set up the foreign keys. Was that necessary? ALTER TABLE unit ENGINE=InnoDB; ALTER TABLE payment ENGINE=InnoDB; I was then able to set up the relationships. I set the payment_id a the primary_key and the unit_id as the foreign key in the payment table. The unit_id is the unit table should be the primary key set to auto_increment. Is this good? Edited December 23, 2018 by CyberShot Quote Link to comment Share on other sites More sharing options...
Barand Posted December 23, 2018 Share Posted December 23, 2018 I would change the amount type unit +----------------+---------------------+-----------------+ | column | type | keys | +----------------+---------------------+-----------------+ | unit_id | int | PK AI | | address | varchar(20) | | +----------------+---------------------+-----------------+ payment +----------------+---------------------+-----------------+ | column | type | keys | +----------------+---------------------+-----------------+ | payment_id | int | PK AI | | unit_id | int | FK | | date_paid | date | | | amount | decimal(10,2) | | monetary amounts are better with fixed precision instead of float +----------------+---------------------+-----------------+ Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.