davedevelopment / phpmig

Simple migrations system for php
Other
569 stars 92 forks source link

fixed PHP Warning in Illuminate/Database adapter #111

Closed michael-mader closed 8 years ago

michael-mader commented 8 years ago

in fetchAll method array_map is called with the result of this statement:

$all = $this->adapter
  ->table($this->tableName)
  ->orderBy('version')
  ->get();

But get() returns an instance of \Illuminate\Support\Collection which is not accepted by array_map. There a call to toArray() has to be added to convert the Collection to an array

davedevelopment commented 8 years ago

Thanks!

ricardorzgz commented 8 years ago

Hi, this is not right, the table() function returns an instance of Query\Builder so the get returns an array not a Collection, so after this commit this package is broken for Laravel. The toArray() function is only available for the Eloquent Builder.

PHP Fatal error: Call to a member function toArray() on a non-object in vendor/davedevelopment/phpmig/src/Phpmig/Adapter/Illuminate/Database.php on line 47

michael-mader commented 8 years ago

hm what version of Illuminate/Database do you use? maybe there was a change?

edit: It is working with version 5.3

ricardorzgz commented 8 years ago

I'm working with 5.2.45, and

$this->adapter
  ->table($this->tableName)
  ->orderBy('version') 

is returning a Illuminate\Database\Query\Builder so cannot use toArray() there.

michael-mader commented 8 years ago

ok, so we have to check what is returned and only call toArray() for \Illuminate\Support\Collection.

Is it working if you remove toArray()? Is the Builder automatically converted to array?

ricardorzgz commented 8 years ago

Yes, if I remove toArray() is working. Query Builder returns an array

michael-mader commented 8 years ago

can you provide a new pull request fixing this issue for both versions? Maybe we should check if get() returns an array, if not call toArray() to convert it. This should work for you and for me

ricardorzgz commented 8 years ago

Done