8000 [5.2] Checking value in isJsonCastable function by evsign · Pull Request #12013 · laravel/framework · GitHub
[go: up one dir, main page]
Skip to content

[5.2] Checking value in isJsonCastable function #12013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[5.2] Checking value in isJsonCastable function #12013

wants to merge 2 commits into from

Conversation

evsign
Copy link
@evsign evsign commented Jan 24, 2016

Situation

We need to store some json data in pivot table. And for that we decide to use Eloquent attribute casting functionality, for more easily setting and getting data.

For that we must create custom pivot model and set protected $casting property.

expected:

attribute setted in $casting property as json or array will be automaticaly casted in array by accesing that property on our custom pivot model and any setted array or JsonSerializable object will be json encoded for storing in database.

actually:

when pivot model loading, values in castable properties will be double encoded and therefore can not be correctly decoded as we expect.
#10533 and partially #8972

how to reproduce:

create new laravel project

php artisan make:migration create_students_table --create=students 
php artisan make:migration create_subjects_table --create=subjects
php artisan make:migration create_student_subject_table --create=student_subject
php artisan make:model Student
php artisan make:model Subject
php artisan make:model StudentSubjectPivot

Migrations

students:
    $table->increments('id');
    $table->string('name');
    $table->string('email');
    $table->timestamps();
subjects:
    $table->increments('id');
    $table->string('name');
    $table->string('description');
    $table->timestamps();
student_subject:
    $table->unsignedInteger('student_id');
    $table->unsignedInteger('subject_id');
    $table->text('marks'); //or $table->json('marks');
    $table->timestamps();

Student.php model

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;

class Student extends Model
{
    protected $fillable = [
        'name',
        'email'
    ];

    public function subjects()
    {
        return $this->belongsToMany('App\Subject')->withPivot('marks');
    }

    public function newPivot(Model $parent, array $attributes, $table, $exists)
    {
        if ($parent instanceof Subject) {
            return new StudentSubjectPivot($parent, $attributes, $table, $exists);
        }

        return parent::newPivot($parent, $attributes, $table, $exists);
    }
}

Subject.php model

<?php

namespace App;

use Illuminate\Database\Eloquent\Model;

class Subject extends Model
{
    protected $fillable = [
        'name',
        'description'
    ];

    public function students()
    {
        return $this->belongsToMany('App\Student')->withPivot('marks');
    }

    public function newPivot(Model $parent, array $attributes, $table, $exists)
    {
        if ($parent instanceof Student) {
            return new StudentSubjectPivot($parent, $attributes, $table, $exists);
        }

        return parent::newPivot($parent, $attributes, $table, $exists);
    }
}

StudentSubjectPivot.php model

<?php

namespace App;

use Illuminate\Database\Eloquent\Relations\Pivot;
use Illuminate\Database\Eloquent\Model;

class StudentSubjectPivot extends Pivot
{
    protected $casts = [
        'marks' => 'array'
    ];

}

Then fill database with test data including pivot marks column with json

$student = App\Student::create(['name' => 'evsign', 'email' => 'evsign@live.ru']);
$subject = App\Subject::create(['name' => 'mathematics', 'description' => 'language of the universe']);
$student->subjects()->attach([$subject->id => [ 'marks' => '[{"date":"2009-12-29 03:01:18","mark":"D"},{"date":"2009-12-29 03:00:40","mark":"F"},{"date":"2009-12-29 03:01:00","mark":"B"},{"date":"2009-12-29 03:00:33","mark":"C"},{"date":"2009-12-29 03:01:09","mark":"A"},{"date":"2009-12-29 03:00:27","mark":"A"},{"date":"2009-12-29 03:00:37","mark":"A"},{"date":"2009-12-29 03:00:25","mark":"F"}]']]);
dd(App\Student::find($student->id)->subjects()->first()->pivot->marks); // will be string instead of array

For fix this issue

Use accessors and mutators with checking values

or

I suggest to send and check values for array or object in isJsonCastable function in Illuminate\Database\Eloquen\Model.php. ☺️

@GrahamCampbell GrahamCampbell changed the title Checking value in isJsonCastable function [5.2] Checking value in isJsonCastable function Jan 24, 2016
@taylorotwell
Copy link
Member

Did you uncover the root reason why the values are double encoded? What is causing that in the Laravel core?

@evsign
Copy link
Author
evsign commented Jan 26, 2016

@taylorotwell Yes, when pivot relation setting, pivot model filling with fill function and all attributes sets by calling setAttribute function inside wich situated all mutating and casting logic. Whereupon all values retrieved from database passing in this function and will be double setMutated/casted. In another words, values retrie 8000 ved from database already casted/setMutated and if they passing through setAttribute function thay will be double setMutated/casted and in situation of array casting we can just check value for array or object for prevent setMutating/casting actions on already setMutated/casted values.
Also i try to suggest, maybe casting/setMutating logic must be in moment of saving model to database and reverse logic in setAttribute function and in moment of setting attributes with data retrieved from database. In this way model always wil contain data like we want, and will write in database like should ☺️

@taylorotwell taylorotwell mentioned this pull request Jan 29, 2016
Closed
@taylorotwell
Copy link
Member

I see the problem but this isn't really the correct fix. The problem lies in the Pivot model constructor where we call ->fill()... however, changing this now would introduce another breaking change if someone is manually creating a new Pivot instance and they expect the values they pass to be mutated.

@taylorotwell
Copy link
Member

Any change would probably have to go on 5.3 since it would be breaking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@evsign @taylorotwell
0