Börja med att fixa indentering så att man ser hur kodens logik är uppbyggd. En vettig editor gör detta automatiskt, exempelvis:
<li>
MEDLEMSPORTALEN
<ul>
<?php
if (isset($_SESSION["username"])) {
$username = $_SESSION['username'];
$info_ = mysql_query("SELECT status FROM medlemmar WHERE username='$username'");
$information = mysql_fetch_assoc($info_);
$admin = "{$information['status']}";
}
if ($admin == "headhunter") {
echo "<li><font color='#FFFFFF'>" . $username . "</font></li>";
echo "<li><a href='index.php?id=login'>HANTERA MEDLEMMAR</a></li>";
echo "<li><a href='index.php?id=skapa_nyhet'>HANTERA NYHETER</a></li>";
echo "<li><a href='index.php?id=logout'>LOGGA UT</a></li>";
} else if ($admin == "medlem") {
echo "<li><font color='#FFFFFF'>" . $username . "</font></li>";
echo "<li><a href='index.php?id=logout'>LOGGA UT</a></li>";
}
?>
<?php
if (!isset($_SESSION["username"])) {
echo "<li><a href='index.php?id=login'>LOGGA IN</a></li>";
echo "<li><a href='index.php?id=registrera'>REGISTRERA DIG</a></li>";
}
?>
</ul>
</li>
Skrivet av downup:
if(isset($_SESSION["username"])) {
$username = $_SESSION['username'];
Inget fel, men det finns ingen anledning att blanda apostrofer och citationstecken för att ange vektorindex. Välj en konvention och håll dig till den. Att följa en viss konvention gör att man enklare och snabbare kan få en överblick över koden, vilket minimerar fel.
Skrivet av downup:
$info_ = mysql_query("SELECT status FROM medlemmar WHERE username='$username'");
Använd `mysqli` och prepared statements. Finns mängder med information och guider om detta på nätet. Så som det står ovan så har du troligen ett direkt och trivialt exploaterbart säkerhetshål i koden. Även om du inte tänker dig att dina användare är elaka så är det en dålig vana, och det ligger konstant botar som söker efter liknande hål och hamrar på alla sidor som är tillgängliga på det publika nätet.
Skrivet av downup:
$admin = "{$information['status']}";
Bekymret med klammerparenteser är rätt onödigt här. Enklare är att skippa både dessa paranteser och citationstecknen och bara skriva:
$admin = $information['status'];
vilket är ekvivalent, förutom att PHP nu kan tilldela vänsterledet värdet i högerledet direkt i stället för att behöva tolka högerledet som en sträng som tillåter interpolering, direkt träffa på en variabel och interpolera den, och sedan tilldela denna till vänsterledet.
Men det är egentligen än mer onödigt om du använder "prepared statements" då du kan binda en variabel direkt till ett resultat av en databasfråga i stället för att bolla runt den i koden till ingen vinning.
Skrivet av downup:
echo "<li><font color='#FFFFFF'>" . $username . "</font></li>";
Skapa en CSS-klass med namn `username`, definiera `color: white;` och applicera den på listelementet i stället för att hårdkoda färg, och skippa med all fördel `<font>`-taggen; dvs skriv ut HTML likt:
<li class="username">Användarnamn</li>
där `username`-klassen i CSS definieras likt
.username {
color: white;
}
Så som du skrivit din `echo`-sats så behöver du inte gå ur citationstecknen för att inkludera en variabel, så det du skriver är ekvivalent med
echo "<li><font color='#FFFFFF'>$username</font></li>";
Det är egentligen ett otyg att skriva ut HTML-kod direkt som argument till `echo` på detta sätt. Bättre är generellt att lämna PHP-läge och skriva "ren" HTML, där variabler flikas in som små PHP-snuttar:
<?php
if ($admin == "headhunter") {
?>
<li class="username"><?=$username?></li>
<li><a href="index.php?id=login">HANTERA MEDLEMMAR</a></li>
<li><a href="index.php?id=skapa_nyhet">HANTERA NYHETER</a></li>
<li><a href="index.php?id=logout">LOGGA UT</a></li>
<?php
}
?>
där `<?=$username?>` är en "snabbsyntax" för `<?php echo $username; ?>` som är trevlig att använda i detta syfte.
Som en sidonotis så behöver man i praktiken inte skriva `</li>` i HTML när det följs av exempelvis `<li>` eller `</ul>`, vilket städar upp koden lite, men det är ett stilval (utläggning om valbara sluttaggar här).
Detta är dock fortfarande inte riktigt bra, för du skriver ut `$username` utan att se till att det faktiskt bara är vanlig text i variabeln. En busig användare skulle kunna döpa sig till `<h1>HULKEN!</h1>` eller allmänt förstöra sidans layout, och en elak användare skulle kunna döpa sig till JavaScript-kod som anropar externa resurser som då skulle kunna snoka fram uppgifter om exempelvis andra användare på sidan. Tumregel, som det väldigt sällan finns anledning att frångå: allt som skrivs ut ska behandlas med `htmlspecialchars()`. Skriv alltså snarare
<li class="username"><?=htmlspecialchars($username)?></li>
Tycker man det blir för bökigt att skriva så är ett "trick" att skapa en funktion likt
function h(str) { return htmlspecialchars(str); }
så kan du sedan skriva
<li class="username"><?=h($username)?></li>
men det sparar inte speciellt mycket tid i längden. Återigen så kommer en vettig editor hjälpa dig att skriva PHP snabbare och mer korrekt.
Skrivet av downup:
if ($admin == "headhunter") {
…
} else if ($admin == "medlem") {
…
}
Felet kommer av att du försöker använda `$admin` oavsett om du satt denna variabel eller inte. Antingen kan du låta denna kontroll ligga innanför det block där du sätter `$admin`, eller så kan du testa med `if (isset($admin))` innan du använder variabeln. Jag ser att du valde att lösa det på det första sättet.
När du vill testa en enstaka variabel (`$admin` i detta fall) mot flera värden så används ofta tydligast en `switch`-sats (tidigare utläggning i ämnet på forumet).
I detta fall så är du ju dock egentligen inte intresserad av de som har status "Medlem" så som logiken är skriven, så du kan helt enkelt lägga till en `if`-sats för att kolla om användaren är en "headhunter" och i så fall skriva ut länkarna för "Hantera medlemmar" och "Hantera nyheter".
<li>
MEDLEMSPORTALEN
<ul>
<?php
if (isset($_SESSION["username"])) {
…Skriv om med `mysqli`, sätt `$admin`…
?>
<li class="username"><?=htmlspecialchars($username)?>
<?php
if ($admin == "headhunter") {
?>
<li><a href="index.php?id=login">HANTERA MEDLEMMAR</a>
<li><a href="index.php?id=skapa_nyhet">HANTERA NYHETER</a>
<?php
}
?>
<li><a href="index.php?id=logout">LOGGA UT</a>
Därefter, i stället för att först kolla om `$_SESSION["username"]` är satt och i så fall utföra ett block, och sedan direkt efter kolla igen om denna variabel inte är satt och i så fall köra ett block, så kan du ju använda en vanlig `else`-sats. Fortsätt då ovanstående kodstycke med:
<?php
} else {
?>
<li><a href="index.php?id=login">LOGGA IN</a>
<li><a href="index.php?id=registrera">REGISTRERA DIG</a>
<?php
}
?>
</ul>
</li>
…på ett ungefär, väldigt otestat (klass 1-varning för syntaxfel).
Därutöver så skulle du kunna ha nytta av att veta skillnaden mellan `==` och `===` i PHP (skillnaden kan lätt förbluffa) och när det senare föredras. Ifall sidan du är på är `index.php` så behöver du inte ange den i länkmålet, utan det räcker med `href="?id=registrera"`. `id` låter för övrigt som ett lite märkligt namn där, då det inte är ett ID-nummer, men det är ett rent stilval. `$admin` är också ett lite märkligt variabelnamn, då det låter som något som skulle vara `true` eller `false`.
Det är ofta fördelaktigt att sköta alla databasfrågor högst upp i filen, innan du börjar skriva ut HTML. Det ger mycket renare layout om man separerar sådan logik från rena HTML-utskrifter. PHP kan se rätt vettigt ut som ett templatingspråk om man följer en del sådana konventioner, men gör man inte det blir det fruktansvärt lätt otydligt definierad spagetti av alltihop .
