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

Add additional types allowed for date properties #1056

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

Conversation

jpickwell
Copy link

@jpickwell jpickwell commented Sep 14, 2020

Summary

Date properties added by laravel-ide-helper currently do not allow additional types supported by Laravel. The $model->asDateTime($value) method (\Illuminate\Database\Eloquent\Concerns\HasAttributes::asDateTime) is used when setting a date attribute/property. This method checks for \Carbon\CarbonInterface, \DateTimeInterface, "numeric" value, or a date string.

This PR fixes this, and creates a PHPDoc type union containing the custom date class and the above mentioned types (\Carbon\CarbonInterface, \DateTimeInterface, integer, and string).

Note: based on what the asDateTime method expects, it no longer makes sense to type hint against a custom date class because any date value must be "numeric", a string, or implement either \Carbon\CarbonInterface or \DateTimeInterface (\Carbon\CarbonInterface implements this interface). \Illuminate\Support\Carbon extends \Carbon\Carbon which in turn implements \Carbon\CarbonInterface. Something to consider.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

I have created this PR in response to @mfn's comments on #989.

@jpickwell jpickwell changed the title fix: add additional types for date properties Add additional types allowed for date properties Sep 14, 2020
@jpickwell
Copy link
Author

@mfn, I think it would be better if this PR is merged before #989. What do you think?

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we've another catch-22 there 😞

The problem is the duality, the different expectations for input/output.

The currently generated phpdoc like \Illuminate\Support\Carbon say:

  • you can set a \Illuminate\Support\Carbon
  • you get a \Illuminate\Support\Carbon

With this change it becomes

  • you can set any of \Carbon\CarbonInterface|\DateTimeInterface|\Illuminate\Support\Carbon|integer|string
  • you get any of \Carbon\CarbonInterface|\DateTimeInterface|\Illuminate\Support\Carbon|integer|string

What it really should be:

  • you can set any of \Carbon\CarbonInterface|\DateTimeInterface|\Illuminate\Support\Carbon|integer|string
  • you get a \Illuminate\Support\Carbon

With the casts, you never get any of \DateTimeInterface|\Illuminate\Support\Carbon|integer|string; it's just not possible.

I'm not an expert on phpdoc, I know there's @property-read and @property-write => https://docs.phpdoc.org/latest/references/phpdoc/index.html but their respective documentation is actually clear:

The @property-write tag allows a class to know which ‘magic’ properties are present that are write-only. The … tag is used in the situation where a class contains the __set() magic method and allows for specific names that are not covered in a __get() magic method.

The @property-read tag allows a class to know which ‘magic’ properties are present that are read-only. The … tag is used in the situation where a class contains the __get() magic method and allows for specific names that are not covered in a __set() magic method.

Seems there's no way currently for magic properties to express a way that the types accepted by setters are different than the type(s) returned by getters.


This whole thing however matters for static analysis.

When you e.g. use phpstan (I don't know at which level this starts), it will take @property into account and the situation:

  • before this PR, it complains about writing data to the property (e.g. assign int or string)
  • after this PR, it complains about reading data from the property
    E.g. you've code like this
    $model->some_date->timezone(…)->toDatestring();
    will now complain
    Cannot call method timezone() on DateTimeInterface|int|string

Unless we find a way to properly express this input/output behaviour, I would argue:

Properties are more often read than written (*)

and thus would object to change this.

(*) no scientific data here, just checked a couple projects and the read:write ratios where like "100:1" so to say

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.

None yet

2 participants