Hum, mixed fellings about this since I use a different approach to handle dev and prod. What I do is have two HttpErrorHandler implementations (DevHttpErrorHandler and ProdHttpErrorHandler) and then have different configuration files for each environment.

Also, if we decide to have these methods, shouldn't this be available at HttpErrorHandler level?

The HttpErrorHandler interface only defines the onClientError and onServerError methods. I think this should stay as it is. The DefaultHttpErrorHandler just enhances the HttpErrorHandler with all the specific methods like onBadRequest, onForbidden, etc. to make it easy for the user to override this probably most used error pages.

Your approach of having different implementations for the HttpErrorHandler, depending on the environment, is of course a reasonable solution, but is maybe not desired or overkill for other Play users (even less for newcomers...)

However I think it absolutely makes sense to split the methods into onDevNotFound and onProdNotFound:

• The same is true already for onServerError: onDevServerError and onProdServerError
• I the docs we recommend to extend the DefaultHttpErrorHandler and even show how to use onProdServerError. So if we recommend to extend from DefaultHttpErrorHandler I think we should make it easy to override the not found page for production as well. Right now you always have to manually add a isProd if-switch. (Someone probably hardly ever wants to override the notfound page for dev mode)

I think this woud make it much more easier/straightforward for people to just override the not found page for production like they can do with server errors already.

I think binding separate HttpErrorHandler implementations is more flexible. Using separate implementations also means you're not limited to just server errors and not found errors. If we were going to be totally consistent each of those error handling methods would have a "dev" and "prod" method, but that seems a little bit ugly.

I think separate bindings would also be cleaner to test. You can explicitly override the binding to be a ProdHttpErrorHandler even though the mode is Test.

If we ignore the fact that onDevServerError and onProdServerError already exist, maybe it would make sense to instead recommend in the documentation different bindings/configuration for dev and prod?

Your approach of having different implementations for the HttpErrorHandler, depending on the environment, is of course a reasonable solution, but is maybe not desired or overkill for other Play users (even less for newcomers...)

Since I already have other settings that vary according to the environment, it does not add a burden. But I have a problem with APIs trying to handle more than one concept here (how to handle errors and dealing with the running mode). See more below.

I think separate bindings would also be cleaner to test. You can explicitly override the binding to be a ProdHttpErrorHandler even though the mode is Test.

That is exactly why I prefer this approach and consider it lighter. Testing it is way more easier since I don't have to worry about the running mode.

If we ignore the fact that onDevServerError and onProdServerError already exist, maybe it would make sense to instead recommend in the documentation different bindings/configuration for dev and prod?

I think this is a place were a "private" API leaked. Most of the time, users should not worry about the running mode, but about what the application must do. It is Play responsibility to worry about how to different running modes affect the application (hot reloading or not, which default error page to show, etc).

I think this is a place were a "private" API leaked. Most of the time, users should not worry about the running mode, but about what the application must do. It is Play responsibility to worry about how to different running modes affect the application (hot reloading or not, which default error page to show, etc).

@marcospereira You may be right. See @richdougherty's comment on #3444:

Passing around Mode.Mode is a pet peeve of mine. It would be nice to get rid of the separate methods for dev and prod mode and instead bind different error handlers for dev and prod mode. However I don't think we can easily do this at the moment, because by binding different dev and prod mode error handlers we would make the default error handling logic into something that can only be known at runtime. So if a user wants access to the default error handler, they can't access the logic by overriding DefaultErrorHandler. They would need to get the error handler bound at runtime. Unfortunately if they bind their own error handler then they would override any default error handler.

So I agree that for the default error handling logic, we can do a check for mode. This way we know we are overriding Play's default logic no matter what context we're using the error handler in. But we never addressed what the right thing is for a user to do.

On another note, the dependencies of DefaultHttpErrorHandler are more broad than they need to be. The constructor really only needs the Mode, not the whole Environment. If you were to create a ProdHttpErrorHandler you might want to explicitly set the mode to prod without having to pass the whole Environment:

class ProdHttpErrorHandler @Inject()(conf: Configuration, sm: OptionalSourceMapper, router: Provider[Router])
  extends DefaultHttpErrorHandler(Mode.Prod, conf, sm, router)

@mkurz you have to rebase with master because of #6001.

Also, onDevServerError and onProdServerError are called after we create a UsefulException to pass to that method and log the error. In this case, you're just doing a mode check, so it would be easy to implement yourself if you wanted it.

Separate point: I don't like the naming. DevServerError and ProdServerError are not two different types of events. Method names that start with on should be followed by the event name. Perhaps onServerErrorDev/onServerErrorProd? (It would certainly better to avoid it somehow, like by having a different handler's onServerError method be called.)

Actually, if we agree that separate dev/prod bindings is a better way to go, then I would just have one method (not sure about the naming) handleServerError(request, usefulException) that is delegated to from onServerError. If we did this, we'd want to deprecate the old methods and provide clear documentation on the right way to do it, though.

I am closing this in favor of #6171