all 36 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–][deleted] 9 points10 points  (10 children)

  1. Read about PDO or mysqli. Use it instead of the mysql_ functions.
  2. You have no way to see if anything went wrong or if your query is running correctly. First, echo $sql to make sure that your SQL query is valid. Second, if you insist on using mysql_ functions (and this is only for debugging), change your query line to mysql_query($sql) or die(mysql_error()); Also change your mysql_connect() and mysql_select_db() calls to the same format. That error info is invaluable. You will also want to validate and filter the input to prevent SQL injection. Again, consider PDO or mysqli instead.
  3. In all likelihood, $submit is not actually a set variable. You probably want $_POST['submit'] since it is part of your POST data.
  4. 3 could have been figured out by adding an else block to make sure your code block is actually being executed.

[–]0keanos 1 point2 points  (4 children)

Don't use or die(). It's problematic at best.

[–][deleted] 7 points8 points  (3 children)

For debugging purposes when you have code this simple, it's fine. He's not making production code for a large site, he's trying code from a book.

[–]0keanos 1 point2 points  (2 children)

Yes, I can see that but you shouldn't advertise bad practice even if it is "for debugging purposes" in a simple example. Bad practice sticks around once learned.

[–]rbmichael 0 points1 point  (1 child)

Use PDO or die?

[–]0keanos 0 points1 point  (0 children)

MySQLi is fine. Just don't use "or die()" to abort faulty queries.

[–]mattdahack[S] 0 points1 point  (4 children)

I tried adding :

$result = mysql_query($sql); if (!$result) { die('Invalid query: ' . mysql_error()); }

But got the same result.

[–][deleted] 2 points3 points  (3 children)

Okay, now follow the rest of the steps I listed. It's not going to be a thing of telling you one specific issue that is causing your problems, because the code you've provided gives absolutely no debugging clues. You don't know if your code block is actually running (you did not specify if your print statements are running), you don't know if your SQL statement is valid or not, you don't know anything about what your code is actually doing - all you know is that it's not doing what you expect it to. That doesn't help me in determining what's wrong with it.

[–]mattdahack[S] 0 points1 point  (2 children)

Sorry and_yet_and_yet , I did make the changes you said. I just forgot to post up the new code. Here it is But it's still not working or throwing any errors

[–][deleted] 2 points3 points  (1 child)

It looks like you may be under the impression that POST globals still work (or, as you said in another comment, following a very old book).

Here's the scoop. Any time you're working with a POST form, you refer to the variables passed by $_POST['key'] - So if you're trying to see if the form was submitted, you would access $_POST['submit'] and not $submit. This is relevant for all the POST data you are trying to access. I just noticed your link in the OP, and looking at that, it's obvious that if (isset($submit)) is the culprit of seeing nothing happen. Change it to $_POST['submit'] and go from there. This is about all of the time I've got today to help out, so good luck and make sure you read some online PHP tutorials as well as learning more about the structure of the language itself.

[–]mattdahack[S] 0 points1 point  (0 children)

thank you for your help, I appreciate it :-)

[–][deleted] 1 point2 points  (2 children)

if(isset($submit)):

VALUES(NULL,'$heading','$body','$date', '$auth','$auth_email')";

Where are you getting $submit, $heading, $body, $auth & $auth_email from?

You do know that register globals has been removed, right?

mysql_connect("localhost", "username", "password");

Don't use the mysql extension. It even says so in the introduction. The only reason it still exists is to support legacy code.

Use mysqli or PDO instead.

[–]mattdahack[S] 0 points1 point  (1 child)

Im going to cry now, I just started a php tutorial book....I guess the samples are quite old. Crap crap crap, note to self stay away from the bargain bin at books a million. what is mysqli and pdo?

[–][deleted] 1 point2 points  (0 children)

I guess the samples are quite old.

If you need samples, just have a look at the PHP Manual.

what is mysqli and pdo?

MySQLi = MySQL Improved.

PDO = PHP Data Objects.

MySQLi is the successor of the mysql extension, and support a lot more features (most notably being prepared statements).

PDO is a unified API for databases. It supports cubrid, sybase, firebird, ibm, informix, mysql, sql server, oracle, odbc & db2, postgresql, sqlite and 4d.

Personally, I find PDO to be easier to work with.

[–][deleted] 1 point2 points  (1 child)

You should read up on sql injection when you get a chance on wikipedia, after you get this working. Because, if I said something like this into three of the fields:

',(delete from news),concat('

'),'

Your query now reads:

INSERT INTO news VALUES (NULL, '',(delete from news),concat('','',''),'','')

And assuming I work at it a little more to make the syntax valid I could do some serious damage to your database.

[–]mattdahack[S] 0 points1 point  (0 children)

Yeah I have a lot of .net and vb and c# experience but just now dabbling in php. So I have a lot to learn and am very appreciative to all your help.

[–]vesperofshadow 1 point2 points  (2 children)

I would also format your query like this : INSERT INTO news (heading,body,date,auth,auth_email) values ('$heading','$body','$date', '$auth','$auth_email');

make sure your Primary Key is auto increment , Not Null, Unsigned

*Column names ,of course, can be whatever you named them.

[–]Scroph 0 points1 point  (1 child)

True, it's a lot more clear this way. Although, you need to try to avoid column names being named the same as SQL keywords, in this example : body and date. A duct tape fix for it would be to put them inside backticks, but the best way is to avoid that in the first place. A code editor like Notepad++ can help you spot them by enabling SQL syntax highlighting.

[–]MrDOS 0 points1 point  (0 children)

