Pytanie o poprawność kodu ;)

[LoBo]

Witam.
Od niedawna uczę się programowania dlatego proszę nie negować za bardzo

Aktualnie pisze na swoje potrzeby cms i jestem na etapie komentarzy do newsów. Wszystko działa prawidłowo, natomiast mam pytanie czy ten podwójny odczyt z bazy danych by wyciągnąć komentarz z jednej i login autora z drugiej tabeli można jakoś inaczej zastąpić czy tak po prostu musi byc i jest to prawidłowe.

Kod PHP:

<?php
ini_set( 'display_errors', 'On' ); 
error_reporting( E_ALL );
$nid=$_GET['nid'];
include('../engine/baza.php');
/*###################Odczyt komentarzy#######################*/
$czytaj=mysql_query("select * from komentarze where nid='$nid'");
while($row = mysql_fetch_array($czytaj)) {
$user_kom=mysql_query("select user from userzy where id=".$row['uid']."");
while($row_user = mysql_fetch_array($user_kom)) {?>

<table border="0" width="373" height="55">
    <tr>
        <td height="22" width="228"><b>Nick:</b><font color="#0000FF"><b><?php echo $row_user['user']; ?></b></font></td>
        <td height="22" width="129"><font color="#BC0707"><i><?php echo date('d.m.Y H:i', $row['data']);  ?></i></font></td>
    </tr>
    <tr>
        <td height="25" width="373" colspan="2"><?php  echo $row['tresc']; ?><br><hr width="300" /></td>
    </tr>
</table>
<?php    }}
 ?>

Za porady/sugestie z góry dziękuję

[R_Rafalsky]

Można ale tak też jest prawidłowe (i łatwiejsze). Staraj się pisać w aktualnych standardach, zapomnij o tabelkach, zapomnij o czymś takim: font color="#0000FF". Skrypt jest niebezpieczny. Po pierwsze włączamy error_reporting tylko dla własnych potrzeb. Jak strona gotowa to koniecznie wyłącz pokazywanie jakichkolwiek błędów. Im świat mniej wie o budowie strony tym lepiej. A po drugie ciekawostka dla Ciebie.
jeżeli wejdę na adres index.php?nid=1'+or+1=1 to co się stanie?

[LoBo]

Generalnie rzecz biorąc to raportowania nie zostawiam i używam tylko dla własnych potrzeb, a że próbuję sklecić komentowanie co jest "in progress" to i raportowanie sobie załączam
Co do tabelek to tez się dużo naczytałem, że tego się nie używa no ale jak na razie skupiam się na nauczeniu php+mysql, później zajmę się smartami divami i CSS (nie wszystko na raz )
Co do nid=1'+or+1=1 zrobiłem i nic sie nie stało

Warning: mysql_fetch_array(): supplied argument is not a valid MySQL result resource in /home/HTTP/dawid/includes/news_komentarze.php on line 8

Ale domyślam się że chodzi o żebym użył htmlspecialchars oraz strip_tags ??

[Pedro84]

Ja bym jednak już od początku nie mieszał logiki z warstwą prezentacji.

1.

Kod:

$nid=$_GET['nid'];
$czytaj=mysql_query("select * from komentarze where nid='$nid'");

SQL injection.

2. Bezsensownie komentujesz kod. Po co komentujesz oczywiste zapytanie zaśmiecając kod?
3. Wiesz co to są wcięcia, po co się je stosuje?

[LoBo]

1. nie bardzo rozumiem o co chodzi, co zmienić ?
2. Komentuje żeby się nie pogubić
3. Wiem Co to są wcięcia, nie wiem po co się stosuje

Generalnie dopiero uczę się kodowania dlatego proszę o wyjaśnienie co poprawić, co zmienić,jak obronić się przed SQL injection. w tym przykładzie.

[Pedro84]

1. Poczytaj: http://blogophp.com/2008/10/13/rozdzielenie-warstw-aplikacji/
2. To bezsensu. Uczysz się złych nawyków. Pisz kod tak, aby sam się komentował. Nazywaj poprawnie zmienne, zastosuj jedną konwencję, etc.
3. Żeby kod był czytelny, Twój nie jest.
4. Poczytaj: http://php.net/manual/pl/function.mysql-real-escape-string.php

[KowR]

Wracając do dwóch zapytań... Lepiej złączyć je. Poczytaj o typach złączeń: LEFT JOIN, RIGHT JOIN, INNER JOIN, OUTER JOIN. Dzięki jednemu zapytaniu pobierzesz wszystko co tylko Ci potrzeba bez pętli w pętli. Resztę koledzy już wypisali.

[LoBo]

Ciąg dalszy mojej nauki php z wziętymi waszymi radami, generalnie testuję bez logowania dlatego id użytkownika nie jest jeszcze zdefiniowane/pobierane tak samo sesja.
Prosze o ocenę czy czytelność/jakość kodu oraz bezpieczeństwo się poprawiły czy tylko mi się wydaje oraz co jeszcze mogę zastosować i poprawić by kod był lepszy.
PS: zaznaczam, że dopiero się ucze także o smarty, PDO, CSS czy innych zaawansowanych (i tych mniej zaawansowanych) technikach na razie nie ma mowy. Póki co skupiam sie na zrozumieniu kodowania.

Kod PHP:

<?php

//ini_set( 'display_errors', 'On' );
//error_reporting( E_ALL );

include('../engine/baza.php');
$nid = mysql_real_escape_string(htmlspecialchars($_GET['nid']));
$t = time();
$tresc = mysql_real_escape_string(htmlspecialchars($_POST['tresc']));
if(!empty($nid)){
    $czytaj = mysql_query("select * from komentarze, userzy WHERE komentarze.uid = userzy.id AND nid='$nid'");
    while($row = mysql_fetch_array($czytaj)){
        include('news_komentarze_view.html');
        }
            if(isset($_POST['submit']))
            {
                if (strlen($_POST['tresc']) < 5 or strlen($_POST['tresc']) > 250) 
                {
                    echo 'Treść musi mieć od 5 do 250 znaków.';
                    } 
                    else
                    {
                $add_comment = "insert into komentarze (nid, uid, tresc, data) VALUES ('$nid','$uid','$tresc','$t')";
                $wykonaj = mysql_query ($add_comment);
                                if($wykonaj == 'TRUE'){
                                    echo 'komentarz dodany!';
                                    }
                                    else
                                    { 
                                    echo 'Nie dodano komantarza!';
                                    }
                    }
            }
            if (!isset($_SESSION['user'])) //Testuję bez zalogowania.
            {
            include('news_komentarze_add.html');
            }
            else
            {
                echo'Musisz być zalogowany by pisać komentarze.';
            }
}
else
{
    echo 'Nie wybrano strony newsa';
}
?>