Jump to content

mac_gyver

Staff Alumni
  • Posts

    5,357
  • Joined

  • Days Won

    173

Posts posted by mac_gyver

  1. the points that have been made have INCLUDED the reason why you should do these things. these points have been to make your code - secure (it currently is not), provide a good user experience, simplify (i and others have used a form of the word simple in several of the replies) the code (what you have shown contains a lot of unnecessary and unused code, variables, and queries), reduce resources and speed up the code (making a database connection is one of the most time consuming operations you can do on a page, which is why you should make only one per page), and through having useful error handling and validation logic, help get your code/query(ies) to either work or get them to tell you why they won't.

    the reason we are trying to get you to simplify the code is because you MUST go though all this code at least once to update it. if you can eliminate the unnecessary code, variables, and queries, you will have less things that must be updated.

    if you really have hundreds of pages of bespoke code like what you have shown so far, it may be a better use of your time learning and using a general-purpose data driven design, so that you can get the computer to do the work for you, rather than you spending hours upon hours writing and trying to maintain code on so many web pages.

  2. if you make use of the suggestions given in this thread for the LogMeX2 function code, you should end up with this simple, and secure code (untested) -

    // verify the user log in credentials
    // return user id if a match, else return false
    function LogMeX2($pdo,$username,$password)
    {
    	$sql = "SELECT User, UserKey FROM LIBusersX WHERE UserN = ?";
    	$stmt = $pdo->prepare($sql);
    	$stmt->execute([$username]);
    	// fetch/test if a row was found, i.e. the username was found
    	if(!$row = $stmt->fetch())
    	{
    		// username not found
    		return false;
    	}
    	// username found, test password
    	if(!password_verify($password,$row['UserKey']))
    	{
    		// passwrod didn't match
    		return false;
    	}
    	// username and password matched, return user id
    	return $row['User'];
    }

     

  3. nothing has changed about or die() being supported. it was always a bad practice to use for error handling and since the PDO extension uses exceptions for errors for all (default in php8+) the statements that interact with the database server - connection, query, prepare, execute, and exec, you were told the or die() logic will never be executed upon an error and should be removed.

    the great thing about using exceptions for errors is that your main in-line code only 'sees' error free execution, since execution transfers to the nearest correct type of exception handler upon an error or to php if there is no exception handling in your code. if execution continues to the line right after you have executed a query, you know that there was no error, without needing any conditional logic, simplifying the code.

    the posted logic makes no sense. you are running a SELECT query to get a count of the number of rows, to decide to run the SELECT query again. just run the second SELECT query and fetch the data from it. if the query matched a row, the fetched data will be a boolean true value. if the query didn't match a row, the fetched data will be a boolen false value.

    you were also told elsewhere that magic_quotes, which provided some security for string data values, has also been removed from php and that you need to use a prepared query when using external, unknown, dynamic data with a query.

    converting any query to a prepared query is extremely simple -

    1. remove the variables that are inside the sql query statement (keep these variables for use later).
    2. remove any single-quotes that were around the variables and any {} or concatenation dots that were used to get the variables into the sql query statement.
    3. put a prepared query place-holder ? into the sql query statement where each variable was at.
    4. call the ->prepare() method for the resulting sql query statement. this returns a PDOstatement object, and should be named $stmt or similar.
    5. call the ->execute([...]) method with an array containing the variables you removed in step #1.
    6. for a SELECT query, use either the ->fetch() method (for a single row of data), the ->fetchAll() method (for a set of data), or sometimes the ->fetchColumn() method (when you want a single value from one row of data.)

    lastly, md5() was never intended for password hashing. you should be using php's password_hash() and password_verify() for passwords. also, why are you hashing names/usernames? this prevents any wild-card searching or sorting.

  4. you have stated you have a lot of code that needs to be converted. a GOOD reason to name the variable as to the type of connection it holds, e.g. $pdo, is so that you can tell by looking/searching which code has been converted and which code hasn't, then after it has been converted, anyone looking at the code can tell which database extension it is using (parts of the mysqli extension look exactly like the PDO extension.)

  5. the OP is attempting to learn, develop, and debug code/query(ies) on a live/public server. either the last posted code wasn't successfully getting uploaded to the server or a lot of cheap web hosting set disk caching (time) to a high value to get as many accounts onto one server as possible and it was taking some time for the changes to take effect.

    you should be learning, developing, and debugging your code/query(ies) on a localhost development system and only putting your code onto a live/public server when it is mostly completed.

  6. 21 minutes ago, Paul-D said:
        		PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        		PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_OBJ

    when you make the connection, you should set the error mode to exceptions, which is what you are doing, set emulated prepared queries to false, you need to add this, and set the default fetch mode to assoc, you are setting it to obj, which is not what you are using in the example code and is probably not what your existing code is using  with the fetched data, which will require you to make even more changes to your existing code.

  7. the error is due to a counting problem. you have 5 place-holders in the sql query statement, but are only supplying 4 values.

    as to the posted code -

    1. don't attempt to SELECT data first to determine if it exists. there's a race condition between multiple concurrent instances of your script, where they will all find from the existing SELECT query that the data doesn't exist, then try to insert duplicate values. instead, define the username and email columns as unique indexes, just attempt to insert the data, and detect if a duplicate index error (number) occurred. if the data was successful inserted, the values didn't exist. if you did get a duplicate index error, and since there is more than one unique index defined, it is at this point where you would query to find which column(s) contain duplicate values.
    2. the SELECT query, in addition to needing = (comparison operators) needs an OR between the WHERE terms, to find any matching rows with either value, not just one row with both values.
    3. if you use implicit binding, by supplying an array of values to the ->execute([...]) call, you can eliminate all the bindValue and bindParam calls, simplifying the code.
    4. if you set the default fetch mode to assoc when you make the database connection, you  won't need to specify it in each fetch statement, simplifying the code.
    5. you should use exceptions for database statement errors and only catch and handle a database exception in your code when inserting/updating duplicate user supplied values (which is what you are trying to do.) the PDO extension always uses exceptions for errors for the connection, and in php8+ the default is to use exceptions for all the other database statements that can fail. you would have exception try/catch logic for the insert query, then test if the error number is for a unique index error, then query to find which/both of the two unique columns already contain the submitted values. for all other error numbers, just rethrow the exception and let php handle it.

    edit: you should also be using php's password_hash() and password_verify() to hash and test the password value.

  8. your login system should only store the id of the logged in user in a session variable. this is the only session variable you should be using related to what you are trying to accomplish in this thread. you would then query the database table(s) on each page request to get any other user data, such as a username, user permissions, membership level, ... the reason for doing this is so that any change made to this user data will take effect on the very next page request (without requiring the user to log out and back in again.)

    once you have the user data, or lack of, in an appropriately named variable (or instance of a class), you would use it, or lack of, in logic to control what the current user can do or see on the current page.

  9. php builds associative indexed arrays from left to right and top to bottom. when you have the same index name more than once in the data, the last value overwrites any previous value. since you are SELECTing the accounts.id last, that is the value you are getting for the id index.

    are you even using the accounts.id in the fetched data? if not, don't include it in the SELECT list. likewise for any other data values you are not actually using in the feteched data.

  10. true prepared queries, regardless of the database extension, are equally safe, since they separate the parsing of the sql query syntax from the evaluation of the data. PDO has emulated prepared queries, that should be avoided whenever possible, since, if you haven't set the character set that php is using to match your database tables, which is rarely shown in connection code examples, it is possible for sql special characters in a value to break the sql query syntax, which is how sql injection is accomplished.

    the PDO examples you have seen that look messier, are probably using named place-holders. PDO also has positional ? place-holders, just like the mysqli extension, which makes converting from using the mysqli extension to PDO straight forward, since the sql query syntax using ? place-holders is exactly the same. to convert to using the PDO extension, you would eliminate the existing msyqli bind_param() call, and supply the array of input data to the ->execute() call. because you can directly fetch data from a PDO prepared query, exactly the same as for a non-prepared query, you don't need to deal with mysqli's bind_result() or get_result() (which is not portable between systems that use and don't use the msyqlnd driver.)

    another issue with the mysqli extension is that the procedural and OOP notation have different php error responses (though php finally made the mysqli connection always throw an exception upon an error.) things which are fatal problems, that produce fatal php errors when using OOP notation, only produce warnings when using mysqli procedural notation, and allow code to continue to run, producing follow-on errors.

    an advantage of learning the PDO extension, is that the same php statements work with 12 different database types, so, if you ever need to use a different database type, you don't need to learn a completely new set of php statements for each one.

     

     

    • Like 1
  11. or you could use the much simpler, more consistent, and more modern PDO extension, which doesn't have any function/method that takes variable length arguments. in fact, you don't need to use explicit binding with the PDO extension. you can simply supply an array of inputs to the ->execute([...]) method call, which can be an empty array if you happen to dynamically build a query that doesn't have any input parameters in it.

  12. you need to put the form processing code and the corresponding form on the same page. what you have now takes almost two times the amount of code and by passing messages through the url, it is open to phishing attacks and cross site scripting.

    the code for any page should be laid out in this general order -

    1. initialization
    2. post method form processing
    3. get method business logic - get/produce the data needed to display the page
    4. html document

    here's a list of points that will help you with your code -

    1. validate all resulting web pages at validator.w3.org
    2. since you will be putting the form and the form processing code on the same page, you can leave the entire action='...' attribute out of the form tag to cause the form to submit to the same page it is on.
    3. if you put the <label></label> tags around the form field they belong with, you can leave out the for='...' and corresponding id='...' attributes, which you don't have anyways.
    4. use 'require' for things your code must have for it to work.
    5. if you are building multiple web pages, use 'require' for the common parts so that you are not repeating code over and over.
    6. the post method form processing code should first detect if a post method form was submitted. in your current code, writing out a series of isset() tests, you are creating bespoke code that must be changed every time you do something different. also, if you had a form with 30 or a 100 fields, would writting out a series of 30 or a 100 isset() statements seem like a good use of your time?
    7. forget about this validate() function. it is improperly named and the only thing it is doing that is proper is trimming the data.
    8. don't create a bunch of discrete variables for nothing. instead keep the form data as a set in a php array variable, then operate on elements in this array variable throughout the rest of the code.
    9. your post method form processing code should trim, than validate all input data, storing user/validation errors in an array using the field name as the array index. after the end of all the validation logic, if there are no errors (the array will be empty), use the submitted form data.
    10. the only redirect you should have in your code should be upon successful completion of the post method form processing and it should be to the exact same url of the current page to cause a get request for that page.
    11. you should not store plain text passwords. use php's password_hash() and password_verify()
    12. you cannot successfully echo or output content before a header() redirect.
    13. the only value that you should store in a session variable upon successful login is the user's id. you should query on each page request to get any other user information, such as their username or permissions.
    14. if there are user/validation errors, and since you are now putting the form and the form processing code on the same page, your code would continue on to display the html document, display any errors, re-display the form, populating any appropriate fields with there existing values so that the user doesn't need to keep reentering data over and over.
    15. any dynamic value that you output in a html context, should have htmlentities() applied to it when you output it, to help prevent cross site scripting.
    16. when conditional failure code is much shorter then the success code, if you complement the condition being tested and put the failure code first, it is easier to follow what your code is doing. also, any time you have an exit/die in a conditional branch of code, you don't need to make any following branch 'conditional' since exaction won't continue past that point.
    17. don't let visitors (and hackers) know when internal errors have occurred, such as database connection errors. instead, use exceptions for errors and in most cases simply let php catch and handle any exception, where php will use its error related settings to control what happens with the actual error information. also, in the latest php versions, a mysqli connection error automatically uses exceptions, so your existing connection error handling logic will never get executed upon an error and might as well be removed, simplifying the code.
    18. the file upload form processing must detect if the $_FILES array is not empty before referencing any of the $_FILES data. if total posted form data exceeds the post_max_size setting, both the $_POST and $_FILES arrays will be empty, you must detect this condition and setup a message for the user that the size of the uploaded file was too large. i suspect this is the reason why you are getting an empty $_FILES array and the undefined array index error.
    19. after you have detected that there is data in $_FILES, you must test the ['error'] element. there will only be valid data in the other elements if the error element is a zero (UPLOAD_ERR_OK) you should also setup user messages for the other errors that the user has control over. you can find the definition of the upload errors in the php documentation.
    • Great Answer 1
  13. just use a single JOIN query to do this.

    next, you should always list out the columns you are SELECTing in a query so that your code is self-documenting. you should build your sql query statement in a php variable, to make debugging easier and help prevent syntax mistakes, and you should set the default fetch mode to assoc when you make the database connection so that you don't need to specify it in each fetch statement.

    the reason why your current code doesn't work, is because fetchAll(), the way you are using it, returns an array of the rows of data. if there are three rows in the company table, you will have an array with three rows in it, with each row having an assignedto element. you can use print_r() on the fetched data to see what it is.

     

    • Like 1
  14. 35 minutes ago, mythri said:
    SELECT email FROM ven_contacts WHERE vendor=?

    how many email addresses per vendor? if there's only one, you don't need the 2nd foreach() loop to fetch it, just directly fetch the single row without using a loop.

    next you don't even need the 1st foreach() loop, just implode $_POST['vendor'] to make a comma separated list, then use FIND_IN_SET(vendor,?) in the WHERE clause to match all the rows in one query, then just use the result set from that single query. if you use PDO fetchAll() with the PDO::FETCH_COLUMN fetch mode, you will get a one-dimensional array of just the email addresses, that you can directly implode() to produce the comma separated list of email addresses.

    • Like 1
  15. 41 minutes ago, Ivan007 said:

    when I var_dump($stmt) I got the right result but nothing is output at this stage

    and what is the right result?

    if it displayed the sql query string, and not a false or null value, all that means is that the query didn't fail with an error. it doesn't mean that the query matched any rows of data.

  16. edit: repeats advice already given - use the browser's developer console network tab to see what is being sent from the javascript and returned by the server-side code.

    next, your search requires an exact match with existing data. you should use a select/option menu instead.

    also, don't put external, unknown, dynamic values directly into an sql query. use a prepared query instead. you would probably want to switch to the much simpler PDO database extension, since using the mysqli extension with a prepared query is overly complicated and inconsistent.

    don't unconditionally output raw database connection/query errors onto a web page. the connection error contains the database hostname/ip, username, if you are using a password or not, and web server path information. when a hacker intentionally triggers a connection error, by flooding your site with requests that consume all the available connections, the existing connection error handling will give them 2 or all 3 pieces of information they need to connect to your database. instead use exceptions for database statement errors (the latest php versions automatically does this for mysqli connection errors anyways, so your existing connection error handling will never be executed) and in most cases simply let php catch and handle any database exception, where php will use its error related settings to control what happens with the actual error information (database statement errors will 'automatically' get displayed/logged the same as php errors.) this will let you remove any database statement error handling logic you have now, simplifying the code.

    for a query that is expected to match at most one row of data, don't use a loop. just fetch that single row of data.

  17. if you do this -

    2 hours ago, mac_gyver said:

    by pre-fetching the data into an array and indexing/pivoting it by first the station, then the fuel.

    a majority of this code will go away.

    assuming i decoded what you are trying to produce as output. the following should be all the code that you need for the data table section  -

    // pre-fetch the data, indexing/pivoting it using the station, then fuel as indexes
    $data = [];
    while ($row = sqlsrv_fetch_array($result))
    {
    	$data[$row['station']][$row['fuel']] = $row;
    }
    
    // produce the output
    ?>
    <table class = "main">
    <tr class="main"><td></td>
    <?php
    // output the heading
    foreach($fuel as $fu)
    {
    	echo "<td class = 'main'>$fu</td>";
    }
    ?>
    <td class = "main">Atnaujinta</td></tr>
    
    <?php
    // output the data
    foreach($station as $st)
    {
    	echo "<tr><td class='main'><a href='levelStation.php?station=$st'><b>$st</b></a></td>";
    	foreach($fuel as $fu)
    	{
    		if(empty($data[$st][$fu]['vol']))
    		{
    			echo '<td>-</td>';
    		}
    		else
    		{
    			$row = $data[$st][$fu];
    			$class = '';
    			if ($row['fmm']<=100)
    				$class = 'class="low"';
    			if ($row['fmm']>100 AND $row['fmm']<250)
    				$class = 'class="alert"';
    			if ($row['vmm']>=5)
    				$class = 'class="water"';
    			
    			$qs = [];
    			$qs['station'] = $st;
    			$qs['fuel'] = $fu;
    			$qs['type'] = 'all';
    			$qs = http_build_query($qs);
    			
    			echo "<td $class><a href='levelFuel.php?$qs'>{$row['vol']}</a></td>";
    		}
    	}
    	echo "<td>".date_format(date_create($row['date']),"Y-m-d H:i:s")."</td></tr>\n";
    }
    
    // output the totals
    ?>
    <tr><td>Iš viso:</td>
    <?php
    foreach($fuel as $fu)
    {
    	$sum = 0;
    	foreach($station as $st)
    	{
    		$sum += $data[$st][$fu]['vol']??0;
    	}
    	echo "<td class = 'main'>$sum</td>";
    }
    ?>
    <td></td></tr>
    </table>

     

    • Like 1
  18. the code is unconditionally looping over some number of $fuel choices and expects there to be corresponding data in $cellt. IF (a big if) the data in $cellt has gaps in the 1st index, then the code should be testing if $cellt[$k] isset() before referencing it.

    what does adding the the following, after the point where $cellt is being built, show -

    echo '<pre>'; print_r($cellt); echo '</pre>';

    this code is apparently (hard to tell without all the actual code, sample data, and desired output) outputting data for each station (starts a new section when the station value changes) and each fuel type. the code is repetitive, because it must complete the previous section when a new section is detected, and it must also complete the last section at the end. you can greatly simplify all of this by pre-fetching the data into an array and indexing/pivoting it by first the station, then the fuel. to produce the output from the pre-fetched data then just takes a couple of loops, without repetition in the code or extra variables to detect when the station changes.

    you should also use associative index names for the fetched data, so you are not having to keep track of numbers.

    lastly, in the code building the links, the only thing that is different is the class='...' markup. this is the only thing that should be conditional. the rest of the markup is identical and should only exist once in the code - Don't Repeat Yourself (DRY.) build the class='...' markup in a php variable, then use that variable when building the single link. you should also build the query string key/values in an array, then use http_build_query() to produce the query sting for the link.

  19. 10 hours ago, PNewCode said:

    This one has no errors listed but just says "page isn't working"

    you are getting a http 500 error because the code contains a php syntax error. you don't know this because you haven't put the php error related settings into the php.ini on your system, so that ALL php errors will get reported and displayed. when you put these settings into your code, they don't do anything for php syntax errors in that file since your php code never runs in this case to change those settings.

    the approximate point where the syntax error is occurring at can be seen since the color highlighting stops changing, both in your programming editor and in the forum post. if you always build sql query statements in a php variable, it helps prevent syntax errors like this by separating the sql query syntax as much as possible from the php code. you can also help prevent syntax errors like this by using a prepared query (which will also prevent sql special characters in a value from being able to break the sql query syntax) when supplying external,  unknown, dynamic values to the query when they get executed, since you are only putting a simple ? prepared query place-holder into the sql syntax for each value.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.