Anyone think this is a good script? (for adding hyperlinks to a Links page)

samsunnyuk

Hi I have written a script where the user submits a website URL, website name and (optional) description, which is then added to a text file and displayed inline on the Links page of a website.

So basically this script is to add hyperlinks to a page...

Test the script out here (just add any link). Do you think this is a good system??

Here is my code:

[FONT=Courier New]<body>

<form method="POST" action="addlink.php">
<p>Website Address:<br>
<input type="text" name="hyperlink" value="http://"><br>
Website Name:<br>
<input type="text" name="sitename"><br>
Description (optional):<br>
<input type="text" name="description"></p>

<p><input type="submit" value="Add This Link!">
<input type="reset" value="Reset!"></p>

</form>

<?php

$file = fopen ( "links.txt" , "a" );

if ($POST[hyperlink] && $POST[sitename] > null)
{
fwrite ( $file , "<p><a href=\"$POST[hyperlink]\" target=\"_blank\">$POST[sitename]</a> - $POST[description]</p>\r\n" );
echo "You have added the following link: <a href=\"$POST[hyperlink]\" target=\"_blank\">$POST[sitename]</a> - $POST[description]<br>
Click <a href=\"links.php\">here</a> to visit the Links page";
}

</body>[/COLOR][/FONT]

AND the follwing code is used to show links.txt on the Links page:

[FONT=Courier New]<?php
readfile ("links.txt")
?>[/FONT]

thorpe

Sorry... without being to harsh. A few things. Firstly, alpha numeric array keys should be surrounded in quotes. eg;

foo['bar'];

Not...

foo[bar];

Secondly.... this line.

if ($_POST[hyperlink] && $_POST[sitename] > null)

doesn't make a great deal of sense. NULL is nothing (the absense of a value). So $POST[sitename] can never be bigger than NULL.
This would be more appropriate.

if (isset($_POST['hyperlink']) && isset($_POST['sitename']))

You never check to see if your call to fopen works.

if ($file = fopen ( "links.txt" , "a" )) {
  // process in here.
}

And finally... why use readfile? [man]include[/man] would be more appropriate.

include "links.txt";

samsunnyuk

Thanks for those pointers..

-I tried using "isset" instead of "> null" but i had trouble with it (it would still fwrite if one text field was filled but not the other) I'll try it again though with your code

EDIT: this line doesnt work by the way:

if (isset($POST['hyperlink']) && isset($POST['sitename']))

It lets you post even when the fields are blank, the original code required the fields to be filled..

-I'll add the fopen check
-and I'll use "include" instead of "readfile" -WORKED
-and sort out the numeric array keys.. -DIDNT WORK

Im new to PHP by the way and this is my first proper script!

thorpe

Sorry, I wasn't aware you where checking to see if these variables actually contained a value. Try this instead...

if (!empty($_POST['hyperlink']) && !empty($_POST['sitename']))

samsunnyuk

oh, Cool that works thanks. is it unprofessional to use this then:

[FONT=Courier New]if ($POST[hyperlink] && $POST[sitename] > null)[/FONT]

Also how do i do the fopen check?

Weedpacket

It is unprofessional, for the reasons thorpe gave: it's sloppy grammar. It would be like going to a job interview wearing a beanie with a spinning propellor on it and saying "'Sup?"

To check whether the fopen() succeeded, use code that like thorpe gave (and I'd follow it by an

else
{ //...I couldn't open the file! I have to do something about that!
}

samsunnyuk

oh i thought there would be a special command for checking that.. Thanks. What do you think of my idea though? here

Errant

It's a nice idea but how about ading the following (IMO essential) checks ot your code.

Check that each url submitted has www. and .(tld) parts
Have a list of words that are not allowed in the url / description (look at the page for ideas...)
SOmeone managed to post a picture as the description , you should either try to capture and resize it if that happens or stop it happenening (by removing html code or escaping the text.

The rest ogf the code (with suggestions) looks ok tho!