[5.8] Fix isset if not array or ArrayAccess

[GrahamCampbell]

No description provided.

[taylorotwell]

This feels hard to read. What are you trying to check?

[taylorotwell]

Why did we stop using array_key_exists in the first place? Was it a PHP 7.4 reason?

[jasonvarga]

I haven't wrapped my head around the change yet, but I can confirm that upgrading from 5.8.34 to 5.8.35 broke because of this change (which this PR fixes, at least for me)

[taylorotwell]

In what situation is $offset an array?

I feel like we need to better understand this issue and not just throw code against the wall hoping that is sticks.

[jasonvarga]

Its $this->resource that's the issue.

See this comment

Cannot use object of type Results\ListingResult as array

If your resource doesn't implement ArrayAccess, you now get an error.
When it was using array_key_exists, it would just return false and continue without an issue.

[taylorotwell]

How does yours not implement ArrayAccess? Are you extending the built-in Laravel resource stuff?

[jasonvarga]

return Illuminate\Http\Resources\Json\Resource::collection($items);

where $items is a collection of Arrayable (but not ArrayAccess) objects.

To be fair, the items are not Eloquent models, but it worked just fine.

[taylorotwell]

Can someone post an entire stack trace so we can see where this is even being called from and get a better idea of the overall use case?

[matthewnessworthy]

There are a couple of cases where the Resource is instantiated with a non array, non ArrayAccess object

[taylorotwell]

@matthewnessworthy Like? Where? When? How? We need more details.

[driesvints]

Where does the offsetExists call happen?

[jasonvarga]

I misspoke - $items in my comment above is a LengthAwarePaginator.

The stack trace looks like:

[2019-09-04 20:51:57] local.ERROR: Cannot use object of type Statamic\Data\Entries\Entry as array {"userId":"jason","exception":"[object] (Symfony\\Component\\Debug\\Exception\\FatalThrowableError(code: 0): Cannot use object of type Statamic\\Data\\Entries\\Entry as array at /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Http/Resources/DelegatesToResource.php:53)
[stacktrace]
#0 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Support/Arr.php(148): Illuminate\\Http\\Resources\\Json\\JsonResource->offsetExists('resource')
#1 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Support/helpers.php(524): Illuminate\\Support\\Arr::exists(Object(Illuminate\\Http\\Resources\\Json\\Resource), 'resource')
#2 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Support/Arr.php(387): data_get(Object(Illuminate\\Http\\Resources\\Json\\Resource), Array)
#3 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Support/Collection.php(1107): Illuminate\\Support\\Arr::pluck(Array, Array, NULL)
#4 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Support/Traits/ForwardsCalls.php(23): Illuminate\\Support\\Collection->pluck('resource')
#5 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Pagination/AbstractPaginator.php(647): Illuminate\\Pagination\\AbstractPaginator->forwardCallTo(Object(Statamic\\Data\\Entries\\EntryCollection), 'pluck', Array)
#6 /users/jason/code/statamic/three/cms/app/Extensions/Pagination/LengthAwarePaginator.php(34): Illuminate\\Pagination\\AbstractPaginator->__call('pluck', Array)
#7 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Http/Resources/Json/PaginatedResourceResponse.php(28): Statamic\\Extensions\\Pagination\\LengthAwarePaginator->__call('pluck', Array)
#8 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Support/helpers.php(1124): Illuminate\\Http\\Resources\\Json\\PaginatedResourceResponse->Illuminate\\Http\\Resources\\Json\\{closure}(Object(Illuminate\\Http\\JsonResponse))
#9 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Http/Resources/Json/PaginatedResourceResponse.php(31): tap(Object(Illuminate\\Http\\JsonResponse), Object(Closure))
#10 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Http/Resources/Json/ResourceCollection.php(71): Illuminate\\Http\\Resources\\Json\\PaginatedResourceResponse->toResponse(Object(Illuminate\\Http\\Request))
#11 /Users/jason/Code/statamic/three/laravel/vendor/laravel/framework/src/Illuminate/Routing/Router.php(733): Illuminate\\Http\\Resources\\Json\\ResourceCollection->toResponse(Object(Illuminate\\Http\\Request))

[taylorotwell]

It's a LengthAwarePaginator of plain PHP objects? No Eloquent stuff, etc?

[jasonvarga]

That's right, they are not Eloquent models.

[taylorotwell]

Hmm, OK. We may have to add this to the upgrade guide that all underlying objects of resources should implement ArrayAccess. I think patching around that may hide other problems.

[driesvints]

@jasonlbeggs can you post the code of that object?

[GrahamCampbell]

For Laravel 5.8, at least, we shouldn't break code.

[jasonlbeggs]

@driesvints, I think you meant to tag @jasonvarga?

[jasonvarga]

I spoke to Dries outside of this thread, and landed on if you're not putting actual Resource objects in there, it was basically being misused. If it was working, it was a coincidence.

Might apply to @matthewnessworthy 's issue too.

[driesvints]

@jasonlbeggs yes, sorry!

@jasonvarga yeah exactly. All resource objects should extend the JsonResource class (as it's shown in the docs). If you do that then you'll never run into this issue.

@GrahamCampbell @taylorotwell I think in this case this PR can be closed.

[GrahamCampbell]

The phpdoc should be changed from mixed to JsonResource then.

[driesvints]

@GrahamCampbell no. It's the property that needs to be an instance of JsonResource. Not $offset.

[GrahamCampbell]

Yeh, exactly. That property has type documented as mixed. It should be JsonResource.

[driesvints]

@GrahamCampbell ah yeah. Feel free to send in a PR 👍

[GrahamCampbell]

Hmmm, why would someone construct a JsonResource with a JsonResource?

[GrahamCampbell]

https://github.com/laravel/framework/blob/v5.8.35/src/Illuminate/Http/Resources/Json/JsonResource.php#L48-L57

[driesvints]

@GrahamCampbell resources' collections work a little weird and are needed for this I think

[GrahamCampbell]

Heh. Maybe a note in the docs is sufficient.

[lbavendanos]

I believe that the result obtained when using Query Builder should be a class that implements ArrayAccess and not a stdClass, since not everyone uses Paginator from a Model Eloquent. It is for this reason that it also throws an error when using Resources.