Skip to content

Commit

Permalink
Backport PHP 7.4 Support (#29842)
Browse files Browse the repository at this point in the history
Co-authored-by: Dries Vints <dries.vints@gmail.com>
  • Loading branch information
2 people authored and taylorotwell committed Sep 3, 2019
1 parent 7b39e08 commit dbb3b8c
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 12 deletions.
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ matrix:
- php: 7.3
- php: 7.3
env: SETUP=lowest
- php: 7.4snapshot

cache:
directories:
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Http/Resources/DelegatesToResource.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public function resolveRouteBinding($value)
*/
public function offsetExists($offset)
{
return array_key_exists($offset, $this->resource);
return isset($this->resource[$offset]);

This comment has been minimized.

Copy link
@matthewnessworthy

matthewnessworthy Sep 4, 2019

Contributor

Cannot use object of type Results\ListingResult as array

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

How did this compare with array_key_exists? Did it allow non-arrays?

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

@matthewnessworthy could you make a full issue on the tracker please, so we can discuss fixing this?

This comment has been minimized.

Copy link
@matthewnessworthy

matthewnessworthy Sep 4, 2019

Contributor

@GrahamCampbell issue created #29858

array_key_exists allows array|ArrayAccess, and it would appear that for backwards compatibility reasons, objects

https://www.php.net/manual/en/function.array-key-exists.php

For backward compatibility reasons, array_key_exists() will also return TRUE if key is a property defined within an object given as array. This behaviour should not be relied upon, and care should be taken to ensure that array is an array.

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

Right, and we're not specified that resource must be array or ArrayAccess, so we need to support objects. I'll prep a PR.

This comment has been minimized.

Copy link
@matthewnessworthy

matthewnessworthy Sep 4, 2019

Contributor

Great, thanks @GrahamCampbell! Sorry for the trouble.

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

Hmmmm, I'm not sure ever actually intended to support objects that were not either array accessable, or arrays. One could argue this is not a breaking change.

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

Yeh, like, look at the other methods:

image

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

All of these will be f***ed unless you have something that is an array, or array accessible.

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

On the other hand, one could argue those other methods arn't implemented correctly...

This comment has been minimized.

Copy link
@matthewnessworthy

matthewnessworthy Sep 4, 2019

Contributor

there's also a functional difference between isset and array_key_exists, in that array_key_exists does not do any value checking

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Sep 4, 2019

Author Member

I think the correct fix is going to be to do: #29860. This is a change against the original behaviour, but the original behaviour where properties were checked was incorrect.

This comment has been minimized.

Copy link
@matthewnessworthy

matthewnessworthy Sep 4, 2019

Contributor

I gave the change a quick test drive and I'm not experiencing any issues.

This comment has been minimized.

Copy link
@driesvints

driesvints Sep 4, 2019

Author Member

@matthewnessworthy Is Results\ListingResult extending the base resource? Is Results\ListingResult an implementation of ArrayAccess?

This comment has been minimized.

Copy link
@matthewnessworthy

matthewnessworthy Sep 4, 2019

Contributor

@driesvints it is not extending the base resource, nor does it implement ArrayAccess

This comment has been minimized.

Copy link
@matthewnessworthy

matthewnessworthy Sep 4, 2019

Contributor

It does implement Illuminate\Contracts\Support\Arrayable, but that just provides for the toArray public function

This comment has been minimized.

Copy link
@driesvints

driesvints Sep 5, 2019

Author Member

@matthewnessworthy does your resource extends the JsonResource object? All resources need to extend that.

This comment has been minimized.

Copy link
@driesvints

driesvints Sep 5, 2019

Author Member

Let's take this discussion to the issue you opened. That's a little easier than on a commit message.

}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/Illuminate/Support/Facades/Facade.php
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,9 @@ protected static function resolveFacadeInstance($name)
return static::$resolvedInstance[$name];
}

return static::$resolvedInstance[$name] = static::$app[$name];
if (static::$app) {
return static::$resolvedInstance[$name] = static::$app[$name];
}
}

/**
Expand Down
10 changes: 6 additions & 4 deletions src/Illuminate/Support/Traits/Macroable.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,13 @@ public static function __callStatic($method, $parameters)
));
}

if (static::$macros[$method] instanceof Closure) {
return call_user_func_array(Closure::bind(static::$macros[$method], null, static::class), $parameters);
$macro = static::$macros[$method];

if ($macro instanceof Closure) {
return call_user_func_array(Closure::bind($macro, null, static::class), $parameters);
}

return call_user_func_array(static::$macros[$method], $parameters);
return $macro(...$parameters);
}

/**
Expand All @@ -110,6 +112,6 @@ public function __call($method, $parameters)
return call_user_func_array($macro->bindTo($this, static::class), $parameters);
}

return call_user_func_array($macro, $parameters);
return $macro(...$parameters);
}
}
2 changes: 1 addition & 1 deletion tests/Integration/Migration/MigratorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public function test_dont_display_output_when_output_object_is_not_available()

$migrator->run([__DIR__.'/fixtures']);

$this->assertTrue($this->tableExists('members'));
$this->assertTrue($this->tableExists('people'));
}

private function tableExists($table): bool
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
<?php

namespace Illuminate\Tests\Integration\Migration\fixtures;

use Illuminate\Support\Facades\Schema;
use Illuminate\Database\Schema\Blueprint;
use Illuminate\Database\Migrations\Migration;

class CreateMembersTable extends Migration
class CreatePeopleTable extends Migration
{
/**
* Run the migrations.
Expand All @@ -15,7 +13,7 @@ class CreateMembersTable extends Migration
*/
public function up()
{
Schema::create('members', function (Blueprint $table) {
Schema::create('people', function (Blueprint $table) {
$table->increments('id');
$table->string('name');
$table->string('email')->unique();
Expand All @@ -32,6 +30,6 @@ public function up()
*/
public function down()
{
Schema::drop('members');
Schema::drop('people');
}
}

0 comments on commit dbb3b8c

Please sign in to comment.