MjM8082 Posted August 8, 2012 Share Posted August 8, 2012 When I click on the post I want to update it brings me to the next page to a blank form. I'm trying to make it so the form has all the information from the database in it so the user and change it and then click update. For some reason my form just keeps showing up blank and wont display the database data. Here is the update page that includes the form... <?php include "connect_to_mysql.php"; if (!isset($_POST['submit'])) { $post_id = (int)$_GET['post_id']; $sql = mysql_query ("SELECT * FROM posts WHERE post_id = '$post_id'"); $results = mysql_query($sql); $row = mysql_fetch_array($results); } ?> <form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post"> Title<input type="text" name="title" value="<?php echo $row['title']; ?>" /><br/> Content<input type="text" name="content" value="<?php echo $row['content']; ?>" /><br/> <input type="hidden" name="id" value="<?php $post_id; ?>" /> <input type="submit" name="submit" value="Edit" /> </form> </font> Quote Link to comment Share on other sites More sharing options...
requinix Posted August 8, 2012 Share Posted August 8, 2012 Go into your php.ini and set error_reporting = -1 display_errors = on Then restart the web server and try the page again. You'll see (at least) two errors: one for mysql_query(), one for mysql_fetch_array(). Those should clue you in on what's wrong. The other bunch of errors after those two are because of them so don't worry about trying to fix those. Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 8, 2012 Share Posted August 8, 2012 1. Do not use PHP_SELF, because it makes your page vulnerable to XSS attacks. Instead, use an absolute URL for the action attribute, leave it blank, or omit it from the form tag. 2. What are you doing when the form is submitted? Currently all that would happen is a bunch of undefined variable ($row) errors. 3. Don't use mysql_fetch_array() when you're not using numerical indexes, it is a waste of memory. Use mysql_fetch_assoc() instead. Quote Link to comment Share on other sites More sharing options...
MjM8082 Posted August 8, 2012 Author Share Posted August 8, 2012 Okay I will make those changes. Right now I'm not worrying about it updating in the database, first I just want to information to display into and fill the form from the database. Quote Link to comment Share on other sites More sharing options...
Christian F. Posted August 8, 2012 Share Posted August 8, 2012 While I do agree that $_SERVER['PHP_SELF' is indeed unnecessary in this case, as you can just the action parameter blank, you still might find this thread interesting. http://forums.phpfreaks.com/index.php?topic=363628.0 As for scootstah's second point, this is where you really need to take a second look at. Check the PHP manual for mysql_query () as well, as it appears you have some confusion about the usage of this function. His third point, however, I don't agree with. This is micro-optimization and should not be a concern at all, in fact it may be detrimental in some cases. At most it would save around 500 bytes, for the few nanoseconds this code would run. Not worth the effort of changing routines, or going back to change. Only do micro-optimizations like these if there is an actual need for it, and that you will only know after thoroughly profiling the code. Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 8, 2012 Share Posted August 8, 2012 At most it would save around 500 bytes How do you figure that? You are fetching the same data twice. array( 0 => 'some title', 'title' => 'some title', 1 => 'some content', 'content' => 'some content' ) Quote Link to comment Share on other sites More sharing options...
Christian F. Posted August 8, 2012 Share Posted August 8, 2012 That's true, but considering the max length of a VARCHAR field (times 2 of those, unless there's fields the OP doesn't use), plus a int for the primary key, You end up at 501 bytes of data, plus a little overhead for the indices, and you're at a maximum of a little over 500 bytes. (Probably less than 510.) He retrieves a single row, after all, and once the script is done parsing PHP will clean the data (or mark it for cleaning) from the memory. Even if he did retrieve multiple rows, they would overwrite the previous content anyway. In other words: Quite insignificant. Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 8, 2012 Share Posted August 8, 2012 He is selecting all columns from the table, though. Who knows how much data that is. Also, a varchar can hold 65,535 bytes, which is the maximum row size. And just because this is a case where the difference might be negligible, it is still bad practice. What is the point to holding the extra data if you're not even going to use it? Quote Link to comment Share on other sites More sharing options...
MjM8082 Posted August 8, 2012 Author Share Posted August 8, 2012 I got the text box problem to work.. I'm working on my update statement now. Here is my code <form action="" method="post"> Title<input type="text" name="title" value="<?php echo $row['title']; ?>" /><br/></br> Content</br><textarea name="content" cols="50" rows="10"><?php echo $row['content']; ?></textarea><br/> <input type="hidden" name="post_id" value="<?php $post_id; ?>" /> <input type="submit" name="submit" value="Edit" /> </form> </font> </p> <?php if (isset($_POST['submit'])) { $post_id = (int)$_GET['post_id']; $sql = ("UPDATE posts SET 'title'='$_POST[title]', content='$_POST[content]' WHERE post_id = '$post_id'"); echo 'Post has been updated!</a><br /></br>'; } ?> Having trouble getting it to work. Is the update statement similar to a INSERT statement? Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 9, 2012 Share Posted August 9, 2012 1. You have improper array and string syntax. 2. You can't put quotes around MySQL field names. They require either back ticks (`), or nothing at all (unless they have a space in the name) 3. post_id does not need quotes around the value, because it is an integer 4. You are not calling mysql_query(). 5. You are not sanitizing the input data, which could lead to SQL injection $title = mysql_real_escape_string($_POST['title']); $content = mysql_real_escape_stirng($_POST['content']); $sql = "UPDATE posts SET title='$title', content='$content' WHERE post_id = $post_id"; mysql_query($sql); Quote Link to comment Share on other sites More sharing options...
MjM8082 Posted August 9, 2012 Author Share Posted August 9, 2012 I've been messing around with the things you mentioned but I seem to still be getting a error because nothing is changing in the database when I hit update. Quote Link to comment Share on other sites More sharing options...
Christian F. Posted August 9, 2012 Share Posted August 9, 2012 MjM8082: Please post the updated code, so that we can see exactly what you've done. He is selecting all columns from the table, though. Who knows how much data that is. That's an argument for selecting only the fields that he needs, which I agree that he should be doing instead of using "*". Though, that's not only for performance reasons, but also for readability. And just because this is a case where the difference might be negligible, it is still bad practice. What is the point to holding the extra data if you're not even going to use it? My point wasn't related to whether or not the data was used, but the fact that micro-optimization is bad and unnecessary. Readability is a far greater concern, and if the author prefers to use fetch_array () then he should do so. The only time micro-optimization should be used is after you've profiled the code, have proved that it will give a noticeable effect, and you know what you're doing. Someone learning a programming language doesn't fill the last criteria, and none of us have actually profiled the code, so the first two aren't filled either. That said, this is really off-topic for this thread, so I won't discuss it more in here. If someone is interested in continuing it, I'll be more than happy to join a thread dedicated to this topic. Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 9, 2012 Share Posted August 9, 2012 He is selecting all columns from the table, though. Who knows how much data that is. That's an argument for selecting only the fields that he needs, which I agree that he should be doing instead of using "*". Though, that's not only for performance reasons, but also for readability. Well, you said that the difference would only be ~500bytes. I'm saying that you cannot possibly know that, because you don't know what data the table holds. If he is selecting all of the rows, and then you double that, that could be a pretty big difference. My point wasn't related to whether or not the data was used, but the fact that micro-optimization is bad and unnecessary. Readability is a far greater concern, and if the author prefers to use fetch_array () then he should do so. I agree that micro-optimization is not really a big concern. However, this is not micro-optimization, it has (can have) a pretty significant impact on memory usage. And this has absolutely nothing to do with readability. This is how the three mysql_fetch functions (fetch_assoc, fetch_array, fetch_row) work: data set: id | title | content 1 | some title | some content mysql_fetch_assoc(): array( 'id' => 1, 'title' => 'some title', 'content' => 'some content' ) mysql_fetch_row(): array( 0 => 1, 1 => 'some title', 2 => 'some content' ) mysql_fetch_array(): array( 0 => 1, 'id' => 1, 1 => 'some title', 'title' => 'some title', 2 => 'some content', 'content' => 'some content' ) So, as you can see, mysql_fetch_array() is simply combining mysql_fetch_row() and mysql_fetch_assoc(). Why would you ever want to do that, especially when you are only using string keys? In some cases, I could see the need for numerical indexes. Say, something like this: $result = mysql_query("SELECT COUNT(*) FROM table"); list($num) = mysql_fetch_row($result); In which case, you would simply use mysql_fetch_row(). But, I don't know why you would ever need both forms of data at the same time. It is inefficient, and should be avoided. Quote Link to comment Share on other sites More sharing options...
MjM8082 Posted August 9, 2012 Author Share Posted August 9, 2012 Here is my code exactly how it looks in my program. <form action="" method="post"> Title<input type="text" name="title" value="<?php echo $row['title']; ?>" /><br/></br> Content</br><textarea name="content" cols="50" rows="10"><?php echo $row['content']; ?></textarea><br/> <input type="hidden" name="post_id" value="<?php $_GET['post_id'] ?>" /> <input type="submit" name="submit" value="Edit" /> <input type="button" value="Back" ONCLICK="window.location.href='edit_post.php'"/> </form> </font> </p> <?php include "connect_to_mysql.php"; if (isset($_POST['submit'])) { $post_id = isset($_POST['post_id']) $_POST['post_id'] : ''; $title = isset($_POST['title']) mysql_real_escape_string($_POST['title']) : ''; $content = isset($_POST['content']) mysql_real_escape_string($_POST['content']) : ''; if ( $post_id > 0 ){ $sql = ("UPDATE posts SET title='$title', content='$content' WHERE post_id = $post_id"); mysql_query($sql); echo 'Post has been updated!</a><br /></br>'; } ?> Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 9, 2012 Share Posted August 9, 2012 You really need to turn on error reporting. Go into your php.ini and set error_reporting = -1 display_errors = on Then restart the web server and try the page again. Quote Link to comment Share on other sites More sharing options...
Christian F. Posted August 9, 2012 Share Posted August 9, 2012 I've cleaned up the code, added output escaping, and left some hints to what you need to do. <form action="" method="post"> <fieldset> <label for="title_input">Title</label> <input id="title_input" type="text" name="title" value="<?php echo htmlspecialchars ($row['title']); ?>" /> <label for="content_area">Content</label> <textarea id="content_area" name="content" cols="50" rows="10"><?php echo htmlpecialchars ($row['content']); ?></textarea> <input type="hidden" name="post_id" value="<?php intval ($_GET['post_id']); ?>" /> <input type="submit" name="submit" value="Edit" /> </fieldset> </form> <?php include "connect_to_mysql.php"; if (isset ($_POST['submit'])) { $post_id = isset ($_POST['post_id']) ? intval ($_POST['post_id']) : ''; // TODO: Write the two validate functions. $title = isset ($_POST['title']) ? validateTitle ($_POST['title']) : ''; $content = isset ($_POST['content']) ? validateContent ($_POST['content']) : ''; // CF: Altered the check to require a title and content as well. if ($post_id > 0 && $title && $content) { // CF: Using sprintf () and mysql_real_escape_string () to escape output to database. $sql = "UPDATE `posts` SET `title` = '%s', `content` = '%s' WHERE `post_id` = %d"; $res = mysql_query (sprintf ($sql, mysql_real_escape_string ($title), mysql_real_escape_string ($content), $post_id)); // Check if the query failed or succeeded, and show the correct message. if ($res) { echo '<p class="success">Post has been updated!</a></p>'; } else { echo '<p class="error">Post could not be saved due to SQL error.</p>'; die (mysql_error ()); // Remove this line when done with development. } } } I cannot see anything directly wrong in the code, so it should work. Thus I recommend that you do what scootstah wrote, and see if that sheds some light on the issue. Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 9, 2012 Share Posted August 9, 2012 I cannot see anything directly wrong in the code, so it should work. The problem was his ternary statements. $post_id = isset($_POST['post_id']) $_POST['post_id'] : ''; $title = isset($_POST['title']) mysql_real_escape_string($_POST['title']) : ''; $content = isset($_POST['content']) mysql_real_escape_string($_POST['content']) : ''; Quote Link to comment Share on other sites More sharing options...
Christian F. Posted August 9, 2012 Share Posted August 9, 2012 Gah.... You're right. I was correcting them on auto-pilot, before looking over the code. >< Sorry about that. Quote Link to comment Share on other sites More sharing options...
MjM8082 Posted August 10, 2012 Author Share Posted August 10, 2012 I didn't need like basically anything you guys mentioned... I got it working with this code... include "connect_to_mysql.php"; if (isset($_POST['submit'])) { $title = $_POST['title']; $content = $_POST['content']; $post_id = $_GET['post_id']; $sql = "UPDATE posts SET title ='". $title ."', content ='". $content ."'WHERE post_id = $post_id"; mysql_query($sql); I was just looking for basic code like this from the start, you guys made it seem so confusing when it was just missing a few "" here and there. Quote Link to comment Share on other sites More sharing options...
MMDE Posted August 10, 2012 Share Posted August 10, 2012 I didn't need like basically anything you guys mentioned... I got it working with this code... include "connect_to_mysql.php"; if (isset($_POST['submit'])) { $title = $_POST['title']; $content = $_POST['content']; $post_id = $_GET['post_id']; $sql = "UPDATE posts SET title ='". $title ."', content ='". $content ."'WHERE post_id = $post_id"; mysql_query($sql); I was just looking for basic code like this from the start, you guys made it seem so confusing when it was just missing a few "" here and there. Remember to sanitize the user input. Quote Link to comment Share on other sites More sharing options...
scootstah Posted August 10, 2012 Share Posted August 10, 2012 I didn't need like basically anything you guys mentioned... I got it working with this code... include "connect_to_mysql.php"; if (isset($_POST['submit'])) { $title = $_POST['title']; $content = $_POST['content']; $post_id = $_GET['post_id']; $sql = "UPDATE posts SET title ='". $title ."', content ='". $content ."'WHERE post_id = $post_id"; mysql_query($sql); I was just looking for basic code like this from the start, you guys made it seem so confusing when it was just missing a few "" here and there. Except your code is now, again, vulnerable to SQL injection and will probably not work if all of the expected data doesn't exist (like the post_id). Quote Link to comment Share on other sites More sharing options...
jazzman1 Posted August 10, 2012 Share Posted August 10, 2012 Except your code is now, again, vulnerable to SQL injection and will probably not work if all of the expected data doesn't exist (like the post_id). Why the posters skipped your answers ? I don't really understand this, in case they spoke and read English just fine 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.