Skip to content

[5.7] Improve the return value of the caseKey method - #24168

Merged
taylorotwell merged 1 commit into
laravel:masterfrom
seth-shi:5.5
May 11, 2018
Merged

[5.7] Improve the return value of the caseKey method#24168
taylorotwell merged 1 commit into
laravel:masterfrom
seth-shi:5.5

Conversation

@seth-shi

Copy link
Copy Markdown
  • this PR is for improving the return value of the caseKey method
  • The castkey method should have returned the value of the primary key type, rather than judging by the is_number or is_string method

Example

class User extends Eloquent
{
    protected $table = 'users';
    protected $fillable = ['id', 'email'];
    public $timestamps = false;

    public function articles()
    {
        return $this->belongsToMany(Article::class, 'article_user', 'user_id', 'article_id');
    }
}
class Article extends Eloquent
{
    protected $table = 'articles';
    protected $keyType = 'string';
    public $incrementing = false;
    public $timestamps = false;
    protected $fillable = ['id', 'title'];
}
## users
---------------------------------------------------   
id 
1  
## articles
id      
---------------------------------------------------                                                            
123                                                              
7b7306ae-5a02-46fa-a84c-9538f45c7dd4 
(string)PHP_INT_MAX+1                             
  • In the past, I have set the primary key type of the Article model to string, but if I insert an integer overflow number through`$user->articles()->sync(), I can successfully insert the data, but if the type of the return value is a long numeric string, Will not get the right results.

Date:   Thu, 10 May 2018 03:15:47 +0000

    * fixed InteractsWithPivotTable::caseKey return value type
@seth-shi

Copy link
Copy Markdown
Author

@tillkruss Hello, can the style of the test code be beautified through you?

@sisve

sisve commented May 10, 2018

Copy link
Copy Markdown
Contributor

Would these changes be needed in other (read: non-pivot) places too? I'm thinking mostly about consistency; this looks like it will introduce new types for primary keys for pivots, so we should probably do that for normal models too?

@seth-shi

Copy link
Copy Markdown
Author

@sisve The correct type relationship has been handled in the model, as long as keyType is set, the error here is mainly because the primary key value in the parameter is simply used in the code to determine whether it is a number or a string. Rather than judging by the type of primary key of the model

@seth-shi seth-shi changed the title Author: Roc <1033404553@qq.com> improving the return value of the caseKey method May 10, 2018
@GrahamCampbell
GrahamCampbell changed the base branch from 5.5 to master May 10, 2018 11:48
@GrahamCampbell GrahamCampbell changed the title improving the return value of the caseKey method [5.7] Improve the return value of the caseKey method May 10, 2018
@taylorotwell

Copy link
Copy Markdown
Member

What if the relationship is using a custom pivot model class with its own key type?

@taylorotwell
taylorotwell merged commit 1e53e32 into laravel:master May 11, 2018
@seth-shi
seth-shi deleted the 5.5 branch May 14, 2018 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants