Exakat 1.9.5 includes analysis that prevents code from decaying, or warn you early about it : useless type checks thanks to Typehint, non-implemented interfaces, incompatible signatures with PHP 7.3, wrong expectations with interface typehints. Also, PHP 7.4 migration was upgraded with the upcoming change of bit shift precedence versus concatenation. And, Ambassador reports typehints for methods, and all magic property used.

The Exakat 1.9.5 is beautiful because it doesn’t last.

Useless Type Checks With Typehints

One of the tactical reasons to adopt typehints everywhere is to hand the checks to PHP. Instead of calling is_array(), one may simply use array as a typehint.

<?php

// Before
function foo($array) {
    if (!is_array($array)) {
        throw new Exception('$array is not an array');
    }
    
    //doSomething()
}   

// After
function foo(array $array) {
    //doSomething()
}   

?>

While it is obvious in this example, it also happens that some comparisons are also hidden. They may be hiding deep into the method, or be nested, or even depends on other arguments before being carried.

<?php

// Before
function foo($argument, $arg2, $arg3) {
    // $argument could be a string (doesn't work with ==, but close)
    if ($argument === 'string') {
        throw new Exception('error');
    }

    if (is_array($argument)) {
        // Nested check
        if(is_bool($arg2) {
        
        }
    } elseif (is_object($arg2) && is_bool($arg3)) {
        // dependent test   
    }
}   

?>

When such situation happens, adding a typehint too hastily to the signature of the method will lead to dead code : some of the conditions will never be valid anymore.

Exakat now uses the typehint provided at the method level, and checks when the remaining comparisons and type checks are always constant : either valid, or invalid. Those are then reported as a promising refactoring issue.

Non-implemented Interfaces

Non-implemented interface is one of those typical error that may happen in production, while linting is only providing green lights. In fact, when interfaces and classes are stored in separate files, PHP doesn’t load them to check one another. As such, changing the interface without changing the class leads to no-compile-time error.

<?php

interface foo {
    function bar();
    function bar2();
    function bar4();
}

?>

Obviously, this will lead to a runtime error. It would be best caught by unit tests, or any code execution that makes use of the code. Though, it may also be code that is on the way out, or is less and less used. Exakat makes a systematic check of the code, and reports those as Dead code.

<?php

// Do you remember which method is not implemented? 
class x implements foo {
    function bar1() {}
    function bar2() {}
    function bar4() {}
}

?>

Upgrade it or remove it : that’s a motto for those.

Incompatible Signatures in PHP 7.3 and 7.4

Incompatible signature is the fact that PHP checks when overwritten methods have the same signature. Same number of arguments, same typehint, same usage of reference. On the other hand, the name of the argument and its default value could change.

Until PHP 7.3, it was a strict comparison, or a drop : you could remove a typehint, but not change it.

<?php

interface foo {
    function bar(A $a);
}

class x implements foo {
    function bar( $a) {}
}

//7.4 : Could not check compatibility between xx::bar(B $a) and foo::bar(A $a), because class A is not available
//7.3 : Declaration of xx::bar(B $a) must be compatible with foo::bar(A $a)
class xx implements foo {
    function bar(B $a) {}
}

?>

In PHP 7.4, the notion of covariance and contra variance was introduced : argument typehint may be compatible with the parent’s typehint. In particular, the parent’s typehint maybe a child class of the child’s typehint. This is the contra variance of arguments : with arguments, the typehint change in opposite direction.

The covariance is for the return value : the parent’s return value must be a parent of the return value of the child. Return type hint must change in the same direction.

The banana, the gorilla and the jungle

This leads to this new error message, as displayed in the example above : “Could not check compatibility between xx::bar(B $a) and foo::bar(A $a), because class A is not available”. At runtime, PHP has to check if the B typehint is compatible with the A type. It needs to load the classes or interfaces B and A, to see if one implements or extends the other.

It is going to be an interesting change in the behavior of the code : until now, with a one-for-one check, PHP never loaded any class when loading one of them : either the type hint is A, either it is not. In every case, there is no need to load the class to check. Only the parent classes are needed.

Nowawady, loading one class means loading the parent classes, but also the type hinted classes (when method signature disagree), and their parent. At that point, if those classes matches, their signatures will be checked again, leading to a new load of classes, and potentially, other classes.

Check below : the class foo() in xx requires B and A to be loaded, then C and D, which may require other classes… You want the banana, you get the whole regimen, the gorilla and then the whole jungle…


<?php
class A extends B {
    function foo2( C $a) {}
}
// This may only be solved by loading C and D.
class B {
    function foo2( D $a) {}
}

interface foo {
    function bar(A $a);
}

class x implements foo {
    function bar( $a) {}
}

// Making $a type hinted with B means that A, B are loaded, then C and D...s
class xx implements foo {
    function bar(B $a) {}
}

?>

One point to keep in mind : with the above example, loading xx leads to a lack of information on C and D. With split files, this is going to be difficult to debug.

Covariance and contra variance are completely unavailable until PHP 7.4 : PHP 7.3 only accepts identical typehint, which will be the default fallback if you need to keep your code backward compatible, or want to make those pesky error message go away.

Self is not what you believe

Yet, if PHP 7.3 is the safe behavior, it also has its own range of shortcomings. Here is one of them, that contra variance will remove.

<?php

interface i { 
    function foo($a) : self ; 
}

class A implements i {
    function foo($a) : self {}
}

?>

Class A, which indeed implements i, cannot return self, because i also returns self. “Declaration of A::foo($a): A must be compatible with i::foo($a): i “. PHP 7.3 only consider selfto be the current class, and doesn’t check its implemented interfaces. Later, i is also using selffor its return type, leading to the error. PHP 7.4 removes this error.

All the hard work of the PHP core group on the notions of covariance and contra variance lead us to split the incompatible signature analysis in two : one before PHP 7.4 and one after PHP 7.4.

Wrong Expectations With Interfaces

The best practice goes as “Do not use classes for typehint, use interfaces”. A class is a concrete implementation, and the typehint creates a hard dependency between the current method and another class. What will happen when this class must evolve to a new version or implementation? The good behavior is to use an interface, which leads to multiple implementation.

There is already an analysis for No Class As Typehint.

Then, this conflicts with the popular usage of public properties : an interface doesn’t specify any properties, but only constants and methods.

<?php

interface i { 
    function foo($a); 
}

class A implements i {
    public $p = 1;
   function foo($a) {}
}

// this function abuses the notion of interface : $i->p is not guaranteed.
function bar(I $i) {
    return $i->p
}

?>

At that point, any class implementing I must have a public property called p. Better options would be to add a getter in the interface, to read the value, and make the property private, though it may only shift the problem to another place.

Whatever the solution, Exakat detects usage of interfaces that go beyond the method and constant usage. Maybe using the class, or a parent class is still the best choice here, until the property is privatized.

Also, No Class As Typehint has been upgraded to avoid suggesting interfaces when it is conflicting with this analysis.

More inventories for Ambassador Report

The Ambassador report now includes 2 new inventories : the inventory of magic properties and the inventory of typehint.

Both those two sections are available in the Ambassador report.

Inventory of magic properties

Magic properties are used when a class implements __get and __set methods, allowing the calling script to use on the fly properties. A large number of those properties are detected by Exakat : such properties are undefined, and depend on object whose class implement the magic methods above.

When such a link is established, Exakat reports all magic properties in the ‘Used magic properties’ inventory. The name of every property is reported, giving the opportunity to review their spelling or their distribution. In upcoming versions, details about usage (read and write), and originating class will be included in that report. Ping us if you want that with a passion!

Inventory of type hints

With the upcoming advent of type hinted properties, it seemed necessary to add an inventory of typehint usage in Exakat. This is the first take with exakat 1.9.5!

The inventory of type hints list all classes and their methods and arguments, with their respective return typehint and typehint, and their default value.

Next version will include property typehints, and then, we’ll start including to typehint suggestions : based on the usage of the argument or the return value, and other contextual elements, it is possible to identify a large number of typehints. Stay tuned!

The Weekly Audits: 2019, Week #38

Exakat includes a ‘weekly’ report: this report is built with a selection of five analyses. This means a short audit report, with few issues to review. This is not a lot to read them, and review them in your code. Everyone in the PHP community can focus on one of the classic coding problems and fix it. Talk about the weekly audit around you: you’ll find programmers facing the same challenges.

To obtain the ‘weekly’ audit, run an audit, and request the ‘Weekly’ report.

# Init the project (skip when it is already done)    
php exakat.phar init -p <yourproject> -R https://github.com/Seldaek/monolog.git -git 

# Run the project (skip when it is already done)    
php exakat.phar project -p <yourproject> 

# Export the weekly project (every Monday)    
php exakat.phar report -p <yourproject> -format Weekly 

# Open projects/<yourproject>/weekly/index.html in your browser

Every week, you can find here 5 new analysis to review in your code. In fact, when your code is clean, you can also take a quick look at the upcoming analysis.

Weekly recommendations for PHP code review : 2019, week 2019-38

Ambiguous Static : Methods or properties with the same name, are defined static in one class, and not static in another.
One Variable String : These strings only contains one variable or property or array.
Too Many Native Calls : Avoid stuffing too many PHP native call inside another functioncall.
Use Named Boolean In Argument Definition : Boolean in argument definitions is confusing.
Constant Class : A class or an interface only made up of constants.

Happy PHP Code Reviews

All the 384 analyzers are presented in the docs, including the guitly Undefined Variable: Variable that is used before any creation.

It is a frequent bug : 77% of all projects are affected.

You can check all of the Exakat reports at the gallery: exakat gallery.

Download Exakat on exakat.io, install it with Docker, upgrade it with ‘exakat.phar upgrade -u’ and like us on github.

Want to Keep in touch with us, subscribe to our newsletter !

Code auditing

Exakat 1.9.5 Review