[PHP] Best practices

[Anoniem: 49227]

Voor mijn zomerproject ben ik bezig om PHP-scripts statisch te analyseren. Het eindproduct moet uiteindelijk aangeven of er 'un-safe'-variabelen gebruikt worden in functies waarin dit gevaar kan opleveren. Klein voorbeeldje:

PHP:

<?php
 echo $_GET['foo']; // gevaarlijk

 $bar = $_GET['bar'];
 echo $bar;           // ook gevaarlijk
 
 $fred = htmlentities(stripslashes($bar));
 echo $fred; // ongevaarlijk
?>

Het volgen van 'un-safe'-variabelen door assignments, functies en classes heen is vrij complex. Het gaat ook nog wel even duren voordat dit helemaal af is. Mijn mentor gaf mij daarom de tip om ook andere dingen te rapporteren die statisch te detecteren zijn. Hierdoor kan het project een soort PHP equivalent van FindBugs worden.

Enkele voorbeelden van dingen die opgemerkt kunnen worden:
1: Variabelen gebruiken zonder ze eerst te declareren
2: Het gebruik van haakjes bij include en require (zie ook de PEAR-coding style)
3: Bij elke conditie in lange "if-elseif" telkens 1 variabele vergelijken met een waarde, hier is een switch voor bedoeld.

Nu is het laatste voorbeeld al iets waarover je kan discussieren. Bad practices bedenken die altijd gelden is dan ook niet triviaal. Dat kan misschien ook de reden zijn waarom er niet veel te vinden is over bad practices specifiek voor PHP. Sommige van de dingen die in FindBugs gerapporteerd worden zijn wel nuttig en er zijn ook een aantal algemene code smells die ik kan gebruiken. Maar deze zijn allemaal niet specifiek voor PHP. En er moeten toch specifieke code smells voor PHP zijn lijkt me.

Na dit lange verhaal komt dan eindelijk de vraag:
Wat vinden jullie bad practices in PHP? Waar moeten (beginnende) programmeurs op gewezen worden om hun code te verbeteren?

Mocht je iets weten probeer dan alstublieft ook te onderbouwen waarom iets niet netjes is of gevaar op levert. Nuttige 'bad practices' zullen worden opgenomen in het project als blijkt dat ze inderdaad statisch te detecteren zijn.

[chem]

Tsjah, the usual aan dode-code vinden, ongebruikte variabelen, ongedeclareerde variabelen, functies zonder return (of soms geen return), functies met een variabele return (niet per se slecht)

In principe is het lijstje gelijk aan de analyzers van de meeste talen lijkt me zo.

Andere dingen die fout gaan: pak eens 100 topics uit P&W die binnen 5 reacties zijn gesloten met php in de titel

[crisp]

of je al dan niet stripslashes moet uitvoeren hangt af van de 'magic quotes' instelling in PHP - een ranzige PHP feature net als register_globals.
htmlentities gebruiken is over-the-top, htmlspecialchars is doorgaans voldoende.

[Anoniem: 49227]

chem schreef op vrijdag 21 juli 2006 @ 11:37:
Tsjah, the usual aan dode-code vinden, ongebruikte variabelen, ongedeclareerde variabelen, functies zonder return (of soms geen return), functies met een variabele return (niet per se slecht)

In principe is het lijstje gelijk aan de analyzers van de meeste talen lijkt me zo.

Dat klopt, de meeste dingen zijn ook terug te vinden in de meeste andere talen. Maar ik vraag juist naar PHP-specifieke dingen.
Een voorbeeld van een discussie punt is voor PHP:

PHP:

<?php
   new foo(); //netjes
   new foo;   //niet netjes? 
?>

beide statements geven hetzelfde resultaat, maar is het ene netter dan het andere? Zou je willen dat het gerapporteerd wordt?

crisp schreef op vrijdag 21 juli 2006 @ 11:38:
of je al dan niet stripslashes moet uitvoeren hangt af van de 'magic quotes' instelling in PHP - een ranzige PHP feature net als register_globals.
htmlentities gebruiken is over-the-top, htmlspecialchars is doorgaans voldoende.

Dat klopt, maar het in de analyse gaat het er niet zozeer om hoe je een variabele veilig maakt, maar of het gebeurd. En of het nu door htmlentities of htmlspecialchars gebeurd maakt niet zoveel uit.
Vind je trouwens dat het gebruik van htmlentities zou moeten worden gerapporteerd met een opmerking dat htmlspecialchars meestal genoeg is? Heeft het zoveel impact op performance?

chem schreef op vrijdag 21 juli 2006 @ 11:38:
Andere dingen die fout gaan: pak eens 100 topics uit P&W die binnen 5 reacties zijn gesloten met php in de titel

Deel 2 van mijn research

[Spider.007]

Anoniem: 49227 schreef op vrijdag 21 juli 2006 @ 11:52:
[...]

beide statements geven hetzelfde resultaat, maar is het ene netter dan het andere? Zou je willen dat het gerapporteerd wordt?

[...]

Heeft het zoveel impact op performance?

Wat is je scope dan? Bugs, nutteloze code, performance optimalisaties of netheid Zo zijn je 'lange-if-else' constructies erg lastig te detecteren. /half-OT Wat gebruik je trouwens als analyzemethode? Reguliere expressies in PHP, of de tokenizer oid?

[Anoniem: 49227]

Spider.007 schreef op vrijdag 21 juli 2006 @ 12:32:
[...]

Wat is je scope dan? Bugs, nutteloze code, performance optimalisaties of netheid Zo zijn je 'lange-if-else' constructies erg lastig te detecteren.

Scope is alle vier. Als je ook kijkt naar findbugs delen ze bugs in in 6 gebieden:
1) Correctness
2) Malicious code vulnerability
3) Multithreaded correctness
4) Performance
5) Style
6) Internationalization
Nu zijn deze gebieden niet allemaal nuttig voor PHP, maar de gebieden 1, 2, 4 en 5 zeker wel.

Spider.007 schreef op vrijdag 21 juli 2006 @ 12:32:
/half-OT Wat gebruik je trouwens als analyzemethode? Reguliere expressies in PHP, of de tokenizer oid?

Ik heb een syntax-definitie voor PHP gemaakt en een pretty-printer. De syntax definitie parsed een PHP-bestand naar een ATerm van dit bestand. Dit is een boom met knopen voor de verschillende taal constructies. Deze boom kan ik op verschillende manieren aflopen en op specifieke knopen kan ik specifieke dingen doen. Het systeem waarin dit alles gebeurd is Stratego/XT

Zie de blog van het project voor meer info.

[ Voor 5% gewijzigd door Anoniem: 49227 op 21-07-2006 15:04 ]

[orf]

meer ranzige dingen:

variabelen binnen quotes
array keys zonder quotes
if (constante) / if ($_POST['iets']) ipv defined / isset
geen / slechte foutafhandeling (file openen, vervolgens allerlei dingen met file gaan doen zonder te kijken of file geopend is)
includes met procedurele code

Alles wat een notice oplevert bij error reporting E_ALL / E_STRICT

[Anoniem: 49227]

orf schreef op vrijdag 21 juli 2006 @ 19:26:
meer ranzige dingen:

variabelen binnen quotes

Ik ben zelf ook van mening dat je beter een concat kan doen van variabelen dan deze rechtstreeks binnen je string gebruiken. Maar hierover verschillen de meningen nogal geloof ik.

orf schreef op vrijdag 21 juli 2006 @ 19:26:
array keys zonder quotes
if (constante) / if ($_POST['iets']) ipv defined / isset
geen / slechte foutafhandeling (file openen, vervolgens allerlei dingen met file gaan doen zonder te kijken of file geopend is)
includes met procedurele code

Alles wat een notice oplevert bij error reporting E_ALL / E_STRICT

Het laatste is inderdaad iets wat ik wel zou willen omdat de meeste dingen statisch gechecked kunnen worden. Het probleem is dat ik nog geen lijst heb kunnen vinden van welke dingen een E_NOTICE opleveren. Iemand enig idee? Tweede punt is trouwens een zeer goede, thxs!

[rickmans]

Wellicht is dit nog wel leuke link voor je: http://gerard.yoursite.nl/got/php-tiplist/

[EnsconcE]

Anoniem: 49227 schreef op vrijdag 21 juli 2006 @ 20:22:
...
Het probleem is dat ik nog geen lijst heb kunnen vinden van welke dingen een E_NOTICE opleveren. Iemand enig idee?
...

E_NOTICE

Soms gebeurt er iets wat een fout kan zijn of niet. Het script wordt wel verder uitgevoerd. Een voorbeeld is het gebruik van bijvoorbeeld $foo[bar], of het gebruiken van een nog niet bestaande variabele.

http://nl3.php.net/manual/nl/language.constants.php

Misschien helpt dit?

[Anoniem: 49227]

rickmans schreef op vrijdag 21 juli 2006 @ 20:27:
Wellicht is dit nog wel leuke link voor je: http://gerard.yoursite.nl/got/php-tiplist/

Sommige dingen zijn wel handig, andere wellicht een beetje kort door de bocht. Het eerste punt is eigenlijk alleen nodig in ontwikkel-omgevingen. Anders zou ik de error-reporting lager zetten. Het laatste punt is in ieder geval het overwegen waard om te noemen. Bedankt voor de tip!

EnsconcE schreef op vrijdag 21 juli 2006 @ 21:09:
[...]

E_NOTICE

Soms gebeurt er iets wat een fout kan zijn of niet. Het script wordt wel verder uitgevoerd. Een voorbeeld is het gebruik van bijvoorbeeld $foo[bar], of het gebruiken van een nog niet bestaande variabele.

http://nl3.php.net/manual/nl/language.constants.php

Misschien helpt dit?

Niet echt, dit is hetzelfde wat er ook staat bij: http://nl3.php.net/manual/nl/ref.errorfunc.php#e-notice.
Run-time notices. Indicate that the script encountered something that could indicate an error, but could also happen in the normal course of running a script.

Eenvraag is nu of iemand een lijst weet van de dingen die een eventueel een error zouden kunnen zijn. Er staan her en der in de documentatie wel dingen aangegeven, zoals hierboven, maar ik heb nog nergens een hele lijst gevonden. Maar bedankt voor het meedenken

[aex351]

Anoniem: 49227 schreef op vrijdag 21 juli 2006 @ 11:52:

Dat klopt, de meeste dingen zijn ook terug te vinden in de meeste andere talen. Maar ik vraag juist naar PHP-specifieke dingen.
Een voorbeeld van een discussie punt is voor PHP:

PHP:

<?php
   new foo(); //netjes
   new foo;   //niet netjes? 
?>

beide statements geven hetzelfde resultaat, maar is het ene netter dan het andere? Zou je willen dat het gerapporteerd wordt?

Deel 2 van mijn research

De eerste eist een constructor en de tweede niet. Heeft weinig te maken met netjes of niet netjes. Je programmeerd gewoon met de mogelijkheden die zo'n taal te bieden heeft.

edit van de constructor klopt toch niet

[ Voor 14% gewijzigd door aex351 op 22-07-2006 01:11 ]

[brokenp]

