Refactor resolving of null information for custom casted attribute types

[wimski]

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)
• 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

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

[mfn]

Why is this changed? Is it intentional based on the fix? Thanks

[wimski]

Yes, this is intentional. I didn't change the migration for these properties and they weren't marked as nullable(), so the |null gets removed by the fix in this PR.

[daniel-de-wit]

@mfn Can you please take another look at this?

[mfn]

applyNullability make my head hurt but me feel is this is a "LGTM"!

[mfn]

@wimski can you please update the changelog too?

[barryvdh]

Added to changelog in aa1aa66
Thanks!