[5.4] Mix() wont throw an exception in production mode.

[adampatterson]

Return $path if env is in production mode. Otherwise, throw an exception.

ref #20355

[laurencei]

Should it at least do an entry to \Log::debug() or something?

Otherwise you wont know it failed at all?

[adampatterson]

@laurencei I'm not sure how far you need to go with missing assets. But I would appreciate seeing something show up in Sentry.

I don't do much custom logging. But if \Log::debug() is independent of Sentry then no. Only because out sites on Docker and we won't have access to those logs.

[laurencei]

Writing to the log should trigger an event, which all the log tracking services should pick up on.

Tbh though - its not about Sentry or Docker. It's about logging that fact there was a failure - and not just silently failing.

I agree that not everyone will care about missing assets - but I know I would - so logging it as debug (or maybe info) gives people the ability to action if they want.

[adampatterson]

@laurencei good insight, Let me see what I can do. What kind of messaging makes sense?

Unable to locate Mix file: {$path}. Please check your webpack.mix.js output paths and try again.

This is what Momolog caught:

[2017-08-01 15:21:09] production.WARNING: Unable to locate Mix file: /static/js/wrong.js. Please check your webpack.mix.js output paths and try again.

[laurencei]

That looks good to me tbh...

p.s. I just noticed this is your first contribution to Laravel. Welcome to the community and thanks for taking the time to submit a PR on this issue. Hopefully it's the first of many 😄

[adampatterson]

@laurencei Thanks!

[laurencei]

Sorry - I gave you some bad advice.

It should be app('log')->info($mixMessage); as we are inside the helpers.php file.

[JeffreyWay]

@taylorotwell Seems like a reasonable compromise to me. Agree that it's a bit dramatic for a missing asset to 500 in production.

[adampatterson]

@laurencei can you explain why we would use app()->info() over \Log::info()?

[laurencei]

I'm wondering if checking for production is the right check? It might make more sense to check if we are in debug mode or not? That future proofs against people in staging environments etc?

So something like if (! app('config')->get('app.debug')) {

[adampatterson]

Good point! Updated

[dwightwatson]

I get that a 500 feels pretty harsh for a missing asset but I think it's also sort of reasonable. If you're attempting to use an asset that doesn't exist, something has gone wrong.

I actually ran into this just a day ago, during deployment the Mix compilation failed silently on just one of our servers - only found out about it because it popped up in Sentry.

Could we consider using some sort of MissingAssetException which would allow developers to handle this however they like?

[adampatterson]

@dwightwatson The patch has been changed to add a log entry. It's a tough one really.

If you are missing your entire vendor file, then yes. Something should let you know ASAP. If you're missing a footer logo. Well, then not such a big deal.

I know that every time I deploy I kick around doing a quick once over. The site I work on has a few hundred people a day looking at it internally and 20k+ users a day. For us, it's better to fail than to cripple.

I would love to see something show in Sentry but I think that could only happen if the asset was loaded through a server and triggered some other kind of exception. I then think server setup is another issue.

[dwightwatson]

If you're referring to a footer logo that doesn't exist I think that's still an issue. I don't think it's fair to start judging assets on a file-by-file basis as to whether they should throw an exception or not. If your app is trying to load something that doesn't exist, that's wrong, it's generally exceptional and I think it deserves to be treated as such.

My team can deploy any number of times throughout the day, and I don't think they should have to hit the site manually after each deployment to see if the assets still work. In my case they may have still missed it, as it was only one of six servers that failed, so there's only 1/6 chance they would have seen it. In addition, I really don't want to have my customers coming to me and telling me that my assets are broken - I should already know.

I get both sides of the argument here, I know it comes down to is it better to load a page with no assets vs. no page at all. I know a 500 is harsh but I believe it's fair because something has gone wrong.

Just dropping my 2 cents (maybe even 4 cents at this point) but I'd like to see it stay as-is.

[laurencei]

I actually ran into this just a day ago, during deployment the Mix compilation failed silently on just one of our servers - only found out about it because it popped up in Sentry.

But isnt this why we have logs? The issue is still logged - so you can see it.

The question is to do you fail the whole thing because an asset is missing - or try to allow it to run anyway. Either way - you will still know about it.

[taylorotwell]

I just pushed a new report($e) helper in 5.5 which accepts an exception and then calls the "report" method of your exception handler. I would use that instead of Log::info. Then let the exception handler decide how to report it / send it to Sentry / Bugsnag, etc. Then you can return empty string or null or whatever after calling that.

This helper has been pretty useful in my own projects.

[adampatterson]

@taylorotwell Should I make a new PR for 5.5?

[mathieutu]

I agree with #20376 (comment).

But what about the exception thrown even in debug mode? (in cases we absolutely don't care about assets)

This Exception thrown problem was discussed in laravel/ideas#634, and a complete, fully tested and collaborative solution was proposed in #19895.

@JeffreyWay what do you think about these?

[taylorotwell]

This should definitely go to 5.5 regardless, and report method will have to be used instead of logging.