kirkbushell / eloquence

A drop-in library for certain database functionality in Laravel, that allows for extra features that may never make it into the main project.
MIT License
537 stars 58 forks source link

Make compatible to laravel 5.5 #68

Closed stefensuhat closed 4 years ago

stefensuhat commented 6 years ago

I've tried using camel casing its not working properly even I put fillable on my model.

Laravel 5.4 working properly

bicatu commented 6 years ago

hi, I am using the camel casing with lumen 5.5 and it is working. what seems to be the issue?

kirkbushell commented 6 years ago

You'll need to show some code.

stefensuhat commented 6 years ago

@kirkbushell For example after I finished my registration I called an event UserWasLogin, send $user parameter and I called the Listeners SaveUserLastLogin.

$user = $event->user;
$user->lastLogin = Carbon::now();
$user->save();

everytime it try to saved it always return Column not found: 1054 Unknown column 'firstName' in 'field list'

In my User model:

protected $fillable = [
        'email', 'firstName', 'lastName', 'gender', 'phone', 'birthday',  'lastLogin',
 ];

All the registration is success but after entering the listeners it return an error

kirkbushell commented 6 years ago

How are you using the features in Eloquence? You haven't shown any code other than the fillable attributes. Have you followed the guide?

stefensuhat commented 6 years ago

@kirkbushell

User.php
class User extends Authenticatable
{
    use Notifiable, UuidModelTrait, HasApiTokens, CamelCasing;
}
// RegisterController.php

$user = new User($binput->all());
$name = $binput->input('full_name');
if (($pos = strpos($name, " ")) !== false) {
    $user->firstName = strtok($name, ' ');
    $user->lastName = trim(substr($name, strpos($name, " ")));
} else {
    $user->firstName = $name;
    $user->lastName = NULL;
}
$user->phone = $phone;
$user->password = $binput->input('password');
$user->referralCode = $this->generateReferralCode($user);
$user->isVerified = 1;

if($user->save()) event(new UserLoggedIn($user));
kirkbushell commented 6 years ago

Okay, do any of those other traits also overload Eloquent methods? That might be your problem.

Also a suggestion - why not simplify the form and your code by asking for first and last name? Assuming names like that will end up wrong in many cases ;)

stefensuhat commented 6 years ago

@kirkbushell other trait supposed not. In laravel 5.4 it works perfectly after upgrading it become error.

Yes the reason I complicate when save the form because I want to save customer time when fill the form.

kirkbushell commented 6 years ago

Yeah but you'er going to get names wrong by making that assumption. Believe me, much better to let the customer fill in the form that best represents their name, because if you get it wrong for marketing.etc. it looks quite unprofessional.

I'll do some more testing on 5.5, haven't had any issues so far. Have you tried doing it without the extra traits there? Also, what is Authenticatable doing?

stefensuhat commented 6 years ago

@kirkbushell Will check with my team for those input. Your solution make sense for marketing issue. 👍

Authenticable is for Laravel authentication. I used Laravel passport so it's required.

I've tried changed the UuidModelTrait to yours Uuid issue still happen

stefensuhat commented 6 years ago

May be the problem is when update.

I've tried to do this:

 $user = $event->user;

// Try to use new query        
$find = User::find($user->id);
$find->lastLogin = Carbon::now();
$find->save();

it return same error.

jasecara commented 6 years ago

I can confirm that I also am getting this same error on a project I just upgraded to Laravel 5.5. This will work because it is the first time saving the model:

$user = new App\Models\User\User(); $user->firstName = "Santa"; $user->lastName = "Claus"; $user->save();

However the same exact code will not work if trying to update a model like so: $user = \App\Models\User\User::find(1); $user->firstName = "Santa"; $user->lastName = "Claus"; $user->save();

According to the Whoops page, the arguments passed to PDO are all camelcase attributes, so it looks like the conversion from camel back to snake case is broken for when trying to update the model.

jasecara commented 6 years ago

@kirkbushell I'm still not actually sure what changed from 5.4 to 5.5, but I was able to patch this problem by adding this override method to the camelcase trait:

 /**
 * Perform a model update operation.
 *
 * @param  \Illuminate\Database\Eloquent\Builder  $query
 * @return bool
 */
