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

Property type isn't detected when casting Eloquent attributes to spatie/laravel-data Data objects #1400

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

code-distortion
Copy link

The problem

When casting Eloquent attributes to spatie/laravel-data Data objects, ide-helper doesn't pick the correct class. For example, the example from Spatie's documentation https://spatie.be/docs/laravel-data/v2/advanced-usage/eloquent-casting:

<?php
# app/Models/Song.php
namespace App\Models;

use App\SpatieData\ArtistData;
use Illuminate\Database\Eloquent\Model;

class Song extends Model
{
    protected $casts = [
        'artist' => ArtistData::class,
    ];
}
<?php
# app/SpatieData/ArtistData.php
namespace App\SpatieData;

use Spatie\LaravelData\Data;

class ArtistData extends Data
{
    public function __construct(
        public string $name,
        public int $age,
    ) {
    }
}

Running php artisan ide-helper:models --dir='app' --write generates this DocBlock:

/**
 * App\Models\Song
 *
 * @property \Spatie\LaravelData\Contracts\BaseData|null $artist // ❌ not the desired prop definition
 * @method static \Illuminate\Database\Eloquent\Builder|Song newModelQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|Song newQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|Song query()
 * @mixin \Eloquent
 */

This means that we don't get the benefit of type-hinting for casted attributes like this:

$songModel->artist->name;

Why does this happen?

This happens because src/Support/EloquentCasts/DataEloquentCast.php from Spatie's package specifies a return type. This is what ide-helper picks up:

# src/Support/EloquentCasts/DataEloquentCast.php in spatie/laravel-datapublic function get($model, string $key, $value, array $attributes): ?BaseData
    {
…

Proposed change

I put this together when looking through the code, trying to work out how to fix it in a project of mine. I thought I'd turn it into a PR for discussion…

This PR lets you instruct ide-helper to use the class that was specified, by adding the DocBlock tag @ide-helper-eloquent-cast-to-specified-class to the class being cast to. e.g.

<?php
# app/SpatieData/ArtistData.php

/**
 * @ide-helper-eloquent-cast-to-specified-class
 */
class ArtistData extends Data
{
…

From which, ide-helper now generates:

/**
 * App\Models\Song
 *
 * @property ArtistData $artist // ✅ the desired prop definition
 * @method static \Illuminate\Database\Eloquent\Builder|Song newModelQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|Song newQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|Song query()
 * @mixin \Eloquent
 */

Notes

  • The @ide-helper-eloquent-cast-to-specified-class tag can be added to a class, or any of it's parents.
  • spatie/laravel-data could add this tag to their Spatie\LaravelData\Data class, so it's always applied to Data classes that use it. But if they don't, it can just as easily be added to Data classes (or a parent class) in individual projects.
  • This solution isn't specific to spatie/laravel-data. It can be used for any classes being cast to in this manner.

Thoughts

  • I couldn't think of a way for it to happen automatically without forcing it in every case. Which I don't think is 100% desired.
  • I don't think having to add a @tag is a "nice" solution, however it does give control over when it happens.
  • An alternative I considered was to specify the relevant classes inside ide-helper's config file. But this doesn't seem very flexible.
  • I'd be interested to hear if anybody has other ideas about how to detect when it should occur.

Other issues and PRs


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)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

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

@jameshulse
Copy link

Fantastic. We are running into the same issue now. I guess the other alternative would be to give laravel-ide-helper special handling for the spatie/laravel-data package. But I think your solution is still workable.

@barryvdh
Copy link
Owner

barryvdh commented Feb 8, 2024

Can we not check if the actual cast is extending the type, and in that case return actual cast (same as fallback $type from #1312?

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

Successfully merging this pull request may close these issues.

3 participants