Introduce declare(strict_types) to force strict typing on builtin method calls

[szepeviktor]

Fixes #9040

@robbieaverill You could add phpcs sniff in the issue to sustain strict typing.

[szepeviktor]

The unit tests need SS_BASE_URL=http://example.com/ this environment variable;

We have a problem. The framework contains many type inconsistencies.
https://travis-ci.org/silverstripe/silverstripe-framework/jobs/552481770

diff --git a/src/Control/HTTPRequest.php b/src/Control/HTTPRequest.php
index 77446332b..0bd7b299b 100644
--- a/src/Control/HTTPRequest.php
+++ b/src/Control/HTTPRequest.php
@@ -877,9 +877,8 @@ class HTTPRequest implements ArrayAccess
                 user_error('HTTPRequest::detect_method(): Invalid "_method" parameter', E_USER_ERROR);
             }
             return strtoupper($postVars['_method']);
-        } else {
-            return $origMethod;
         }
+        return $origMethod;
     }

     /**
diff --git a/src/Control/NullHTTPRequest.php b/src/Control/NullHTTPRequest.php
index 5d9e98667..4b0030501 100644
--- a/src/Control/NullHTTPRequest.php
+++ b/src/Control/NullHTTPRequest.php
@@ -12,6 +12,6 @@ class NullHTTPRequest extends HTTPRequest

     public function __construct()
     {
-        parent::__construct(null, null);
+        parent::__construct('', '');
     }
 }
diff --git a/src/Core/ClassInfo.php b/src/Core/ClassInfo.php
index 586db7aa6..76d552513 100644
--- a/src/Core/ClassInfo.php
+++ b/src/Core/ClassInfo.php
@@ -356,7 +356,7 @@ class ClassInfo
         }

         if (!array_key_exists($lMethod, self::$_cache_methods[$lClass])) {
-            self::$_cache_methods[$lClass][$lMethod] = false;
+            self::$_cache_methods[$lClass][$lMethod] = '';

             $classRef = new ReflectionClass($class);

diff --git a/src/Core/Injector/Injector.php b/src/Core/Injector/Injector.php
index d1a4e1bbd..85432b279 100644
--- a/src/Core/Injector/Injector.php
+++ b/src/Core/Injector/Injector.php
@@ -419,7 +419,7 @@ class Injector implements ContainerInterface
             }

             // okay, actually include it now we know we're going to use it
-            if (file_exists($file)) {
+            if ($file !== null && file_exists($file)) {
                 require_once $file;
             }

diff --git a/src/Core/Manifest/ManifestFileFinder.php b/src/Core/Manifest/ManifestFileFinder.php
index b8bd6604e..8d5600b45 100644
--- a/src/Core/Manifest/ManifestFileFinder.php
+++ b/src/Core/Manifest/ManifestFileFinder.php
@@ -197,7 +197,7 @@ class ManifestFileFinder extends FileFinder
     protected function upLevels($pathname, $depth)
     {
         if ($depth < 0) {
-            return null;
+            return '';
         }
         while ($depth--) {
             $pathname = dirname($pathname);

[robbieaverill]

Yeah I think there's going to need to be some effort involved here to adjust the methods to have consistent return types, while adding upgrade rules to the upgrader and changelogs to note that these are changing in 5.x

[szepeviktor]

You have the advantage of knowing every bit of the framework.

[robbieaverill]

I wish I did 😆

@silverstripe/core-team how would you like to handle the transition to strict typing? This PR goes a long way towards it already. There are cases we'll need to change, and in those can we take the happy path return type and use that? E.g. if (!$bar) { return false; } else return 'hello world'; we can adjust to returning empty strings. Is documenting this in the upgrade guide sufficient, or do we need to add automated upgrader steps for it as well?

[szepeviktor]

return+else may never execute :)

[szepeviktor]

One (hard) way is to run unit tests and get one error.

PHPStan (on a certain higher level) may give you a nice list of all errors.

[micmania1]

Can we apply a bit of caution and fully understand the effects of this before merging? This is effectively going to break every module and project built with SilverStripe.

Since the API wasn't built with this in mind, my worry is that it affects us in much bigger ways than we're expecting here. ie. we've allowed concrete classes to be replaced by injector without any type checking in the past. This probably requires genuine refactoring rather just running an upgrader tool.

For reference, there is some push back from implementing strict typing throughout Laravel: https://twitter.com/taylorotwell/status/1139501976180285442

[szepeviktor]

It is known that there are two opposing groups of developers. One group builds its robust code on strict typing, the other group wishes to be done asap.

[sminnee]

In general, stricter usage of types would be a good thing. To your point about class overrides, those should either be subclasses of the class be overridden, or the service definition should be an interface, not a class.

However, in SS4 I don't think we want to change the return types of our public API, even if doing so would make the return types more consistent.

We could consider this for SS5, although we'd only want to do so if the upgrade impact on developers would be minimal.

In the case of the SS_BASE_URL example, a change like this would be fine for SS4, rather than making Enviornment::getEnv() consistently return a string. Frankly, regardless of return type, this should probably be the code - passing an empty value to parse_url to trigger a no-op isn't great.

diff --git a/src/Control/CLIRequestBuilder.php b/src/Control/CLIRequestBuilder.php
index 73a627343..2d33a4a49 100644
--- a/src/Control/CLIRequestBuilder.php
+++ b/src/Control/CLIRequestBuilder.php
@@ -78,9 +78,12 @@ class CLIRequestBuilder extends HTTPRequestBuilder
     {
         $request = parent::createFromVariables($variables, $input, $url);
         // unset scheme so that SS_BASE_URL can provide `is_https` information if required
-        $scheme = parse_url(Environment::getEnv('SS_BASE_URL'), PHP_URL_SCHEME);
-        if ($scheme) {
-            $request->setScheme($scheme);
+        $baseUrl = Environment::getEnv('SS_BASE_URL');
+        if ($baseUrl) {
+            $scheme = parse_url(Environment::getEnv('SS_BASE_URL'), PHP_URL_SCHEME);
+            if ($scheme) {
+                $request->setScheme($scheme);
+            }
         }
 
         return $request;

Rolling out declare(strict_types) seems like a good idea, but it will require a fair bit of work. As @szepeviktor hints at - this is a codebase going back to 2005 and at that time PHP wasn't really a language that worried about type hygiene (and it's lead dev was a lot less experienced :p).

In general we should look to introduce type hygiene but not at the cost of massive upgrade pain.

[unclecheese]

Per @robbieaverill 's comment on the original issue:

Yes please, into the master branch :-)

Can we retarget this?

[sminnee]

Perhaps let's resolve the discussion of what we plan to do here before going through the rigamole of re-targeting.

Hang on. I would note that declare(strict_types) isn't an API breakage. Making our APIs return consistent types would be, and it might simplify the creation of code to comply with strict_typing, but in some ways I'd prefer a partial application of strict_types in 4 that we polish in 5, rather than a massive divergence between 4 and 5 on this point.

[szepeviktor]

This is how to replicate this PR

composer require slevomat/coding-standard
vendor/bin/phpcbf --standard=vendor/slevomat/coding-standard/SlevomatCodingStandard --sniffs=SlevomatCodingStandard.TypeHints.DeclareStrictTypes src/ tests/

then revert src/View/SSTemplateParser.php

[sminnee]

So one issue with introducing this is that without a lot of guards and type coercion, badly-typed inputs from 3rd party code will then throw issues. We have a couple of choices:

• Introduce in SS4 with appropriate type coercions / guards. This at least makes it explicit where type coercions are occurring.
• Introduce in SS5 and expect 3rd party code to be tidier in what it passes

The most common "badly typed input" appears to be that a null/empty value is passed when a string is expected. So a lot of it comes down to adding if (!$input) return null at the start. Which, in itself is a bit of an improvement as a bunch of unnecessary code isn't being executed. In these cases I wouldn't expect type coercions to be unnecessary even in SS5.

Here's a sample the changes needed - these are the minimum changes needed to get DatabaseTest to pass.

https://github.com/sminnee/silverstripe-framework/commit/1c82eb89d58b3ebcb7eaa67b163c853bddc9e6f5

[szepeviktor]

Developing a widely used framework may be very-very hard. I think most of the users do not think about variable types. PHPStan will complain 1000× This is already a string, why checking?

[sminnee]

...broadening this to get the whole of ORM tests passing wasn't too bad.
https://github.com/sminnee/silverstripe-framework/commit/5361aa7fed39b6d4abe4bc4e53d9bf92b3b93696

[chillu]

...broadening this to get the whole of ORM tests passing wasn't too bad.

We generally don't run tests with unexpected input types which are currently coerced by PHP though, so I'm not sure how much that's proving.

Introduce in SS4 with appropriate type coercions / guards. This at least makes it explicit where type coercions are occurring.

Going through the entire framework or supported modules API surface (both public and protected, due to subclassing/DI) seems like a giant amount of work? We can't really use a fuzzer with varying inputs either, because it would have no idea on how to correctly instanciate classes. We could infer from PHPDoc @param and @return hints on what the correct type should be, but that's still a lot of checks on the top of each method. And it makes the code a lot messier.

Introduce in SS5 and expect 3rd party code to be tidier in what it passes

How is this discussion related to return type declarations (PHP 7.1 and newer) and argument type declarations (PHP 7.0 and newer). Since we require PHP 7.1 as of SS 4.5, we can now introduce those.

It looks like we can retroactively add those type declarations without breaking subclasses or existing function invocations (unless they're using the wrong type already of course). This code doesn't throw on PHP 7.3:

<?php
class C {}

class A {
	function foo(int $int, C $c) {
		// ...
	}
}
class B extends A {
	function foo($int, $c) {
		// ...
	}	
}

$c = new C();

$a = new A();
$a->foo(1, $c);

$b = new B();
$b->foo(1, $c);

Note that strict_types does not complain about type coercion itself unless those type declarations are in place.

<?php
declare(strict_types=1);

function foo($int) {
	var_dump($int * 2); // outputs 2, only throws with an explicit type declaration on the argument
}
foo(true);

[chillu]

Since we supported PHP 5.x until now in SS 4.x, the only valid type declarations in our code can be Class/Interface, array, self and callable (see reference). Other scalar type defintions such as bool and string were ony introduced in PHP 7.0 and PHP 7.1.

So the question is: How much breakage do we expect from usage of this subset of existing type declarations?

• Class/Interface: That already throws without strict_types if you pass in a wrong instance or null
• array: There's 100+ matches for function .*array\s in vendor/silverstripe. Passing Iterable might work in most cases, but already throws without strict_types. Passing string also throws without strict_types.
• self: Not very common?
• callable: function .*callable\s in vendor/silverstripe returns 36 matches. Already throws without strict_types.

So in conclusion, I think we might be able to do set strict_types in 4.x. But: I don't think we can add more type declarations to existing APIs in 4.x, regardless what explicit coercions we would do on top of the method. Using the wrong type will throw before reaching that code. Since the existing type declarations already throw without strict_types, I don't see how adding that in 4.x will have much effect, unless we think that adding more type declarations doesn't constitute a semver violation. Note that "Add type hint to an argument" isn't allowed in Symfony for minor releases.

Some more test cases below (executed on PHP 7.3)

<?php
// class type checks - throws
class B {}
class C {}
function foo(B $b) {
}
$c = new C();
foo($c);

// callable type checks - throws
function foo(callable $fn) {
	$fn();
}
$fn = function() {
	echo 'foo';
};
foo(null);

// array type checks - throws
class MyIterator implements IteratorAggregate
{

	private $items = [1,2,3];

	public function getIterator() {
		return new ArrayIterator($this->items);
    }

}
function foo(array $arr) {
	foreach($arr as $item) {
		echo $item;
	}
}
foo(new MyIterator());

// array string type checks - throws
function foo(array $arr) {
	foreach($arr as $item) {
		echo $item;
	}
}
foo('123');

[szepeviktor]

It is very easy to show what strict types hold for us: execute these 2 commands #9101 (comment) on the framework and try running the unit tests. Every breakage will show what type difference strict types prevents.

my first one was ini_set() 9d8f6e1

[sminnee]

Note that "Add type hint to an argument" isn't allowed in Symfony for minor releases.

Yeah, I agree with this. Notably it throws E_STRICT errors for people overriding such methods in a subclass. We could add it for new APIs, and I think we could get away with adding it in SS5, especially if we could automatically rewrite any method override definitions.

Adding declare(strict_types) is a bit different, in that it effectively puts type-hints on builtin methods for calls to those methods.

Naïvely it seems like it's not going to break APIs, because it only affects calls made from the file in which the declaration is set. The problem comes with cases such as this:

Imagine this is our legacy code:

<?php

function fiveLengths($input) {
  return strlen($input) * 5;
}

fiveLengths("hello"); // 25
fiveLengths(234); // 15

Adding declare_strict(types) is tantamount to putting a type hint on fiveLengths:

<?php
declare(strict_types = 1);

function fiveLengths($input) {
  return strlen($input) * 5;
}

fiveLengths("hello"); // 25
fiveLengths(234); // error

Which you can avoid with appropriate guards:

<?php
declare(strict_types = 1);

function fiveLengths($input) {
  return strlen((string)$input) * 5;
}

fiveLengths("hello"); // 25
fiveLengths(234); // error

This, however, reduces the benefit, although we could still be fussy about types in internal code. But the full benefit really comes if you tidy up your type hinting as a major change:

<?php
declare(strict_types = 1);

function fiveLengths(string $input): int {
  return strlen($input) * 5;
}

fiveLengths("hello"); // 25
fiveLengths(234); // error

[sminnee]

I've changed the title of this as I believe the previous title was confusing people :p

[sminnee]

I wonder if it's better to complete #9103 before picking this up? It'll be easier to dive into strict type compliance with phpstan there to support us...

[GuySartorelli]

Closing this PR.
There are a lot of merge conflicts so this would have to be done effectively from scratch anyway, and it may not be the approach we choose for introducing strong typing.
If we do something like this going forward, it will be a new piece of work.