protected function performUpdate(Builder $query)
{
    // If the updating event returns false, we will cancel the update operation so
    // developers can hook Validation systems into their models and cancel this
    // operation if the model does not pass validation. Otherwise, we update.
    if ($this->fireModelEvent(\'updating\') === false) {
        return false;
    }

    // First we need to create a fresh query instance and touch the creation and
    // update timestamp on the model which are maintained by us for developer
    // convenience. Then we will just continue saving the model instances.
    if ($this->usesTimestamps()) {
        $this->updateTimestamps();
    }

    // Once we have run the update operation, we will fire the "updated" event for
    // this model instance. This will allow developers to hook into these after
    // models are updated, giving them a chance to do any special processing.
    $dirty = [];
    foreach(array_except($this->getDirty(), $this->appends) as $key => $value) {
        $dirty[snake_case($key)] = $value;
    }

    if (count($dirty) > 0) {
        $this->setKeysForSaveQuery($query)->update($dirty);

        $this->fireModelEvent('updated', false);

        $this->syncChanges();
    }

    return true;
}

What appears to be happening here is that $dirty holds the attributes in camel case, and additionally is including appended attributes that are not real db columns. When this gets passed off to be executed as a query, it's still the wrong casing. Converting $dirty to camel case and removing appended attributes seems to fix this behavior, but it warrants further investigation to make sure it doesn't break anything else.

kirkbushell commented 6 years ago

Thanks @jasecara. Will definitely look into this more next weekend, or later this week when I get time. Interesting issue.

acarpio89 commented 6 years ago

As @jasecara pointed out, the issue is that getDirty now returns attributes in camelCase. This is the change that introduced the problem: https://github.com/laravel/framework/commit/a0402824bf479dc5135b40a7f40095d4e2bbb265

In Laravel 5.5 the model's attributes are grabbed using getAttributes(), which is overridden by the CamelCasing trait in this package (https://github.com/kirkbushell/eloquence/blob/master/src/Behaviours/CamelCasing.php#L57).

A solution that worked for us was to override getAttribute and getOriginal again in our models so they can do what the framework is expecting:

public function getAttributes()
{
    return $this->attributes;
}

public function getOriginal($key = null, $default = null)
{
    return Arr::get($this->original, $key, $default);
}

Direct property access continues working with camelCase ($user->firstName) but if you grab all attributes at once the array keys will be in snake_case.

GuihaiHU commented 6 years ago

Thanks @acarpio89 . I do as what you suggest. But I don't know what Arr is, so I replace Arr::get with array_get and it works.

    public function getAttributes()
    {
        return $this->attributes;
    }

    public function getOriginal($key = null, $default = null)
    {
        return array_get($this->original, $key, $default);
    }
Cedricvb commented 6 years ago

If I use the solutions suggested, I get an 'Array to string conversion' error when saving a model with an updated attribute that is being cast as an array.

maclonghorn commented 6 years ago

@acarpio89 I used this as well and it works fine. However, when dealing with pivots, I'm still having a problem.

@kirkbushell have you seen this on Laravel 5.5 as well? Conversion doesn't seem to work on retrieve or insert. My scenario:

I have Client and Vendor tables with a many-to-many pivot. Each of those models is using CamelCasing trait (modified per @acarpio89 workaround above). Here's my API request:

PUT /clients/1/vendors/2

{
    "data": {
        "type": "vendors",
        "attributes": {
             "linkDate": "2018-02-01"
        }
    }
}

and my controller method:

public function addVendor($clientId, $vendorId, Request $request)
    {
        try {
            $attributes = $request->json('data.attributes');
            $model = $this->repository->find($clientId);
            $model->vendors()->attach($vendorId, $attributes);

            return $this->jsonApiEncodedDataResponse($model);
        } catch (\Exception $e) {
            return $this->jsonApiEncodedErrorResponse(500, 'API Error', $e->getMessage());
        }
    }

The error:

Undefined column: 7 ERROR:  column \"linkDate\" of relation \"vendor_client\" does not exist
maclonghorn commented 6 years ago

Just saw this:

public function getTrueKey($key)
    {
        // If the key is a pivot key, leave it alone - this is required internal behaviour
        // of Eloquent for dealing with many:many relationships.
        if ($this->isCamelCase() && strpos($key, 'pivot_') === false) {
            $key = camel_case($key);
        }

        return $key;
    }

So is there any way to still convert the pivot attributes when returning response? Here's my api reponse:


{
    "data": {
        "type": "client-vendors",
        "id": "1",
        "attributes": {
            "clientName": "Client A, Inc",
            "vendors": [
                {
                    "businessName": "My Apartments, Inc",
                    "businessCode": "abc",
                    "taxId": null,
                    "isSsn": false,
                    "statusId": 1,
                    "pivot": {
                        "client_id": 1,
                        "vendor_id": 2,
                        "is_priority": false,
                        "is_subscription": false
                    }
                }
            ]
        },
        "links": {
            "self": "/client-vendors/1"
        }
    }
}```