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

PHP 8.4: Implicitly nullable parameter declarations deprecated #50803

Closed
jnoordsij opened this issue Mar 28, 2024 · 6 comments · Fixed by #50922
Closed

PHP 8.4: Implicitly nullable parameter declarations deprecated #50803

jnoordsij opened this issue Mar 28, 2024 · 6 comments · Fixed by #50922
Assignees

Comments

@jnoordsij
Copy link
Contributor

Laravel Version

any

PHP Version

8.4.0

Database Driver & Version

No response

Description

As of PHP 8.4, implicitly nullable parameter declarations will be deprecated. See also https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated and https://wiki.php.net/rfc/deprecate-implicitly-nullable-types.

Currently, a large number of functions within this codebase still use this pattern; see also a quick example I made with rector. This should probably be adjusted.

Implementation suggestions:

  • for maintenance reasons, it might be most fruitful to target any update at 10.x, to also prevent lots of deprecations in the few months where v10 and PHP 8.4 are both supported;
  • StyleCI has a nullable_type_declarations fixer that can automatically fix and enforce this, which is probably the easiest way to automatically fix this;
  • maybe for other people using StyleCI, adding this to the laravel preset might be a good way forward?

Steps To Reproduce

N/A

@driesvints driesvints self-assigned this Mar 28, 2024
@driesvints
Copy link
Member

It's highly unlikely we'll support PHP 8.4 on Laravel v10. But if it's not too much work and there aren't any breaking changes we can give it a go. I'll try to get a CI job up soon.

@jnoordsij
Copy link
Contributor Author

It's highly unlikely we'll support PHP 8.4 on Laravel v10. But if it's not too much work and there aren't any breaking changes we can give it a go. I'll try to get a CI job up soon.

It is functionally equivalent and just guessed it is probably easier to maintain by using same styling across both branches, but if it turns out that's not the case then definitely updating just 11.x sounds fine. Moreover it's only a deprecation and not a require change.

@plumthedev
Copy link
Contributor

It is breaking change as method signature will be changed and all child classes which are overriding affected methods will throw a fatal error.

// framework class
interface Service
{
	public function func(?DateTime $date): void;
}

// application class
class FooService implements Service
{
	public function func(DateTime $date): void
	{
		echo $date->format('Y');
	}
}

// Fatal error: Declaration of FooService::func(DateTime $date): void must be compatible with Service::func(?DateTime $date): void 
(new FooService())->func(new DateTime('now'));

@korkoshko
Copy link
Contributor

@plumthedev This isn't a BC. Your example isn't entirely accurate in this case. This applies only to parameters that default to null (they implicitly become nullable). So, int $foo = null is equal to ?int $foo = null.

Before:

// framework class
interface Service
{
	public function func(DateTime $date = null): void;
}

// application class
class FooService implements Service
{
	public function func(DateTime $date = null): void
	{
		echo $date->format('Y');
	}
}

(new FooService())->func(new DateTime('now'));

After:

// framework class
interface Service
{
	public function func(?DateTime $date = null): void;
}

// application class
class FooService implements Service
{
	public function func(DateTime $date = null): void
	{
		echo $date->format('Y');
	}
}

(new FooService())->func(new DateTime('now'));

@plumthedev
Copy link
Contributor

Indeed you are right @korkoshko - thanks for your eagle eye 😄

@crynobone
Copy link
Member

PR has been merged to 11.x branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants