Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Fixes for PHP 8.1 #37101

Merged
merged 9 commits into from Apr 23, 2021
Merged

[8.x] Fixes for PHP 8.1 #37101

merged 9 commits into from Apr 23, 2021

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Apr 23, 2021

Fixes passing null to non-nullable parameters.

Comment on lines -23 to +25
* @var string
* @var ?string
*/
protected $connection;
protected $connection = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the default in the constructor, which is bypassed by mocks. The same goes for $prefix.

}

if (static::$connectionFailedOnceWithDefaultsSkip) {
$this->markTestSkipped('Trying default host/port failed, please set environment variable REDIS_HOST & REDIS_PORT to enable '.__CLASS__);

return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

markTestSkipped() has @psalm-return never-return annotation. There are 48 occurrences of this method in Laravel tests and only those two are followed by a return.

@taylorotwell taylorotwell merged commit 8765ffe into laravel:8.x Apr 23, 2021
@driesvints
Copy link
Member

@jrmajor thanks a bunch for these prs 👍

@selcukcukur
Copy link
Contributor

@jrmajor If there is an update with PHP 8.1 that will require this pull request, I don't fully understand what this update is about?

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 23, 2021

@selcukcukur I'm not sure I've understood your question. PHP 8.1 will be released on November 25, but early dev builds are available for testing, and we want Laravel to be ready for it when it lands. This PR fixes passing null to non-nullable arguments of internal functions, which was deprecated in this RFC. You can see a list of other implemented proposals there.

@selcukcukur
Copy link
Contributor

selcukcukur commented Apr 23, 2021

@selcukcukur I'm not sure I've understood your question. PHP 8.1 will be released on November 25, but early dev builds are available for testing, and we want Laravel to be ready for it when it lands. This PR fixes passing null to non-nullable arguments of internal functions, which was deprecated in this RFC. You can see a list of other implemented proposals there.

I got it, I looked at this document now. So why do we translate an empty string? Wouldn't it make more sense to have these parameters do the string conversion?

Sample :

protected $foo;

public function bar(string $foo = null) 
{
     return (string) $foo;
}

or

protected $foo;

public function bar($foo = null) 
{
    return $foo ? (string) $foo : $foo;
}

or

protected $foo;

public function bar($foo = null) 
{
   if(is_null($foo) {
     return (string) $foo;
   }
}

I still don't understand why we had to make the parameter an empty string. The transformations I have shown above can be easily applied where necessary. Isn't this a better solution than the examples I've shown, am I wrong?

@jrmajor
Copy link
Contributor Author

jrmajor commented Apr 23, 2021

The first example makes no sense, no need to cast $foo to string, as it's type hinted as a string and no other value can be passed there. The second one would have a return type of ?string, which I don't think was your intention. Did you mean $foo ?: (string) $foo? The third one makes sense, though it would be easier to write as return $foo ?? ''.

Also, what's the aim of having protected $foo in your examples? It's not used anywhere.

I still don't understand why we had to make the parameter an empty string.

What parameter?

*/
protected $connection;
protected $connection = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it will throw a type exception when it takes a "null" value, we can fix it by converting it to a string. I am saying this from the following point of view. It doesn't seem nice to me to define default values for parameters in the class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Briefly, isn't it a good solution to convert a "null" value to a string instead of defining it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem nice to me to define default values for parameters in the class.

It has already had the default value and it was defined in the constructor. Using mocks, however, bypasses the constructor and this property isn't set to its default. I've written that already in this comment.

Seems pretty nice to me, that's how default properties are meant to be used. It just has been unnecessary, as it was set in the constructor anyway (except for when this class was mocked, which nobody noticed).

@jrmajor jrmajor deleted the php81/null-param branch April 24, 2021 13:51
bert-w pushed a commit to bert-w/framework that referenced this pull request Apr 29, 2021
* fix PHP 8.1 deprecations (broadcasting)

* fix PHP 8.1 deprecations (cache)

* remove returns after never-return method call

* fix PHP 8.1 deprecations (bus)

* fix PHP 8.1 deprecations (database)

* fix PHP 8.1 deprecations (validation)

* use strict comparison

* fix PHP 8.1 deprecations (mail)

* fix PHP 8.1 deprecations (routing)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants