-
Notifications
You must be signed in to change notification settings - Fork 11k
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
[6.x] PHP 7.4 support #29482
[6.x] PHP 7.4 support #29482
Conversation
Tests are green but framework/src/Illuminate/Foundation/Providers/FoundationServiceProvider.php Lines 50 to 66 in c9f0144
I mentioned this in the Discord #internals channel, but PHP 8.0 doing this is a massive breaking change to Laravel's Collection macros are dead?By only changing the This macro in Laravel 5.8: Collection::macro('takeWhile', function (callable $callback) {
$items = [];
foreach ($this->items as $key => $item) {
if (!$callback($item, $key)) {
break;
}
$items[$key] = $item;
}
return new static($items);
}); in Laravel 6.0 would have to be: Collection::macro('takeWhile', static function ($collection, callable $callback) {
$items = [];
foreach ($collection->all() as $key => $item) {
if (!$callback($item, $key)) {
break;
}
$items[$key] = $item;
}
return new static($items);
}); The only way that could work is to conditionally change the parameters passed into the closure depending on if you're in a if ($macro instanceof Closure) {
if ($this === null) {
return call_user_func_array($macro->bindTo($this, static::class), $parameters);
}
return call_user_func_array($macro->bindTo($this, static::class), array_merge($this, $parameters));
} |
Can someone link to further explanation about this Closure breaking change? Will Closure macros even be possible anymore? |
They're completely possible and would still work as intended. That part of this PR doesn't seem correct. PHP 7.4 warns against the removal of $this from normal Closures but you can still change See this for a more detailed explanation in code: https://3v4l.org/3OKGT |
I've pushed a new commit but it now throws a segmentation fault 🤔 https://travis-ci.org/driesvints/framework/jobs/570839855 |
if (static::$macros[$method] instanceof Closure) { | ||
return call_user_func_array(Closure::bind(static::$macros[$method], null, static::class), $parameters); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean static::
can no longer be used inside static macros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this change is merged then that won't be possible no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no familiarity with this macro concept, but I'm wondering if you could just rebind the scope here while leaving $this
alone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic hmm not sure what the method call for that would be. Afaik both bind
and bindTo
require $newthis
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit ugly, but you could try passing (new ReflectionFunction($closure))->getClosureThis()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked for me: https://gist.github.com/nikic/a6e97696d1351261951edeac97ef211a At least I'm not getting any test failures that look related (there are some in Carbon, but those use different code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic I think that might be because the failing test was removed because at first we thought this couldn't be supported anymore. I've pushed a new commit with these changes and re-added the test which is now failing. (force pushed this, sorry)
Failing build: https://travis-ci.org/laravel/framework/builds/571421063
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. The issue is that you're using static::
rather than self::
, and static
is determined by get_class($this)
if $this
exists, with the bound scope not playing into it.
I think we need to relax this deprecation to only the cases where $this
is actually referenced inside the closure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should just ignore this issue for now -- I plan to relax the deprecation on the PHP side, after which (I think) no changes in Laravel should be needed anymore. You are only trying to unbind $this
in cases where it is not used inside the closure, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikic the test still fails if I change static::
to self::
on this line it still fails. But that shouldn't be necessary since we're trying to make sure static::
calls in closures still work.
You are only trying to unbind $this in cases where it is not used inside the closure, right?
Correct yes. When using static::
it could technically still be used in a non-static context as well but when you're calling the macro statically then there shouldn't be a concept of $this
inside the closure.
When would these changes be implemented? We're planning to release 6.0 end of this month or early september. Depending on this I'd send the rest of this PR to the 5.8 branch already.
Thank you for all your work on this btw :)
Regarding the segfault:
This looks like fallout from some recent GC changes, I think I know what cause this... |
@driesvints Yup, leaving I've submitted #29535 for |
The segfault should be fixed in the next nightly build. |
I wonder if we should also bring these fixes into Laravel 5.8? They don't seem too heavy. |
@GrahamCampbell don't think it's worth the effort. 5.8 won't be supported anymore after this month. |
Like @GrahamCampbell said, these fixes don't seem too heavy and I personally think that it's weird that the current latest Laravel framework version that you can get (which was released just a couple of months ago) won't support the next minor PHP version which is due to be released in just two-three months. |
Okay. I'll backport these once this is merged. I'd like to keep this PR open for now to continue the discussion. |
What? Existing static macros will not be backward compatibles, and you will add this in a minor release of 5.8? Muting the error in 5.8 to silently support PHP 7.4 is OK, but this is a breaking change. For proof, you removed a unit test. Unit tests are supposed to be what you can do and rely on it for the current version. As far as I understood, if you had: Str::macro('dashCamel', function ($str) {
return '--'.static::camel($str);
}); You need to upgrade it: Str::macro('dashCamel', static function ($str) {
return '--'.Str::camel($str);
}); And you loose the re-usability of the macro function since now you have to explicit the class name. If so, it seems fine to explain this in a migration notice from 5.8 to 6.0, but users will not guess this by themselves I think if you change this in a minor. |
Hey @kylekatarnls please see my conversation with @nikic here: #29482 (comment) We're trying to see if we might loosen up the new implementation a bit. |
Only deprecate unbinding of $this from a closure if $this is syntactically used within the closure. This is desired to support Laravel's macro system, see laravel/framework#29482. This should still allow us to implement the performance improvements we're interested in for PHP 8, without breaking existing use-cases.
I've merged the change to the closure $this deprecation, so you might want to give this another try. |
@nikic How do you tell if |
@GrahamCampbell Only syntactical uses of |
@nikic Does Consider the following: file.php: <?php
echo get_class($this); run.php: <?php
class Foo
{
public function bar()
{
$fn = function () { require 'file.php'; };
$fn();
}
}
(new Foo)->bar(); $ php run.php The following code will write |
@GrahamCampbell To provide some context: The reason we're interested in this deprecation is exclusively to enable some performance improvements in PHP 8 related to $this accesses, which is enabled by the removal of static calls to non-static methods. $this accesses can be split into two categories: Those where we know $this to be non-null and those where we don't. Method calls (will) fall into the former category. With this deprecation closures will also fall into the former category. $this accesses inside free-standing (e.g. required) code fall into the latter. The change we have in mind will make the former category faster and the latter slower, so we'd like closures to fall into the fast case, while we don't particularly care about uses of $this in required code. This is also why indirect uses of $this through required code in closures are not relevant here: While they do use $this, they are not directly part of the closure. From a semantic perspective it is cleaner to treat these (direct $this access and required $this access) the same, and a reasonable person may argue that it would be better to force the use of |
@nikic Ah, I see. Thanks for the detailed explanation. |
@nikic when I retrigger the build it seems that we get a new segmentation fault: https://travis-ci.org/laravel/framework/jobs/571421072 |
Those reflection errors are caused by the phpunit version being too low. PHPUnit hides itself from the stack traces, |
https://github.com/driesvints/framework/pull/3 seems to fix the builds. 🎉 |
@GrahamCampbell ah sorry. Didn't see your comments. Was figuring it out myself in the meantime :') |
Build is finally passing! 🥳 I've had to bump some minimum versions of packages that implemented PHP 7.4 support:
Thanks @nikic and everyone else in this thread for your help! |
@GrahamCampbell can you dismiss your review or approve if everything is okay? :) |
🚢 |
* Add PHP 7.4 build * Fix array_key_exists call on object * Fix CreateMembersTable resolving * Fix array call on null for Facade * Rebind $this for static closure macros * Try reverting unbinding closure * Use argument unpacking * Require new minimum mockery version * Fix PHP 7.4 support for email package * Make Symfony 4.3.4 the new minimum requirement * Bump PHPUnit to 8.3 * Remove 7.4 build from failures
This fixes our own PHP 7.4 build failures. PHP 7.4 is out in November so it's best that we make use of this major release to do the necessarily breaking changes already.
There's two remaining failures:
ArrayInput
from Symfony which is already fixed ([Console] fix warning on PHP 7.4 symfony/symfony#32806). Build will be fixed once Symfony 4.3.4 is out.$this
of closure is deprecated within Carbon which @kylekatarnls will need to fix. I'd like to send in a PR but I think he will probably figure out the internals much faster than me.Problems fixed with this PR:
$this
for closures (c9f0144). Symfony fixed these problems as well here: [Cache] fix warning on PHP 7.4 symfony/symfony#32796The unbinding of closures is the most significant one. Basically no internals in Laravel were changed. In PHP 7.4 people will need to use the
static
keyword when they want to allow to statically call closure macros. But this is a language limitation and not a Laravel one.Fixes #29411