Voor gebied 1 van de genoemde (Correctness) zou je eens kunnen spieken bij ESC/JAVA een static checker voor Java. (http://secure.ucd.ie/products/opensource/ESCJava2/)
Deze probeert ook assertions te checken die je in JML notatie in je commentaar zet.

Voor zover ik weet is er niet zoiets voor PHP

[Anoniem: 84120]

Even op jouw kleine voorbeeldje

Anoniem: 49227 schreef op vrijdag 21 juli 2006 @ 11:29:

PHP:

<?php
 echo $_GET['foo']; // gevaarlijk

 $bar = $_GET['bar'];
 echo $bar;           // ook gevaarlijk
 
 $fred = htmlentities(stripslashes($bar));
 echo $fred; // ongevaarlijk
?>

Wat is daar mis aan: stripslashes ??? En als magic_quotes uit staat (tegenwoordig standaard), verder: wat als $_GET['bar'] niet geset is? Dus mij lijkt het ook wel nuttig altijd een isset($_GET['ikwilditbeesthebben']) te doen voordat je assigned (want geeft zowieso een NOTICE dit, in onberekende omstandigheden)

[Anoniem: 49227]

Anoniem: 84120 schreef op zaterdag 22 juli 2006 @ 18:58:
Even op jouw kleine voorbeeldje

[...]

Wat is daar mis aan: stripslashes ??? En als magic_quotes uit staat (tegenwoordig standaard),

Dat zie ik nu pas, het moest eigenlijk addslashes zijn.

Anoniem: 84120 schreef op zaterdag 22 juli 2006 @ 18:58:
verder: wat als $_GET['bar'] niet geset is? Dus mij lijkt het ook wel nuttig altijd een isset($_GET['ikwilditbeesthebben']) te doen voordat je assigned (want geeft zowieso een NOTICE dit, in onberekende omstandigheden)

Dan krijg je inderdaad een NOTICE. Maar dit is een klein voorbeeld om te laten zien wat mijn project als algemeen doel heeft om de detecteren. Met allerlei checks eromheen wordt dat niet veel duidelijker.

Het opmerken van zoiets als het niet gebruiken van isset is iets extras wat er bij mijn project bij komt. Het gebruik van isset, of eigenlijk het gebrek daaraan, ga ik dan ook zeker meenemen in het project.

[Anoniem: 49227]

Sinds vorige week heb ik een aantal patterns toegevoegd die ook gedetecteerd worden in de tool. Waaronder ook de opmerking van orf.Daarnaast staan er meer beschrijvingen van patronen in de documentatie. Deze worden ook opgenomen in de tool.

Nu er ook voorbeelden van patronen zijn wilde ik het topic weer eens even omhoog helpen

[Koeniepoenie]

Een variatie op de reeds geimplementeerde pattern:

PHP:

doeWat("$var");

is

PHP:

doeWat("".$var."");

Deze constructie zie ik ook nog wel eens voorbij komen.

[TweakBoy]

Koeniepoenie schreef op vrijdag 28 juli 2006 @ 12:25:
Een variatie op de reeds geimplementeerde pattern:

PHP:

doeWat("$var");

is

PHP:

doeWat("".$var."");

Deze constructie zie ik ook nog wel eens voorbij komen.

PHP:

doeWat("{$var}");
// deze ook dan?

[JeromeB]

Koeniepoenie schreef op vrijdag 28 juli 2006 @ 12:25:
Een variatie op de reeds geimplementeerde pattern:

PHP:

doeWat("$var");

is

PHP:

doeWat("".$var."");

Deze constructie zie ik ook nog wel eens voorbij komen.

Misschien begrijp ik je niet goed, maar het kan toch gewoon zo:

code:

<?php doeWat($var) ?>

[PrisonerOfPain]

JeromeB schreef op vrijdag 28 juli 2006 @ 13:33:
[...]


Misschien begrijp ik je niet goed, maar het kan toch gewoon zo:

code:

<?php doeWat($var) ?>

Nu heb je niet de garantie dat $var een string is.

PHP:

<?php doeWat((string)$var)?>

[Anoniem: 49227]

Koeniepoenie schreef op vrijdag 28 juli 2006 @ 12:25:
Een variatie op de reeds geimplementeerde pattern:

PHP:

doeWat("$var");

is

PHP:

doeWat("".$var."");

Deze constructie zie ik ook nog wel eens voorbij komen.

TweakBoy schreef op vrijdag 28 juli 2006 @ 13:03:
[...]

PHP:

doeWat("{$var}");
// deze ook dan?

Die komen inderdaad op hetzelfde neer. Ik heb ze toegevoegd aan de build-farm vanaf revisie 111 (die vanzelf moet langskomen).

PrisonerOfPain schreef op vrijdag 28 juli 2006 @ 14:18:
[...]
Nu heb je niet de garantie dat $var een string is.

PHP:

<?php doeWat((string)$var)?>

Mocht je dat willen natuurlijk. Soms denken mensen dat "$var" hetzelfde is als het doorgeven van $var. Maar ik heb de documentatie aangepast zodat ook deze optie wordt genoemd.