Upgrade to Laravel 7

[cbj4074]

@mblarsen Can you take a look at the changes I made to the tests, in particular? If you look at the Files changed list, there are 3 test files near the very bottom of the list.

I'm not sure whether that is the correct "fix" for them failing upon upgrading from Laravel 6 to 7.

As a bit of background, it seems that, for whatever reason, the way in which integers are quoted when Eloquent Resources are presented as JSON has changed in Laravel 7. Specifically, integers (and perhaps all numeric values) were not quoted in Laravel 6, but in 7, they are.

For example, a year, such as 1980, was not quoted in the JSON HTTP response before, but now it is.

In any case, I'm wondering if altering the test is the wrong "fix", and if instead, we should be altering the response data instead, e.g., using the Custom Eloquent Casts that are new in Laravel 7.

[cbj4074]

@mblarsen Here's a screenshot of the diff when the test fails (expected on left, actual on right), which may help to articulate the problem.

[mblarsen]

@cbj4074 so I think I need a little background on your "model philosophy" I can see that you are not using casts on your Album model. That is ideally where this would handled rendering getJsonStructure unnecessary.

This would also ensure that you get the desired JSON object types for numbers and booleans.

Is there are reason why you are not using casts? On the surface it seems wrong to try to implement this yourself, but maybe you have a good reason for it?

class Album extends Model
{
    protected $guarded = ['id'];

+    protected $casts = [
+        'has_explicit_lyrics' => 'bool',
+        'is_active' => 'bool',
+        'year' => 'int',
+    ];

Then you can simplify the getJsonStructure to this:

    protected function getJsonStructure(Album $album)
    {
        return AlbumResource::make($album)->jsonSerialize();
    }

In a few cases you'll have to exclude some of the values because they are loaded in the test but not in the API. See commit.

[mblarsen]

Hmm, cannot push to the PR. Anyway the change is just what I proposed in the message above. Plus:

    public function testStoreWithValidInputReturnsOkStatusAndExpectedJsonStructure()
    ...
                'data' => Arr::except($this->getJsonStructure($album), ['id'])

    public function testUpdateWithValidInputReturnsOkStatusAndExpectedJsonStructure()
    ...
                'data' => Arr::except($this->getJsonStructure($album), ['artist'])

[mblarsen]

Anyhow, if you are not against casts on the models I can go through them all and update the casts accordingly.

[cbj4074]

@mblarsen Thanks for taking a look!

You're absolutely correct; the casting should be done in the model. I simply overlooked that.

I'm tied-up for another hour or so, but when I'm back, I'll work through this with you!

[mblarsen]

FYI. I you didn’t notice I did manage to push to the PR after all. May b easier to see the changes in the AlbumControllerTest

[cbj4074]

Hah! I did notice that you were able to push to my branch, after all. I was going to say, I could have sworn that I checked Allow edits from maintainers. 😉

This looks much cleaner with your changes; thank you!

I'm just updating my tooling here and will re-run the tests with our mutual changes shortly. If everything looks good, let's merge this PR and perhaps open a separate one for the additional casting changes that we should make.

Sound good?

[mblarsen]

Sounds good. Do you want me to make those changes?

[cbj4074]

Yes, that would be great!

[cbj4074]

Okay, I just pushed a couple of fixes/tweaks and now all tests are passing without any modifications to the tests themselves (as it should be).

If you can review and approve (provided you don't see any other issues) whenever you have a chance, that'd be great!

[cbj4074]

Yes, that's correct. Do you see any downside to that change? Should we have done something different there?

[mblarsen]

Yes, that's correct. Do you see any downside to that change? Should we have done something different there?

Nope, that's fine :) — I can begin on the casting stuff when this is merged.