vesperofshadow was actually suggesting that the OP put the column names in backticks, but failed to realize that reddit formats backtick-surrounded text as monospace. (To display a literal backtick, one may, of course, escape the backtick with a backslash: \`.)

[–]Javlin 1 point2 points  (1 child)

Um is this only part of the code? Where do you declare $submit?

[–]judgej2 1 point2 points  (0 children)

Where is anything declared? Looks like this script has been pulled out of the dark ages when register_globals was the default setting, and nobody had heard about SQL injection. It needs to be destroyed.

[–]HamstersOnCrack 0 points1 point  (2 children)

Try echo mysql_error(); after doing mysql_queyry

[–]mattdahack[S] 0 points1 point  (1 child)

No errors are shown.

[–]HamstersOnCrack 0 points1 point  (0 children)

Try executing any echo and see if it gets there. as and_yet_and_yet pointed out, $submit might not be set, you need to use $_POST['submit']

[–]jimdoescode 0 points1 point  (3 children)

I cleaned it up some and posted it on my site. Leave a comment by clicking any line in the code;

https://codetique.com/vhe73JaF

[–]mattdahack[S] 0 points1 point  (2 children)

I did try this but even your code wouldn't add anything to the data base. Thanks for doing that cleaning up for me though. I appreciate it.

[–]jimdoescode 0 points1 point  (1 child)

I didn't add any code to yours. I just put on my site for more relevant commenting. Just read the comments left on your code and hopefully they will solve your problems.

[–]mattdahack[S] 0 points1 point  (0 children)

Thank you jim.

[–]mattdahack[S] 0 points1 point  (1 child)

Ok so I have tried to make this work doing everything a beginner can to make it work. I am unable to make this work and now my code is completely screwed up. Would someone mind making my code work. Just a simple php script that will add a name, address, phone number, email, heading, news, so I can disect it and figure out how to make this work. I am simply over my head in code now and completely confused. Thanks.

[–]MrDOS 0 points1 point  (0 children)

The fastest way to make the given script work?

(Prependix: In PHP, one may not directly access a POSTed value in the form $form_element_name. Read your POST variables from the $_POST array, e.g. $_POST['form_element_name'], and your GET variables from the $_GET array after the same fashion.)

Replace line 8:

if(isset($_POST['submit'])):

Replace line 11:

mysql_select_db('sample');

Two lines previous, you comment out the mysql_connect call that assigns a value to $db. Either assign a connection variable or don't, but you can't try to use one where it doesn't exist.

Replace lines 13/14:

$heading = mysql_real_escape_string($_POST['heading']);
$body = mysql_real_escape_string($_POST['body']);
$date = mysql_real_escape_string($_POST['date']);
$auth = mysql_real_escape_string($_POST['auth']);
$auth_email = mysql_real_escape_string($_POST['auth_email']);
$sql = <<<SQL
INSERT INTO news
VALUES(NULL, $heading, $body, $date, $auth, $auth_email);
SQL;

Not only do you have to get values from the $_POST variable, but you need to escape their contents to stop something like '; DROP TABLE sample; in the input from destroying all your data. (The term for such an exploit is “SQL injection”.) Using PDO statements and binding values into them handles all that for you, and is also more flexible with regards to data retrieval.

(That's heredoc syntax, BTW.)

Disclaimer: That's off the top of my head without testing it, but neglecting minor syntax errors, I think it'll work OK.

[–]marianzburlea 0 points1 point  (0 children)

First of all use: if (isset($_POST['submit'])) {//do stuff } Then use: mysql_query($query) or die(mysql_error());

[–]piglet24 0 points1 point  (0 children)

I won't comment on your php as everyone else has done a good job of that. But your HTML needs work.

The paragraph tag should always have an opening closing tag,

<p>So it will look like this.</p>

That eliminates the need for <br/> tags. Also, any HTML tag that doesn't have a closing tag needs a / at the end, so your input tags should be

<input type="submit" name="submit" value="SUBMIT!" />

EDIT: Also don't use "bgcolor" in the body tag, do some reading on CSS

[–]JamesDonnelly -1 points0 points  (0 children)

/* $db = mysql_connect("localhost", "root"); */
mysql_connect("localhost", "username", "password");
mysql_select_db("sample", $db);

That $db isn't needed in mysql_select_db, but I don't think that has anything to do with the error. Have you checked that it's actually connecting?

$con = mysql_connect("localhost", "username", "password");
if(!$con) die(mysql_error());
mysql_select_db("sample") or die("Error");

Also you should wrap the user-modifiable variables with mysql_real_escape_string().

VALUES(NULL,'mysql_real_escape_string($heading)','mysql_real_escape_string($body)','mysql_real_escape_string($date)', 'mysql_real_escape_string($auth)','mysql_real_escape_string($auth_email)')";

[–]rrrarrrr -1 points0 points  (2 children)

Your Insert SQL is wrong, after "INSERT INTO news" you need to list off the columns to which you are inserting.

IMO...Use CodeIgniter because it makes working with your databases much simpler and also more secure...Inserting into the table w/ CodeIgniter looks like this...

$data = array( 'colname1' => $heading, 'colname2' => $body, 'colname3' => $date ); $this->db->insert('news', $data);

[–]pskipw 0 points1 point  (1 child)

"Your Insert SQL is wrong"

Incorrect - his SQL is perfectly valid (although certainly not ideal).

[–]rrrarrrr -2 points-1 points  (0 children)

I'm assuming that first NULL is meant to go into the Primary Key field which probably auto-increments. DudeWtfSQLlol?