pkp/pkp-lib#10328 Refactor Announcements

[Vitaliy-1]

No description provided.

[asmecher]

It should be possible to use Eloquent date casts to get columns to come back automagically as Carbon objects; it shouldn't be necessary/desirable to use strtotime anymore.

[touhidurabir]

The case here is bit different here than the usual . we can have a cast define but as our types are coming from schema which will over write it in the ModelWithSettings trait's convertSchemaToCasts method where we are calling the mergeCasts which merge the provided casts with pre defined ones . I think if we want to apply some casts like

/**
 * Get the attributes that should be cast.
 *
 * @return array<string, string>
 */
protected function casts(): array
{
    return [
        'dateExpire' => 'datetime',
        'datePosted' => 'datetime',
    ];
}

we need to override the Illuminate\Database\Eloquent\Concerns\HasAttributes::mergeCasts to defined how it will handle and determine the precedence of provided casts over defined ones .

[touhidurabir]

Thinking about this a bit more, I think we should override the method mergeCasts inside the trait ModelWithSettings to allow model defined casts over write any casts coming form schema if schema available like

/**
     * Merge new casts with existing casts on the model.
     *
     * @param  array  $casts
     * @return $this
     */
    public function mergeCasts($casts)
    {
        $casts = $this->ensureCastsAreStringValues($casts);
        dump($this->casts);
        $this->casts = array_merge($casts, $this->casts);

        return $this;
    }

basically we are just altering how the array_merge getting done .

As trait ModelWithSettings will defined directly in entity model classes, it's mergeCasts should have higher precedence of HasAttributes::mergeCasts used in Illuminate\Database\Eloquent\Model .

[asmecher]

👍 Like this!

[Vitaliy-1]

It seams there is additional conversion of attributes between camel case and snake case happening somewhere in the Eloquent core. Not critical, can be handled later

[Vitaliy-1]

I don't know what is better to return here since we updating two tables at once?

[Hafsa-Naeem]

@Vitaliy-1 Instead of returning a single summed count, could we return an associative array with separate counts for the primary table and the settings table?
we can also use database transaction (DB::transaction) to ensure that both updates either succeed or fail together to maintain data integrity.

[Vitaliy-1]

Think if multilingual value doesn't exists it's better to just return null

[Vitaliy-1]

Deleting without models instantiating, it's much more performant

[touhidurabir]

@Vitaliy-1 added reviews . These are more like thoughts, suggestions and questions than reviews . Feel free to adopt if you feel like those fit the use cases .

BTW, I must say, this is very promising and will help us adopting eloquent more generally.

[touhidurabir]

Perhaps rather than accessing the primary key as getKey, how about setting a attribute setter/getter for primary key in the Announcement model as

protected function id(): Attribute
{
        return Attribute::make(
            get: fn($value, $attributes) => $attributes[$this->primaryKey] ?? null,
            set: fn($value) => [$this->primaryKey => $value],
        );
    }

This will allow us the access the primary key as $modelInstance->id which is easier and cleaner . Also we all are trying to adopt this approach .

[Vitaliy-1]

OK, let's try. There is a possibility that it might interact with schema's id attribute, I don't see anything right now but id is a primary attribute and the logic I've put to the new schema setting origin defines Mutators and composed attributes. Meaning in announcement.json:

"id": {
	"type": "integer",
	"origin": "primary",
	"readOnly": true,
	"apiSummary": true
},

[touhidurabir]

I think this may cause more issue than it will solve in future for us , specially for entities with schema maps with a JSON file . As we can add more props to schema via Hook like Hook::add('Schema::get::' . PKPSchemaService::SCHEMA_ANNOUNCEMENT, [$this, 'someFunction']); , but as we have predefined fillable, eloquent will discard anything and everything not defined in the property $fillable .

In fact in my experience this cause more confusion and frustration even when add new column via migration but forget the update the $fillable. And this above cases where we can override props via scheme hooks, this more to that case .

However we need fillable also as otherwise any unwanted params will casue exception for example in PKPAnnouncementController::add, we have params passed to create as

array:8 [▼
  "title" => array:2 [▶]
  "descriptionShort" => array:2 [▶]
  "description" => array:2 [▶]
  "image" => null
  "dateExpire" => null
  "sendEmail" => null
  "assocType" => 256
  "assocId" => 1
]

where we have no column named sendEmail .

How about we generate the fillable dynamically like

protected function constructFillable(): void
{
    $schemaService = app()->get('schema'); /** @var PKPSchemaService $schemaService */
    $primaryColumns = $schemaService->groupPropsByOrigin(static::getSchemaName())[Schema::ATTRIBUTE_ORIGIN_MAIN];
    $this->fillable = array_merge($this->fillable, $primaryColumns, $this->settings);
}

and then have it from ModelWithSettings::__construct as

public function __construct(array $attributes = [])
    {
        parent::__construct($attributes);

        if (static::getSchemaName()) {
            $this->setSchemaData();
            $this->constructFillable();
        }
    }

and in the models , we only defined a $guarded as

protected $guarded = ['id'];

[touhidurabir]

Given that few entities don't have schema, probably best to allow the generation of fillable not depending on the schema . That is something like

public function __construct(array $attributes = [])
    {
        parent::__construct($attributes);

        if (static::getSchemaName()) {
            $this->setSchemaData();
        }

        $this->constructFillable();
    }

protected function constructFillable(): void
    {
        $primaryColumns = [];

        if (static::getSchemaName()) {
            $schemaService = app()->get('schema'); /** @var PKPSchemaService $schemaService */
            $primaryColumns = $schemaService->groupPropsByOrigin(static::getSchemaName())[Schema::ATTRIBUTE_ORIGIN_MAIN];
        }
        
        $this->fillable = array_merge($this->fillable, $primaryColumns, $this->settings);
    }

[touhidurabir]

Any particular reason we are defining the mergeCasts as abstract here as it's already defined in the Illuminate\Database\Eloquent\Concerns\HasAttributes which used as a trait in Illuminate\Database\Eloquent\Model ? unless this trait is being used in non eloquent entity classes, do we have any use of it here ?

[touhidurabir]

I see we are defining the method getSchemaName as static but then accessing as class instance method . I know laravel's handle it internally but perhaps better for us to stick with the general convention when we can . Or there any particular reason I am missing ?

[touhidurabir]

May be for a model instance, for some unforeseen reason, it may not contains localized data even when the localized property still defines for the model/schema . However checking via the getAttribute, we will only get what right now set for database retrieved model instance and which may not contain any details of localized property. may be better to check if the multilingual props actually exists on model/schema and make decision based on that aviability like

if (!in_array($data, $this->getMultilingualProps())) {
    throw new Exception(
        sprintf("Given localizeble property %s does not exists in %s model", $data, static::class)
    );
}

$multilingualProp = $this->getAttribute($data);

// TODO What should the default behaviour be if localized value doesn't exist?
return $multilingualProp[$locale] ?? null;

[touhidurabir]

The case here is bit different here than the usual . we can have a cast define but as our types are coming from schema which will over write it in the ModelWithSettings trait's convertSchemaToCasts method where we are calling the mergeCasts which merge the provided casts with pre defined ones . I think if we want to apply some casts like

/**
 * Get the attributes that should be cast.
 *
 * @return array<string, string>
 */
protected function casts(): array
{
    return [
        'dateExpire' => 'datetime',
        'datePosted' => 'datetime',
    ];
}

we need to override the Illuminate\Database\Eloquent\Concerns\HasAttributes::mergeCasts to defined how it will handle and determine the precedence of provided casts over defined ones .