[5.7] Adjust mix missing asset exceptions

[timacdonald]

Sooo...I promise after this I'll stop poking at the mix helper 😅 but while I was adding the tests around it I noticed this code path seemed a little weird.

Problem

Given this existing snippet:

if (! isset($manifest[$path])) {
    report(new Exception("Unable to locate Mix file: {$path}."));

    if (! app('config')->get('app.debug')) {
        return $path;
    }
}
return new HtmlString($manifestDirectory.$manifest[$path]);

If you are in debug mode and have an asset missing the following would occur...

1. Exception is silently reported using the report() global helper.
2. An Undefined index exception is thrown at $manifest[$path] in the following snippet:

return new HtmlString($manifestDirectory.$manifest[$path]);

The test added failed previously as it tries to report BOTH the silent report(new Exception(...)) and the Undefined index exception.

Fix

To remedy this I've made it only report the exception silently in debug mode, otherwise it will throw it. This does 2 things:

1. Only reports 1 exception in both debug and non-debug mode, instead of 1 in non-debug and 2 in debug mode.
2. Gives consistency to the exception message. Instead of undefined index you will get "Unable to locate Mix file: PATH"

Potentially breaking

These changes only change behaviour in debug mode. What is more the breaking changes are:

1. It doesn't report 2 exceptions.
2. The exception that is thrown has changed its message.

Which...I can't really see why would be a problem. But if it is, and this fix is wanted, I can push to master.

Bugsnag screenshot showing this duplicate reporting:

[austenc]

Discovered this was breaking some automated builds this morning. Is there a way to override or swap this out prior to #26289 being merged?

Seems that isn't going to come out until 5.8. Overall, the goal for us is to not need to run npm run prod on CI builds since that slows them down considerably. Previously I had worked around this by spoofing the manifest file with touch public/mix-manifest.json in the CI build script.

[timacdonald]

@austenc I know you commented on the other PR, but I just want to confirm - are you saying the merging of this PR broke your builds?

Are your tests relying on the Undefined index exception and that is why it broke? Could you post the failing error message just to confirm?

[austenc]

@timacdonald Sorry, I worded that last comment poorly. Firstly, I'd like to say that this PR and the others related to this mix code are great additions to the framework, so thank you for submitting them! My tests aren't relying on any exception messages in particular. Here's what we were doing:

Prior to this update, we were running touch public/mix-manifest.json (which admittedly seemed like a bit of a hack) in our build scripts. This allowed us to run the tests without having to run npm commands, making them much faster. After updating to the version of 5.7 that has this merged, the Unable to locate Mix file errors started popping up. Not to say they weren't there before, they were just silent. The problem this causes though, is now (until your other PR is merged) there is no official way to disable mix for the tests. We've worked around it for now by manually spoofing / touching our frontend build files in the CreatesApplication trait.

Overall, I think this a move in the right direction since your changes make the exceptions behave more predictably. Seems like the other PR could be merged in the next version of 5.7. What do you think?

[timacdonald]

Hey - I'm glad you find these changes nicer. I've also been doing the hack you mentioned, which lead me to all these mix tweaks, so I'm glad to hear someone else will benefit from them.

The thing I'm a little confused about is if you look at the tests that we changed, before this PR was merged you should have still had an exception thrown - all I've done here is make the message more explicit.

// before this was thrown because the array didn't contain the string...
new Exception("Undefined index: /missing.js")


// after we are now just explicitly throwing this...
new Exception("Unable to locate Mix file: /missing.js.")

If you run the tests on your local machine and change the mix function back and forth with this change does it pass / fail?

We should probably ping @driesvints here so he can also check this out because it's very possible I'm missing something.

[timacdonald]

Note: you can declare you own mix method in your apps helpers.php with the old version as a quick fix until this is resolved for you.

[austenc]

You know... As I wrote that response this morning I realized I hadn't isolated it specifically to this change. It was that we hadn't changed our code much and after updating it was all of a sudden broken. I'll do some testing between patch releases of the framework tomorrow and report back!

…

On Sat, Dec 1, 2018, 8:41 PM Tim MacDonald ***@***.*** wrote: Note: you can declare you own mix method in your apps helpers.php with the old version as a quick fix until this is resolved for you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#26431 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjHvcMqqhbPBMJjaUbT71j21rmfTk40ks5u00vWgaJpZM4YTrzg> .

[timacdonald]

Hey @austenc might try and keep the conversation in this thread so I'll reply to you comment (#26289 (comment)) here to keep everything together.

I can see this issue happening when:

1. The app is in debug mode.
2. The mix manifest has no contents.

The reason I didn't pick up on this is because of this lovely PHP "feature": https://3v4l.org/An2rJ

I didn't account for this in the additional tests which is why they passed.

This is a test that fails at the moment, but passes with this change reverted...

// Illuminate/Tests/Integration/Foundation/FoundationHelpersTest.php

public function testMixSilentlyFailsWhenManifestIsEmptyInDebugMode()
{
    $this->app['config']->set('app.debug', true);
    $manifest = $this->makeManifest();
    file_put_contents($manifest, null);

    $result = mix('missing.js');

    $this->assertSame('', $result->toHtml());

    unlink($manifest);
}

and for comparison, this test fails when the previous one passes...

public function testMixSilentlyFailsWhenManifestIsNotEmptyInDebugMode()
{
    $this->app['config']->set('app.debug', true);
    $manifest = $this->makeManifest();
    file_put_contents($manifest, '[]');

    $result = mix('missing.js');

    $this->assertSame('', $result->toHtml());

    unlink($manifest);
}

So it is how PHP handles calling []['key-that-does-not-exist'] and null['key-that-does-not-exist'] that has caused this. See: https://3v4l.org/haGa9

Perhaps we can revert this to make sure people doing this while testing are not affected and then we merge this bug fix into master.

---

Side note: Looking at your comment in the other thread, you shouldn't need to create the actual css files etc, pretty sure you can get away with just this...

file_put_contents(public_path('mix-manifest.json'), json_encode([
    "/js/app.js" => "",
    "/css/style.css" => "",
    "/css/print.css" => "",
]));

because it just needs the keys to be in the array

[austenc]

@timacdonald Thanks for the explanation and the tip about the manifest! That does indeed work for me.