Add model property types to magic where methods.

[jpickwell]

No description provided.

[mfn]

That actually looks pretty cool!

Can you please run composer test-regenerate to fix the snapshots and also commit them?

[jpickwell]

Alright. I actually made the change right here on GitHub. 😉

[jpickwell]

I wonder why the expectations are still wrong. Could it be because I force-pushed? I can undo that force-push by force-pushing the previous version of the commit, and then committing the snapshot updates as a new commit.

[jpickwell]

Well, force-pushing appears to break the test system. 😁 Do we need a test for the test system. 😆

[mfn]

Seems everything worked out in the end?

[mfn]

Now seeing some of the changes, I'm not sure the approach is entirely correct.

Basically it assumes that the "cast output" is the sole allowed input for those magic methods.

But this isn't correct, is it? No one requires passing a Carbon class for example.

Carbon works because it has some special support or is simply converted to string (didn't check in detail) 🤔

But when I imagine other custom cast objects, it will depend on the objects capability whether they work correctly or not.

What do you think about this scenario?

[jpickwell]

I personally am fine with the current results. However, I understand what you're getting at. In that case, I think the property should also have any other acceptable types specified, or the where param type needs to have its own type method to determine the type based on the column/property type and any additional rules.

I'll dig through Laravel's code to understand the where method and how type hinting can play a part.

[jpickwell]

Illuminate\Database\Query\Builder handles the dynamic where clauses with a dynamicWhere method. It supports "And" and "Or", so more than one property can be specified in the dynamic where method name; e.g., whereFooAndBarOrBax("blah", "bleh", "boo"). By no means am I going to recommend supporting all of those permutations on a model.

Focusing only on what's possible with dynamic wheres, the where method is called like this, where($column, '=', $value, $boolean), from the addDynamic method. $boolean is either "and" or "or", defaulting to "and" for the first column. $boolean is only of concern for multi-column dynamic wheres. $value can be a Closure (delegating to whereSub), null (delegating to whereNull), or anything else. Closure is easy to handle by just prepending \Closure| to the property type for the dynamic where. null is handled by the |null union, if present. The last possibility is the one that concerns us the most. In this case, the $value is added to the $bindings["where"] set.

Illuminate\Database\Connection@prepareBindings loops through the bindings, and formats DateTimeInterface instances (based on the current grammar's date format) and converts bools to ints, and leaves everything else as is.

Illuminate\Database\Connection@bindValues loops through the bindings, calling PDOStatement@bindValue for each one, with the $data_type param set to is_int($value) ? PDO::PARAM_INT : PDO::PARAM_STR.

With all this being said, I think what I can do, for now (time will tell what else may need to be done), is:

1. Prepend \Closure| to the property type to support subqueries.
2. Append |string to the property type if it implements DateTimeInterface. Not sure about this one, but that's probably an acceptable option.

Any additional thoughts on this would be appreciated.

Aside: In the Rails community, dynamic finders, as they're called in Rails, have been frowned upon for quite some time, and in fact were deprecated in favor of using findBy(hash); e.g., findBy(foo: "blah", bar: "bleh"). I don't know why they deprecated dynamic finders, but I do think it makes things easier to use, and more robust to changes. There may have also been a performance reason, but that's conjecture.

[mfn]

From my experience I can come up with two reasons against them:

• column names are unqualified
  If you join with other tables sharing the same column name, you can't use them due to the ambiguity
  Thusly our team made it a requirement that you always have to use ->where('table.column', …'
• different naming style (camel case) makes searching for "usage" harder
  By default/convention, column names map 1:1 model attribute names, i.e. SQL column foo_bar == model attribute foo_bar. If you write manual conditions, you need to use the actual column name ->where('foo_bar', …).
  Means: if you refactor you can easily find column/attribute names
  But if you use the magic where, it's whereFooBar. So if you refactor, it's yet another thing you have to consider.

[jpickwell]

@mfn, so are you against merging this in because magic-where-methods should not be used, in your opinion? I'm just wondering if I should do any more work on this PR, such as adding \Closure| to the param type.

[mfn]

@jpickwell no, this was additional data for your last paragraph starting with "Aside: In the Rails community, dynamic finders, as they're called in Rails, have been frowned upon for quite some time" ;-)

If one doesn't like them, they can already disable them via config write_model_magic_where => false.

I yet have to assess the feedback.

[mfn]

I tested this on a real project and noticed another problem.

Imagine you've a property like this:

 * @property int|null $parent_id

Before:

 * @method static Builder|Post whereParentId($value)

Your PHP generates this:

 * @method static Builder|Comment whereParentId(integer $value)

So my code using whereParentId(null) now is shown as error:

So this definitely needs a bit more refinement / testing.

[mfn]

@jpickwell are you interested in continuing this?

[jpickwell]

@mfn, I can take a look into that.

[mfn]

🙏

[jpickwell]

I refactored the setProperty method to use a new helper method getTruePropertyType which is then also used for the where methods. This fixed the missing null issue, and also does the type overrides, which was missing before.

[mfn]

This looks really good 🤞

There's one small feedback left and can you please also add a changelog entry under Added?

[mfn]

I mention this in my previous feedback but it might just have gotten lost: any method accepting Carbon (or a variation thereof) also accept strings, it doesn't just accept an instance (or null).

[jpickwell]

Understood, but if you look at the created_at property, it's also \Carbon\CarbonImmutable|null, so this is not an issue with this new feature.

[jpickwell]

The whereProperty method parameter type is now exactly the same as the corresponding @property.

[mfn]

Understood, but if you look at the created_at property, it's also \Carbon\CarbonImmutable|null, so this is not an issue with this new feature.
…
The whereProperty method parameter type is now exactly the same as the corresponding @property.

I pointed this out in #989 (review)

Basically it assumes that the "cast output" is the sole allowed input for those magic methods.

However this assumption isn't correct.

Let's look at this tinker session:

$ ./artisan tinker
Psy Shell v0.10.4 (PHP 7.4.10 — cli) by Justin Hileman
>>> $user = App\Models\User;
=> App\Modes\User {#4302
   }
>>> $user->deleted_at;
=> null
>>> $user->deleted_at = '2020-10-10 12:12:00';
=> "2020-10-10 12:12:00"
>>> $user->deleted_at
=> Carbon\CarbonImmutable @1602331920 {#4301
     date: 2020-10-10 12:12:00.0 UTC (+00:00),
   }
>>> $user->deleted_at = \Carbon\CarbonImmutable::now();
=> Carbon\CarbonImmutable @1599856846 {#4319
     date: 2020-09-11 20:40:46.689963 UTC (+00:00),
   }
>>> $user->deleted_at
=> Carbon\CarbonImmutable @1599856846 {#4309
     date: 2020-09-11 20:40:46.0 UTC (+00:00),
   }

Setting either string or Carbon is fine.

However, in the IDE:

---

I noticed something else in a private project I tested this:

Migration:

            $table->jsonb('keywords')->nullable();

Model:

 * @property array|null $keywords
…
    protected $casts = [
        'keywords' => 'array',
    ];

Before:

 * @method static Builder|Automation whereKeywords($value)

After:

 * @method static Builder|Automation whereKeywords(string|null $value)

I think string is practical in this context, there's not much point about this anyway as it's a JSON column.

---

But then I've another model with a similar column which however generated this:

Migration:

            $table->json('some_other_column')->nullable();

Model:

 * @property array $some_other_column
…
 * @method static Builder|Attachment whereSomeOtherColumn(mixed|null $value)
…
    protected $casts = [
        'some_other_column' => 'array',
    ];
…
    public function setSomeOtherColumnAttribute(?array $value): self
    {
…    }

    public function getSomeOtherColumnAttribute(): array
    {
…    }

The mixed is in fact also not that bad here, actually might make even more sense.

Though I could not figure out why one is string here and one is mixed. I tried removing the set/get but it didn't change 🤷‍♀️

---

Whilst I'm not yet in the clear regarding the latter new examples, I think that date casts not accepting string is bad. The laravel-ide-helper is supposed to help but would get in the way here.

These were only easy examples with $date and array casts, but maybe properties in $casts have to be handled differently?

[jpickwell]

I'm not disputing the string issue with dates. However, that's an existing issue in laravel-ide-helper. You have the same issue with setting the magic properties. I'm not going to fix this issue in this PR. That's outside the scope of the PR. If you want that fixed, then create an issue and maybe someone will create a PR to rectify the issue.

If you look at the CustomDate snapshot, you'll see that the @property tags and the corresponding where @method tags have the same value type:

<?php

declare(strict_types=1);

namespace Barryvdh\LaravelIdeHelper\Tests\Console\ModelsCommand\CustomDate\Models;

use Illuminate\Database\Eloquent\Model;

/**
 * Barryvdh\LaravelIdeHelper\Tests\Console\ModelsCommand\CustomDate\Models\CustomDate
 *
 * @property \Carbon\CarbonImmutable|null $created_at
 * @property \Carbon\CarbonImmutable|null $updated_at
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate newModelQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate newQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate query()
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate whereCreatedAt(\Carbon\CarbonImmutable|null $value)
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate whereUpdatedAt(\Carbon\CarbonImmutable|null $value)
 * @mixin \Eloquent
 */
class CustomDate extends Model
{
}

The type for the $value parameter is exactly the same as the corresponding @property type. And, when I say "exactly the same", I mean that the string is calculated using the same helper method in the command class, see the following lines in the ModelsCommand.php file in this PR:

• 482 -- getting the initial type for a property
• 487 -- passing the type to the setProperty method
• 495 -- getting the "true" type for the property for the where method by calling getTruePropertyType
• 728 -- same method call, but for the property itself
• 822 -- shows that no other type manipulation is performed when generating the PhpDoc @property tags

[mfn]

I'm not going to fix this issue in this PR. That's outside the scope of the PR. If you want that fixed, then create an issue and maybe someone will create a PR to rectify the issue.

That's a pretty strong feedback here, not sure where this is coming from.

---

It made me think though whether this whole feature should be optional (opt-in/out => debatable)

[jpickwell]

Using the example of dates as \Carbon\CarbonImmutable and not allowing strings is an issue that already exists in the code base. I did not introduce this issue. If you look at the diffs for the test snapshots, you'll see that the @property tags did not change.

My objection to your feedback is that you're asking me to fix an issue, an issue that already existed, when I'm trying to introduce a new feature. If this existing issue stood in the way of introducing this new feature, then I would fix it. However, the issue does not hinder this feature's implementation. Clearly, what this feature does do is highlight the existing issue. I agree the issue should be fixed, but I believe that it should be fixed in a new PR, one that targets the issue specifically.

I don't have any objection to adding a flag for this feature.

---

Just to be extra clear, to reiterate what I've been saying, the @property tags have not been affected by my changes.

Here's what the CustomDate test snapshot looks like before my changes:

<?php

declare(strict_types=1);

namespace Barryvdh\LaravelIdeHelper\Tests\Console\ModelsCommand\CustomDate\Models;

use Illuminate\Database\Eloquent\Model;

/**
 * Barryvdh\LaravelIdeHelper\Tests\Console\ModelsCommand\CustomDate\Models\CustomDate
 *
 * @property \Carbon\CarbonImmutable|null $created_at
 * @property \Carbon\CarbonImmutable|null $updated_at
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate newModelQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate newQuery()
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate query()
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate whereCreatedAt($value)
 * @method static \Illuminate\Database\Eloquent\Builder|CustomDate whereUpdatedAt($value)
 * @mixin \Eloquent
 */
class CustomDate extends Model
{
}

Notice that the $created_at and $updated_at properties have the same issue you're asking my to fix. I feel like you're ignoring this fact, and just assuming that somehow I have introduced this issue when in reality I have not.

[jpickwell]

I created a new PR (#1056) to fix the type annotations for date attributes/properties.

[jpickwell]

One more thing: looking at the JSON example you gave, I will try to fix that one. That has to do with the command not checking casts until after getting properties from the table:

// ModelsCommand::generateDocs
                    if ($hasDoctrine) {
                        $this->getPropertiesFromTable($model);
                    }

                    if (method_exists($model, 'getCasts')) {
                        $this->castPropertiesType($model);
                    }

castPropertiesType should probably be called from getPropertiesFromTable.

[mfn]

@jpickwell awesome, thanks for your feedback!

Quite a lively discussion going in here ;)

My suggestion to finalize this would be:

• add config (please also add an entry to README for this)
• make it opt-out (aka enable it by default)

What do you think (as you said: independent of "the one issue" and the other PR)?

[jpickwell]

I'm working on fixing the cast issue at the moment. I can add a new config option with test and readme entry.

Unlike the date type issue, not handling casts is a hindrance of this feature.