Add minimal 4xx responses for invalid image derivative requests

I realize this needs tests, but I wanted to send it for an initial once-over by subsystem maintainers to see if there are any objections to the feature request or the approach. Thanks.

I didn't originally understand the report in slack, but this is clearer. To summarise:

Fast 404s are now compatible with image style generation, because the fast 404 kicks in after routing (i.e. when no route is found).

However, some of these errors aren't 404s at all, so they don't get the minimalist response. Manually sending minimalist 400s seems OK for these.

What I'm not clear on is why you're not getting a fast 404 though, can you check both settings.php and system.performance.yml and make sure you're not excluding these URLs or images in general? Last patch on 🐛 Fast 404s are slower than regular 404s Fixed has hints on what to look for.

Seems to CC failure. But some information was requested in #6. Maybe could be PNMI but will leave in NW for now.

I didn't even catch that we're returning 400s in some cases and 404s in others because I mostly just copy/pasted some of the changes from existing code elsewhere in the imageapi codebase. But it totally makes sense to return minimalist 400 "Bad Request" responses with an appropriate brief message explaining the error if any part of the URL is malformed. For one, a 400 response sends a signal to upstream caches like Varnish or CDNs that the request is and will always be invalid, and can therefore be cached, almost indefinitely. Whereas a 404 is generally not going to be cached long term, because even though the URL is "not found" it might exist at some point in the future. Only time we should ever return a 404 from the image derivative routes is when the source image is not found on the server. And even then it could and should be a minimalist 404.

Will address q in #6 next...

Ugh. I didn't realize we had to create a different MR when switching the versions, instead of just rebasing the original MR on the lastest upstream branch. 🤦‍♂️

jwilson3 → closed merge request !4714

jwilson3 → closed merge request !4716

I was able to get the original MR rebased on 11.x, finally. My apologies for the MR mess above.

What I'm not clear on is why you're not getting a fast 404 for the actual not found error though, can you check both settings.php and system.performance.yml and make sure you're not excluding these URLs or images in general?

To be honest, I'm not sure what I'm supposed to configure, or where. Based on the work on the last patch that was now committed to Drupal core to remove fast_404 configurations from settings.php it seems there is nothing to configure anymore, right?

Anyway, I can confirm that if you follow the steps in the issue summary on a clean Drupal 10.1.2 installation on simplytest.me, you'll get the full 404 page if you specify an incorrect itok value for a valid image URL.

I've updated existing tests and they pass locally, so ... fingers crossed ...

My question is whether we need to add more explicit tests to ensure the responses A) contain the expected error messages and B) constitute a minimal response without a full Drupal bootstrapped 404 page.

Yay. Back to NR.

This looks good!

See tests have been added so removing that tag.

Believe a change record will be needed for the new service so moving to NW for that.

But rest appears to be good!

1. I've added API Changes and Release Notes snippet to issue summary.
2. I've created the draft change record here: https://www.drupal.org/node/3386269 →

CR reads well.

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions. The latest answer by @jwilson3 does need to be reviewed.

I read the MR and spotted a coding standards mistake and while adding a suggestion for that I also added some for comments. I didn't examine the test changes but it would be good to know if all the codes changes are tested.

Setting to needs work for those items.