yajra / laravel-oci8

Oracle DB driver for Laravel via OCI8
https://yajrabox.com/docs/laravel-oci8
MIT License
830 stars 237 forks source link

Issue hasColumn function #175

Closed andrei0x309 closed 8 years ago

andrei0x309 commented 8 years ago

I am using the latest version yajra/laravel-oci8 (v5.2.7), and i am also using zofe/rapyd-laravel that uses CRUD, on update i got this error:

ErrorException in Builder.php line 69:
strtolower() expects parameter 1 to be string, object given
in Builder.php line 69
at HandleExceptions->handleError('2', 'strtolower() expects parameter 1 to be string, object given', '/var/www/diz/vendor/laravel/framework/src/Illuminate/Database/Schema/Builder.php', '69', array('table' => 'utilizatori', 'column' => 'nume'))
at strtolower(object(stdClass))
at array_map('strtolower', array(object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass), object(stdClass))) in Builder.php line 69

So for a quick fix i added hasColumn($table, $column) in laravel-oci8\src\Oci8\Schema\OracleBuilder.php:

 public function hasColumn($table, $column)
    {
        $column = strtolower($column);

        $arryOfObj = $this->getColumnListing($table);
        $array = [];
        foreach($arryOfObj as $obj){
            $array[] = $obj->column_name;
        }
        return in_array($column, array_map('strtolower', $array ));
    }

With that added the issue was solved, I think you should consider implementing hasColumn or change getColumnListing method.

yajra commented 8 years ago

@azrael-sub7 Can you please submit a PR using your quick fix? And also please provide some snippets on how to replicate the issue? I am not familiar with the package you are using but will try to have a look at it. Thanks!

andrei0x309 commented 8 years ago

From what I can see the issue is that laravel Schema Builder hasColumn function expects getColumnListing to return an array with strings ( the columns) but instead getColumnListing is returning an array of objects.

So the simplest way to replicate the error is just by calling hasColumn without using any custom packages

 \Schema::hasColumn('colete', 'fid_destinatar');

Here is an example of what getColumnListing is returning now:

array:15 [▼
  0 => {#399 ▼
    +"column_name": "EMAIL_VERIFICAT"
  }
  1 => {#400 ▼
    +"column_name": "TELEFON_VERIFICAT"
  }
  2 => {#401 ▼
    +"column_name": "SYS_NC00013$"
  }
  3 => {#402 ▼
    +"column_name": "UPDATED_AT"
  }
  4 => {#403 ▼
    +"column_name": "CREATED_AT"
  }
  5 => {#404 ▼
    +"column_name": "FID_DESTINATAR"
  }
  6 => {#405 ▼
    +"column_name": "TELEFON"
  }
  7 => {#406 ▼
    +"column_name": "DREPTURI"
  }
  8 => {#407 ▼
    +"column_name": "REMEMBER_TOKEN"
...
...
...

Here is an example of what hasColumn expects from getColumnListing:

array:15 [▼
  0 => "EMAIL_VERIFICAT"
  1 => "TELEFON_VERIFICAT"
  2 => "SYS_NC00013$"
  3 => "UPDATED_AT"
  4 => "CREATED_AT"
  5 => "FID_DESTINATAR"
  6 => "TELEFON"
  7 => "DREPTURI"
  8 => "REMEMBER_TOKEN"
  9 => "LOCALE"
  10 => "password"
  11 => "EMAIL"
  12 => "PRENUME"
  13 => "NUME"
  14 => "PID"
]

Also, getColumnListing should not include entities that are not columns like: SYS_NC00013, but that is a minor bug wich in most cases can be ignored.

I have submitted a PR that should fix the issue, just a overwrite of processColumnListing, in fact, is the same as in MySQL driver i have tested it in my project and it works, it should be better than the previous fix.

Thanks for your attention to this matter.

mstaack commented 8 years ago

@yajra seems legit. maybe we need a test for that

yajra commented 8 years ago

Just tested, merged and released your PR on v5.2.8. Thanks a lot and sorry for the delayed response.

mohd-aidi commented 6 years ago

Hi, i happen to encounter this problem too..i use version "yajra/laravel-oci8": "5.1.*"

can you apply the fix for this version too?

yajra commented 6 years ago

Oh sure, I think this was fixed on PR #176. Will submit a PR and please test. Thanks!