pkp/pkp-lib#10133 Port Genre and GenreDAO to Use Eloquent Model #10194
Conversation
|
|
There was a problem hiding this comment.
Thanks, @Hafsa-Naeem, I've added some comments but I think the biggest thing is to agree on some conventions WRT the settings table and collectors -- feel free to clean up some details, then let's make sure we've got those conventions figured out before continuing!
| // Relationships | ||
| public function submissionFiles(): HasMany | ||
| { | ||
| return $this->hasMany(SubmissionFile::class, 'genre_id'); |
There was a problem hiding this comment.
Are you sure this should be genre_id?
There was a problem hiding this comment.
I used this relationship definition because in the database schema, genre_id is used as the foreign key in submission_files.
| // Accessors and Mutators | ||
| protected function entryKey(): Attribute | ||
| { | ||
| return Attribute::make( |
There was a problem hiding this comment.
Was there an equivalent to this in the previous implementation?
There was a problem hiding this comment.
In the previous implementation we used getter and setter methods for entryKey
public function getKey()
{
return $this->getData('key');
}
public function setKey($key)
{
$this->setData('key', $key);
}
and in the new Eloquent this is replaced with
protected function entryKey(): Attribute
{
return Attribute::make(
get: fn($value) => strtoupper($value),
set: fn($value) => strtolower($value)
);
}
There was a problem hiding this comment.
It's the uppercase/lowercase conversion I was wondering about; if it didn't exist in the previous code, best not to add it here.
| ); | ||
| } | ||
|
|
||
| protected function enabled(): Attribute |
There was a problem hiding this comment.
If we're just looking to cast a PHP boolean into a database int...
- It might be better to use attribute casting, or
- better still, just leave it an integer and at some point we'll declare boolean columns in the DB with the
booleantype so that everything matches nicely without needing casting.
There was a problem hiding this comment.
Will this class still be required after the move to Eloquent is completed?
There was a problem hiding this comment.
@asmecher No. it's temporary until the settings table handling is sorted out.
| /** | ||
| * Get enabled genres by context ID. | ||
| * | ||
| * @param int $contextId |
There was a problem hiding this comment.
The @param phpdocs don't add anything that's not already in the function declaration, so please remove them. (Here and below.)
| } | ||
|
|
||
| return $query->get(); | ||
| } |
There was a problem hiding this comment.
There should be a blank space between function declarations.
for #10133