Hello and welcome to our community! Is this your first visit?
Register
Enjoy an ad free experience by logging in. Not a member yet? Register.
Results 1 to 4 of 4
  1. #1
    New Coder
    Join Date
    Feb 2007
    Location
    Norway
    Posts
    63
    Thanks
    0
    Thanked 0 Times in 0 Posts

    Query to match user ID and username

    I have a script that lets users post results from a montly competition. In the input form a select drop-down list is populated with names from a table called userdata. The value of the select list is the ID of the user. The script works fine, except for one issue: when I post the results to a table called competition I'm unable to post the username. I've tried to create a variable for the name by matching the user_id from the form with the forum_user_id from the userdata table, but with no luck. Here is my attempt to create a variable, but it doesn't work:

    PHP Code:
    $name mysqli_query($link"SELECT user_name FROM userdata WHERE user_id='".$forum_user_id."'"); 
    Can anyone suggest what I' doing wrong? The entire code is posted below.

    Input form:

    PHP Code:
    <form action="competition-register.php" method="post" target="POPUPW"    onsubmit="POPUPW = window.open('about:blank','POPUPW',   'width=800,height=200');">
    <table width="200" border="1">
      <tr>
        <td>User name</td>
        <td><?php
    include(dirname(__FILE__) . "mysqliconnect.inc.php"); 

    if (
    $result mysqli_query($link"SELECT user_id, user_name, logins FROM userdata WHERE logins > '0' ORDER BY user_name")) {
        
        echo 
    "<select name='id'>";
    while(
    $row mysqli_fetch_array($result)) {
        
    echo 
    "<option value='" $row['user_id'] ."'>" $row['user_name'] ."</option>";

    }
    echo 
    "</select>";


    mysqli_free_result($result);

    mysqli_close($link);
    }

    ?></td>
      </tr>
        <td>Score</td>
        <td><input type="text" name="score" onkeypress='return event.charCode >= 48 && event.charCode <= 57'>
    </input></td>
      </tr>
      <tr>
        <td><input type="submit" value="Register" /></td>
        <td>&nbsp;</td>
      </tr>
    </table>
    </form>
    The registration script:

    PHP Code:
    <?php

    $link 
    mysqli_connect("xxx""xxx""xxx""xxx");

    mysqli_set_charset($link,"utf8");

    if(
    $link === false){

        die(
    "ERROR: Could not connect. " mysqli_connect_error());
    }


    $month date('n');
    $forum_user_id mysqli_real_escape_string($link$_POST['id']);
    $score mysqli_real_escape_string($link$_POST['score']);
    $name mysqli_query($link"SELECT user_name FROM userdata WHERE user_id='".$forum_user_id."'");


    $query mysqli_query($link"SELECT * FROM competition WHERE forum_user_id='".$forum_user_id."' AND month='".$month."'");

    if(
    mysqli_num_rows($query) > 0){

        echo 
    "</p> You have already posted your score in this months comeptition.</p>"; }

    else
        
    {

    $sql="INSERT INTO competition (forum_user_id, name, month, score)

    VALUES('
    $forum_user_id', '$name', '$month', '$score')"; } 


    if(
    mysqli_query($link$sql)){



        echo 
    "<h1>Thanks for registering!</h1>    ";

    } else{

        echo 
    "ERROR: Unable to execute $sql. " mysqli_error($link);
    }

    mysqli_close($link);

    ?>

  2. #2
    Senior Coder CFMaBiSmAd's Avatar
    Join Date
    Oct 2006
    Location
    Denver, Colorado USA
    Posts
    3,996
    Thanks
    3
    Thanked 483 Times in 472 Posts
    You have to fetch data after you run a query, like you are doing in the first piece of code with the mysqli_fetch_array() statement.

    The logic of what you are doing makes no sense. The only person who should be able to submit his/her own data is that person. You should A) require a logged in user, and B) get the user id for the current logged in user from a session variable. This will eliminate a lot of your code.

    There are also other problems with your design. You should not repeat data between tables. The name is stored in the userdata table. You would not store it in the competition table. You would just store the user id in the competition table. Months are not unique. Once you have more than a year worth of data, you won't be able to distinguish which year the data corresponds to. You must store data using both the year and the month.

    Next, you should then just run the INSERT query and check if it inserted the row. You don't need to select data first, before trying to insert it. Your database table needs to enforce uniqueness. You would define the user id and year/month columns as a single composite unique index. If the user tries to insert data that already exists, the query will return a unique index violation error message/error number. You can test for this error number (I think it's 1062) and output the 'You have already posted your score...' message.
    Finding out HOW to do something is called research, i.e. keep searching until you find the answer. After you attempt to do something and cannot solve a problem with it yourself, would be when you ask others for help.

  3. Users who have thanked CFMaBiSmAd for this post:

    benanamen (01-11-2017)

  4. #3
    Senior Coder benanamen's Avatar
    Join Date
    Oct 2015
    Posts
    1,155
    Thanks
    2
    Thanked 122 Times in 118 Posts
    Excellent answer @CFMaBiSmAd!
    To save time, lets just assume I am almost never wrong.

    The XY Problem
    The XY problem is asking about your attempted solution (X) rather than your actual problem (Y). This leads to enormous amounts of wasted time and energy, both on the part of people asking for help, and on the part of those providing help.

    "This text has been encoded with ROT26. If you can read this you must have found a backdoor. Congratulations!"

  5. #4
    Senior Coder deathshadow's Avatar
    Join Date
    Feb 2016
    Location
    Keene, NH
    Posts
    2,058
    Thanks
    2
    Thanked 297 Times in 287 Posts
    In addition to the server-side woes @CFMaBiSmAd outlined, there are other issues like FAILING TO EVEN BOTHER using mysql_ properly... and properly means if you are going to plug values into your query from variables, you use prepare/execute and not the outdated outmoded idiotic NONSENSE of _real_escape_string.

    To be frank, mysqli_real_escape_string should NEVER have even been created in the first place, an is yet another reason I say that mysqli should be either neutered or just outright junked in favor of PDO. It molly-coddles along the folks who refuse to acknowledge WHY the old mysql_ functions were trashed in the first place.

    That's why this:
    Code:
    $forum_user_id = mysqli_real_escape_string($link, $_POST['id']); 
    $name = mysqli_query($link, "SELECT user_name FROM userdata WHERE user_id='".$forum_user_id."'");
    Should read something more like this:

    Code:
    $stmt = $link->prepare('SELECT user_name FROM userdata WHERE user_id = ?');
    $stmt->bindParam('i', $_POST['id']);
    $stmt->execute();
    Variables as of about a DECADE ago have ZERO business in your query strings in normal production sites/systems. The only time you should even consider doing so is for table names or field names in the case of creating a tool like phpMyAdmin. Normal codebases for day to day stuff shouldn't even be CONSIDERING putting variables into the query string! Separation of data from query provides your sanitation (no more escape_string rubbish), allows for POEM (prepare once, execute mostly), and is in most cases more efficient. That's part of why mysql_ was deprecated and why if you just use the same thinking and methodology with mysqli as you would have with mysql_, you're basically undoing everything we've been told since PHP 5.1 dropped and COMPLETELY missing the point!

    Also comma delimits instead of string addition and using singles instead of doubles is a hair faster and uses less memory when it comes to echo... and I'd suggest ripping off the training wheels that are the pointless code bloat procedural wrappers and just learn to use the object oriented method -- since those wrappers are LITERALLY just that, wrappers/code bloat meant as crutches for the feeble minded fools who REFUSE to let go of the old mysql_ mindset and methodology.

    As I joked elsewhere, OH NOES, I HAZIES TWOS USEDEDS TISS:
    Code:
    $link->execute();
    INSTEADZIES UF TISS?!?
    Code:
    mysqli_execute($link);
    Cry me a huffing river.

    Your client-side is no prize pig either.

    Tables for layout is something of a no-no particularly when it is plainly clear this is NOT tabular data. Where are your LABEL tags? Your FIELDSET? You're using scripting in a manner that doesn't gracefully degrade scripting off / disabled / blocked. (an increasingly common state of affairs with bandwidth caps looming over much of the US and a reality in places like Canada and Australia)... there's been no such thing as target="" or border="" attributes SINCE 1998, window.open is outright blocked on massive numbers of systems by either the browser itself, or "make JS behave" browser extensions like NoScript or Ghostery. (see why the PM system on these forums is broken when it tries to pull that stunt to open new windows automatically)

    ... and even if that were workable instead of being a laundry list of how NOT to code a form, the scripting should hook the markup instead of pissing on the markup with the outmoded onevent attributes like onsubmit="" or onkeypress="" which will magically go away and cease to exist if you decide later on to try and implement the CSP (content security policy).

    ... and there is not nor ever has been such a thing as </input>, nor do you start a paragraph with </p>. That's just outright gibberish.

    To that end, your first snippet should probably read something more like this:
    Code:
    <?php
    
    echo '
    	<form action="competition-register.php" method="post"> 
    		<fieldset>
    			<label for="register_name">User name</label>';
    			
    include(dirname(__FILE__) . "mysqliconnect.inc.php");  
    
    if ($result = $link->query('
    	SELECT user_id, user_name, logins
    	FROM userdata
    	WHERE logins > 0
    	ORDER BY user_name
    ')) {
    
    	echo '
    			<select name="id" id="register_name">';
    		
    	while ($row = $result->fetch_array()) echo '
    				<option value="', $row['user_id'], '">', $row['user_name'], '</option>';
    		
    	echo '
    			</select>';
    
    } 
    mysqli_free_result($result); 
    mysqli_close($link); 
    
    echo '
    			<br>
    			<label for="register_score">Score</label> 
    			<input type="number" name="score" id="register_score"> 
    			<button>Register</button>
    		</fieldset>
    	</form>';
    ... any scripting should hook the elements instead of slopping the attributes into the HTML or let the new HTML 5 attributes do their jobs... the markup being more concerned with SEMANTICS instead of "what it looks like" meaning a form with an actual fieldset, labels to create the semantic relationship between the text and their corresponding input/select/textarea, and I used a button since the default behavior of button is submit, there's no need to put classes on it as it is the only button in the code. Any appearance you want, that's CSS' job!

    Hence your latter snippet should likely be more like this:

    Code:
    <?php 
    $link = new mysqli("xxx", "xxx", "xxx", "xxx");
    if ($link->connect_error) {
    	die('ERROR: Could not connect. ' . $link->connect_error); 
    } 
    $link->set_charset('utf8');
    
    $stmt = $link->prepare('
    	SELECT user_name
    	FROM userdata
    	WHERE user_id = ?
    ');
    $Stmt->bindParam('i', $_POST['id']);
    $stmt->execute();
    $stmt->bindResult($username);
    
    if ($stmt->fetch()) {
    
    	$stmt = $link->prepare('
    		SELECT *
    		FROM competition
    		WHERE forum_user_id = ?
    		  AND month = MONTH(CURRENT_DATE())
    	');
    	$stmt->bindParam('i', $_POST['id']);
    	$stmt->execute();
    	
    	if ($stmt->fetch()) echo '
    			<p>
    				You have already posted your score in this months comeptition.
    			</p>';
    			
    	else {
    		$stmt = $link->prepare('
    			INSERT INTO competition (
    				forum_user_id, name, month, score
    			) VALUES (
    				?, ?, MONTH(CURRENT_DATE()), ?
    			)
    		');
    		$stmt->bindParam('iss', $_POST['id'], $username, $_POST['score']);
    		$stmt->execute();
    		echo '
    			<p>
    				Thanks for registering!
    			</p>';
    	}
    	
    } else echo '
    			<p>
    				User ID not found.
    			</p>';
    
    $link->close();
    Though there are more efficient ways of handling that. There may also be typo's in the above -- untested... and mysqli_ isn't really my bag. Again it molly-coddles the folks who can't transition their mindset out of the previous decade or two, unlike PDO which pretty well forces you to adapt.

    Oh, I was also doubtful that "thanks for registering" was the heading that everything on every page of your website is a subsection of -- aka what a H1 means unless you're using HTML 5's idiotic pointlessly redundant <section> or <article> tags. Remember, H1..H6 do not MEAN "text in different sizes and weights", they MEAN the start of sections and subsections of the page -- an H2 being the start of a subsection of the H1, a H3 being the start of a subsection of the H2 preceeding it and so forth. JUST as a HR does not mean "draw a line across the screen" it means "the start of a subsection of the page where heading text is unwanted or unwarranted".

    If you choose your tags based on just their default appearance, you are likely choosing all the wrong tags for all the wrong reasons.
    Last edited by deathshadow; 01-11-2017 at 08:28 PM.
    I would rather have questions that can't be answered, than answers that can't be questioned.
    http://www.cutcodedown.com


 

Tags for this Thread

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •