Skip to content
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

[Idea] UnifyModelDatesWithCastsRector: Make rule configurable for date casting #123

Open
mcampbell508 opened this issue Jul 24, 2023 · 1 comment

Comments

@mcampbell508
Copy link

mcampbell508 commented Jul 24, 2023

First of all, this project looks fantastic. I used this rule on a codebase and it covered 90% of my needs.

However, this is just a proposal but could there be a way to add configuration to this rule, so that you can cast specific properties to values that the end user defines?

I.e something that is not just datetime?

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->ruleWithConfiguration(UnifyModelDatesWithCastsRector::class, [
        new UnifyModelDateWithCast('App\Models\User', 'date_of_birth', 'immutable_date'), 
        new UnifyModelDateWithCast('App\Models\AnotherModel', 'another_column', 'immutable_datetime'), 
        new UnifyModelDateWithCast('App\Models\YetAnotherModel', 'another_column', 'date'), 
    ]);
};

and UnifyModelDateWithCast would be something like:

<?php

namespace App\Namespace;

class UnifyModelDateWithCast
{
    public function __construct(
        private readonly string $modelNamespace, 
        private readonly string $column, 
        private readonly string $castToValue, 
    ) {
    }
}

UnifyModelDateWithCast was just off the top of my head, and could be named more appropriately.

and everything else not specified here using the config will be set as datetime, as the current behaviour. Also, by default if there is no configuration, all casting will be done as datetime, as the current behaviour.

Desired outcome of above

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

 class User extends Model
 {
     protected $casts = [
-        'age' => 'integer',
+        'age' => 'integer', 'date_of_birth' => 'immutable_date',
     ];
-
-    protected $dates = ['date_of_birth'];
 }
namespace App\Models;

use Illuminate\Database\Eloquent\Model;

 class AnotherModel extends Model
 {
     protected $casts = [
-        'age' => 'integer',
+        'age' => 'integer', 'another_column' => 'immutable_datetime',
     ];
-
-    protected $dates = ['another_column'];
 }
namespace App\Models;

use Illuminate\Database\Eloquent\Model;

 class YetAnotherModel extends Model
 {
     protected $casts = [
-        'age' => 'integer',
+        'age' => 'integer', 'another_column' => 'date',
     ];
-
-    protected $dates = ['another_column'];
 }

Ideally, looking at this https://github.com/laravel/framework/blob/10.x/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L750-L784, the accepted values should be anything that is:

  • date
  • datetime
  • custom_datetime
  • immutable_date
  • immutable_custom_datetime
  • immutable_datetime
  • timestamp?

I've just gotten into learning about Rector, so could attempt to make a PR for the above, if it was desired behaviour of this project.

I am also not sure of how possible, all of this would be.

Again, all just an idea, which may or may not be progressed upon or even desired.

@GeniJaho
Copy link
Collaborator

Cool idea 😁

The rule UnifyModelDatesWithCastsRector is meant to be run just once per project, and in fact is part of the rules for upgrading to Laravel 10, since the $dates property was removed.

A simpler change that we can do to the rule is to allow casting to something other than datetime, but again, this would only be helpful if you're still using the $dates property in new day-to-day code. In that case, a custom rule would be a better choice in my opinion.

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

No branches or pull requests

2 participants