-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Model branch #37
base: database_migration_seed
Are you sure you want to change the base?
Model branch #37
Conversation
…m intermediary pivot table
app/Models/Author.php
Outdated
|
||
class Author extends Model | ||
{ | ||
use HasFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add SoftDeletes
trait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in other places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
app/Models/Author.php
Outdated
|
||
public function books() | ||
{ | ||
return $this->belongsToMany('App\Models\Book')->using('App\Models\AuthorBook'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use like this:
return $this->belongsToMany(Book::class)->using(AuthorBook::class);
It'll be easier for the IDE to reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in other places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
class AuthorBook extends Pivot | ||
{ | ||
public $incrementing = true; | ||
use HasFactory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trait statement should be the first line inside the Class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
app/Models/Book.php
Outdated
{ | ||
use HasFactory; | ||
|
||
public function authors(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
should be placed in the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
public function bookOfCopies(){ | ||
return $this->hasMany('App\Models\BookCopy'); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove unnecessary empty lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
app/Models/BookUser.php
Outdated
{ | ||
use HasFactory; | ||
|
||
public function userHaveCopy(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming doesn't seem right. Please check. Think about the naming from the usage. For example, in your case it would be:
$bookUser = new BookUser();
$bookUser->userHaveCopy;
$bookUser->userHaveCopy
it doesn't sound good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same is for the below method bookOfUser
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
app/Models/Genre.php
Outdated
{ | ||
use HasFactory; | ||
|
||
public function GenreBooks(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the naming and put the {
on the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in other places too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -16,7 +16,7 @@ public function up() | |||
Schema::create('authors', function (Blueprint $table) { | |||
$table->id(); | |||
$table->string('name'); | |||
$table->string('image'); | |||
$table->string('image')->nullable(); | |||
$table->text('description'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be nullable.
@@ -16,7 +16,7 @@ public function up() | |||
Schema::create('publications', function (Blueprint $table) { | |||
$table->id(); | |||
$table->string('name'); | |||
$table->string('logo'); | |||
$table->string('logo')->nullable(); | |||
$table->text('description'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be nullable.
use Illuminate\Database\Schema\Blueprint; | ||
use Illuminate\Support\Facades\Schema; | ||
|
||
class AddDropForeignOnAuthorBookTable extends Migration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why creating separate migration for 'foreign keys'? It would be hard to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could't write the drop code on down method. I have take this step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. foreign key drop added on there specific table migration file.
No description provided.