laravel-ide-helper icon indicating copy to clipboard operation
laravel-ide-helper copied to clipboard

Refactor resolving of null information for custom casted attribute types

Open wimski opened this issue 3 years ago • 0 comments

Summary

Let's say we have the following custom cast:

class MyCustomCast implements CastsAttributes
{
    /**
     * @param Model                $model
     * @param string               $key
     * @param mixed                $value
     * @param array<string, mixed> $attributes
     * @return SomeObject|null
     */
    public function get($model, string $key, $value, $attributes): ?SomeObject
    {
        if ($value === null) {
            return null;
        }

        return new SomeObject($value);
    }

    // setter omitted from example for brevity
}

Now suppose we have two pretty much identical models which use this cast:

class ModelA extends Model
{
    protected $table = 'models_a';

    /**
     * @var array<string, string>
     */
    protected $casts = [
        'casted_column' => MyCustomCast::class,
    ];
}
class ModelB extends Model
{
    protected $table = 'models_b';

    /**
     * @var array<string, string>
     */
    protected $casts = [
        'casted_column' => MyCustomCast::class,
    ];
}

These models have tables created with the following migrations:

Schema::create('models_a', function (Blueprint $table) {
	$table->id();
	$table->string('casted_column');
	$table->timestamps();
});
Schema::create('models_b', function (Blueprint $table) {
	$table->id();
	$table->string('casted_column')->nullable();
	$table->timestamps();
});

The only difference between A and B is the nullable() setting on the column. Based on that difference I would expect the following properties to be written by the IDE helper:

/**
 * @property SomeObject $casted_column
 */
class ModelA extends Model
/**
 * @property SomeObject|null $casted_column
 */
class ModelB extends Model

However, both models will receive SomeObject|null as the property type, because currently the IDE helper only check the cast class without taking the migration into account. In my opinion the migration should inform the possibility of a null value and not the cast, because the cast doesn't know anything about the column it's going to be used for and should always implement X|null.

Type of change

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)

In my opnion this is a fix for behaviour that was always incorrect, but because it changes behaviour (and outcome), I do consider it a breaking change. I'll let it up to the maintainers how they want to deal with this.

Checklist

  • [x] Existing tests have been adapted and/or new tests have been added
  • [ ] Add a CHANGELOG.md entry
  • [ ] Update the README.md
  • [x] Code style has been fixed via composer fix-style

wimski avatar Mar 11 '22 12:03 wimski