Fantastic Bugs and How to Avoid Them

Legacy code can be riddled with bugs — both ordinary and extraordinary. Quickly finding and conquering them is crucial for upgrades and for less painful maintenance. Join me as I demonstrate fixes and avoidance strategies for some of the most prevalent bugs found in legacy code.

Transcript

1. Fantastic Bugs and How to Avoid Them LONGHORN PHP | OCT 2021 @afilina
2. Zend Framework 1 has undefined variables.
3. Anna Filina • Coding since 1997 (VB4) • PHP since 2003 • Legacy archaeology • Test automation • Public speaking • Mentorship • YouTube videos
4. • Real-world "silly" bugs. • How they came to be. • How you can avoid making mistakes. • Also, I need to rant about bad code. What is this talk about?
5. Bugs as justification for following best practices.
6. $paths = $this->getPaths(); $urls = array_map(function ($path) { return self::BASE_URI . $path; }, $paths); array_map(): Expected parameter 2 to be an array, string given
7. private function getPaths() { return '[ {"list_products":"/products"}, {"view_cart":"/cart"}, ]'; } You have to read every method to know what will happen.
8. private function getPaths(): array { return [ $this->path1, $this->path2, ]; } We get earlier warnings, and IDEs can highlight problems in your code.
9. $paths = $this->getPaths(); $urls = array_map(function ($path) { return self::BASE_URI . $path; }, $paths); Array to string conversion
10. private $path1 = ['list_products' => '/products']; private $path2 = ['view_cart' => '/cart']; We assumed that the paths were strings.
11. private $path1 = '/products'; private $path2 = '/cart'; We change paths to strings.
12. $urls = array_map(function (string $path) { return self::BASE_URI . $path; }, $paths); Useful if closure is very long, but the warning is still not early enough.
13. /** * @return array<int, string> */ private function getPaths(): array Type hints to describe array in more detail, then use a static analysis tool.
14. composer require --dev vimeo/psalm vendor/bin/psalm --init Detected level 7 as a suitable initial default vendor/bin/psalm src
15. /** * @return array<int, string> */ private function getPaths(): array { return [ $this->path1, $this->path2, ]; } ERROR: MixedReturnTypeCoercion - src/TypeMismatch.php:65:16 - The type 'array{0: mixed, 1: mixed}' is more general than the declared return type 'array<int, string>' ...
16. private $path1 = '/products'; private $path2 = '/cart'; It's possible for these values to be set to something other than strings.
17. /** * @var string */ private string $path1 = '/products'; In older PHP versions, we use annotations. In PHP 7.4+, we can specify the type.
18. /** * @return array<int, string> */ private function getPaths(): array { return [ $this->path1, $this->path2, ]; } Annotations potentially lie, unless you can prove them with static analysis.
19. if ($path === '') { throw new InvalidArgumentException('Is blank'); } The problem with this is you have to remember to do it everywhere.
20. $urls = array_map(function (string $path) { return self::BASE_URI . $path; }, $paths); It would be nice if instead of expecting a simple string, we could say that we expect something that is not blank.
21. $urls = array_map(function (Path $path) { return self::BASE_URI . $path; }, $paths); We can declare a custom type and just assume that it's valid.
22. final class Path { public function __construct( private string $path ) { Assert::that($path) ->notBlank(); } public function getValue(): string { return $this->path; } } Class is final; value comes through constructor and gets validated; class has no setters.
23. composer require beberlei/assert Look for assertion libraries to help you with the validation.
24. /** * @return array<int, Path> */ private function getPaths(): array { return [ new Path('/products'), new Path('/cart'), ]; } We now swap our strings for Path objects and update the type hint.
25. array_map(function (Path $path) { return self::BASE_URI . $path->getValue(); }, $paths); When we see a Path, we can assume that it's ever blank.
26. Float coercion.
27. public function setPrice(int $price) { $this->price = $price; } $this->setPrice(1.15); 1.15 float will be coerced to int, rounding it down to 1 without any warnings.
28. <?php declare(strict_types=1); TypeError : Argument 1 passed to MyClass::setPrice() must be of the type int, float given declare_strict works per file, so you can progressively modernize your app.
29. Strict types. Strict types. Strict types.
30. private string $stringPath; private Path $voPath; Since PHP 7.4, you can have property types, which will also prevent you from accessing a value that wasn't yet assigned.
31. NPEs everywhere.
32. class Product { public $name; } //... $this->findByName($this->product->name); TypeError : Argument 1 passed to MyClass::findByName() must be of the type string, null given
33. class Product { public string $name; } The solution is to simply declare the type.
34. class ProductEntity { public $name; public $price; } final class Product { private string $name; private int $price; //... } ORM models/entities can be converted to stricter objects before you let your app interact with them.
35. return new ProductName( $product->name ); We can even have smaller objects to represent a subset of fields, or even just one field.
36. if ($product->getLastPrice() !== null) { return number_format($product->getLastPrice()); } TypeError : number_format() expects parameter 1 to be float, null given We just checked that it was not null, yet it still crashes due to a null.
37. public function getLastPrice() { return array_pop($this->prices); } There is no guarantee that a method will return the same value each time.
38. $lastPrice = $product->getLastPrice(); if ($lastPrice !== null) { return number_format($lastPrice); } Don't assume anything and assign the value to a local variable first.
39. @$array[$foo->a()]; public function a() { trigger_error('my error', E_USER_ERROR); } $array[$foo->a()] ?? 'something else'; In an attempt to suppress missing index notices, we can suppress bigger problems. Avoid @.
40. interface ApiAware { public function setApi(Api $api); } if ($class instanceof ApiAware) { $class->setApi($api); } *Aware interfaces are a common way to inject services.
41. final class MyClass implements ApiAware { private $api; public function setApi(Api $api): void { $this->api = $api; } public function sendApiRequest() { $product = new Product(); $this->api->sendRequest($product); } } If setApi was not called before sendRequest for any reason, this will crash.
42. Error : Call to a member function sendRequest() on null
43. final class MyClass { public function __construct( private Api $api ) {} public function sendApiRequest() { $product = new Product(); $this->api->sendRequest($product); } } Ditch those interfaces and use proper dependency injection.
44. Dependency injection is your friend.
45. if (!empty($array)) { return $array[0]; } Trying to access array offset on value of type bool empty doesn't just operate on arrays.
46. empty(""); empty(0); empty(0.0); empty("0"); empty(null); empty(false); empty(array()); All these things are considered empty.
47. !== "" !== 0 !== 0.0 !== null === true !== [] Check specifically for what you need instead. Otherwise, types can get coerced and break the logic.
48. 0.99 + 0.01 === 1 This is false.
49. IEEE 754 floating point arithmetic. In simple terms, if you use floats, numbers may not add up.
50. $amountInCents + 1 Operate on integers when possible. Divide at the presentation layer.
51. Interfaces vs concrete classes.
52. /** @var PaymentGatewayInterface */ $gateway = $this->getSelectedGateway(); $gateway->preauthorizePayment(); Even though the gateway is the right type, the method might not exist.
53. PaymentGatewayInterface StripeGateway capturePayment PaypalGateway capturePayment preauthorizePayment capturePayment This happens when you implements additional methods, so it but only in some classes. Try to stick to the interface methods in your implementations.
54. @afilina Questions?