Added support for Laravel 7 custom casts #913
Conversation
There was a problem hiding this comment.
just had one problem with this: if return type of
getmetod is not defined, but it has@returnstag, then returned type is not namespaced properly. i couldn't find a way to provide necessary context toDocBlockobject. if anyone knows how to do it, that would be really cool to implement
The whole library has this limitations on get*Attribute methods so IMHO nothing which needs to be fixed specifically for this feature (but sure, a general solution would be "nice").
I like the PR, implementation LGTM and 👍 for tests!!
|
|
||
| $this->assertSame(0, $tester->getStatusCode()); | ||
| $this->assertEmpty($tester->getDisplay()); | ||
| //dd($actualContent); |
|
|
||
| if (version_compare(Application::VERSION, '7.0', '<')) { | ||
| $this->markTestSkipped('This test requires Laravel 7.0 or higher'); | ||
| return; |
There was a problem hiding this comment.
Not needed, markTestSkipped works through exception bubbling
There was a problem hiding this comment.
found it only in AbstractModelsCommand but it checks for 6 version
if (version_compare(Application::VERSION, '6.0', '<')) {
$this->markTestSkipped('This test requires Laravel 6.0 or higher');
return;
}are you sure i can safely remove it? where is this exception debugging you are talking about?
There was a problem hiding this comment.
markTestSkipped throws a special exception recognized by PHPUnit, that's why a return (read: code afterwards) doesn't execute => https://github.com/sebastianbergmann/phpunit/blob/cb6b2d8cb5865b9576adcd9c595a8b4d005361b3/src/Framework/Assert.php#L2645-L2655
There was a problem hiding this comment.
yeah but to be honest, i cant see why i can drop this code
parent class check for 6 version, but my test should run only in 7
There was a problem hiding this comment.
It can/should be removed in the parent as well.
There was a problem hiding this comment.
sorry, but i don't follow why
travis runs test on multiple laravel versions, how else this test would be skipped without this code?
There was a problem hiding this comment.
oh, you talking only about line 20, we can remove return statement, agreed, thanks!
There was a problem hiding this comment.
Yes, exactly; apologies for being not more specific upfront!
|
refactored tests and added some additional test cases |
|
Thanks! Unfortunately this has merge conflicts. Would it be possible to fix those, then I'll merge this. |
|
done, replaced my code with @mfn's |
|
Thanks! |
this pr adresses #904
the idea of improvement is simple:
now we get cast object as property's return type
we just check if this cast object implements laravel's
Illuminate\Contracts\Database\Eloquent\CastsAttributesinterface and if so then we return it'sgetmethod return typejust had one problem with this: if return type of
getmetod is not defined, but it has@returnstag, then returned type is not namespaced properly. i couldn't find a way to provide necessary context toDocBlockobject. if anyone knows how to do it, that would be really cool to implement