Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
A good example of why this is a problem is explained in #1906810: Require type hints for automatic entity upcasting. But in a nutshell, an exception is thrown for a circular dep (for a test testing there is no circular dep :)) but exception caught in index.php are just printed to screen. PHP will default to a 200 in this case, as as far as it knows, it's just printing some strings.
Proposed resolution
Make sure we set the response status code to 500 in this case.
Remaining tasks
Do it.
User interface changes
None
API changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | circular-dependency-2284111-9.patch | 3.13 KB | Berdir |
| #5 | 2284111.5.patch | 4.08 KB | alexpott |
| #4 | 2284111-3.patch | 8.14 KB | Berdir |
| #1 | 2284111.patch | 415 bytes | damiankloip |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedLet's see what fails then, LocaleLocaleLookupTest I am expecting at the very least.... (I would be very surprised if that was the only one.)
Comment #3
damiankloip CreditAttribution: damiankloip commentedWell, well. It is the only one! :)
Comment #4
BerdirThis works but :(
Comment #5
alexpottThis works too - and has the advantage of removing the double static cache of base field definitions in the EntityManager and ContentEntityDatabaseStorage.
Comment #6
catchComment #7
damiankloip CreditAttribution: damiankloip commentedThat fix looks good to me. Let's wait for berdir to rtbc.
Comment #8
BerdirAs discussed in IRC, while the patch from alexpott fixes this test, basic_auth for example is still broken in HEAD and currently has no test coverage for this.
The problem there is that it puts a content entity object into the current_user service, which results in a loop when LocaleLookup tries to fetch the roles from it.
So while we could get this in to fix the test, we will need another critical for basic_auth, where we either need TranslationWrapper just for User because we say that is special, for all entities, possibly automated, so they don't have to do it manually, or we refactor LocaleLookup somehow to not to fail if this happens, which could however result in broken caching or we define that doing this is not allowed (putting a user entity into current_user during bootstrap or calling t() at all before the current user is set).
Possibly a combination of something and the last anyway, because calling t() before we have a user is certainly also before we have the correct current language and therefore bogus anyway.
Comment #9
BerdirThis is what I meant.
The first problem are the t() calls in ContentEntityType and ConfigEntityType annotations but then it quickly goes down the rabbit hole all the way to field item propertyDefinition() methods. So my patch above won't fix this either, we'd have to update all of them too.
Comment #11
damiankloip CreditAttribution: damiankloip commentedMe and berdir discussed this on IRC. We think it would be ok to get the patch in #5 in with Alex's fix. We will then create a critical follow up for the bigger problem. IMO getting this patch in sooner will be beneficial to stop any other bogus test passes like the one here in LocaleLocaleLookupTest.
NR for patch in #5.
Comment #13
catchI'm OK with #5 + critical follow-up as well.
Comment #14
webchickOk, committed and pushed #5 to 8.x.
Marking fixed, although I guess we could just continue to use this issue as the follow-up?
Comment #16
damiankloip CreditAttribution: damiankloip commentedThanks webchick. I think I would like to see a new issue as a follow up anyway. Reusing issues is..meh. IMO. I like this to look back to as it matches the commit :) I will speak to berdir/alexpott about the follow up as they likely know a bit more detail than I.
Comment #17
BerdirOpened #2288123: Basic authentication broken for non-english sites.
Comment #18
damiankloip CreditAttribution: damiankloip commentedThanks!