Blocks #606840: Enable internal page cache by default.
This fixes the 34 failures o/t parent issue in:
\Drupal\user\Tests\UserRegistrationTest
This test modifies the field_config
, field_storage_config
entities for the User entity, as well as the 'register' form display for User entities. It modifies each of them individually, and then verifies that the registration form is updated accordingly. But due to page caching being enabled (in the parent issue), those changes are not actually reflected.
But, really, this is the only example in core that points to a wider problem. This is the only example in core of an entity form that is accessible by anonymous users (the only other one is the comment form, but that is special, because it's rendered via #post_render_cache
— see #2099131: Use #pre_render pattern for entity render caching). But the wider problem is that entity forms don't have the necessary cache tags associated.
Comment | File | Size | Author |
---|---|---|---|
#19 | entity_form_display_cache_tags-2463029-19.patch | 5.31 KB | Wim Leers |
Comments
Comment #1
Wim LeersUserRegistrationTest
does this at the very end:i.e. it test the exact same thing, but with different input values, both with JS (AJAXy form) and without JS (regular form). Each separately works fine. But running both in succession causes this failure on my machine:
I'm hoping it's something that's specific to my machine…
Comment #2
Wim LeersComment #3
Wim LeersComment #4
Wim LeersAnd tests.
Comment #7
Wim LeersComment #9
BerdirTo clarify: There's nothing special about the user registration form, user registration form isn't the only entity form that is visible to anon users (not even the only one by default), we just don't have test coverage for others. The contact form is also accessible to anon by default, for example.
You like this pattern, but I personally prefer having a real method for those helper methods, I'm not wondering how those are reported, for example. You can name a real method asssertSomething() and then call to it will be shown, which is more helpful.
Comment #10
Wim LeersFixed failures, addressed Berdir's concerns :)
Comment #11
Wim Leers#2444231: Fix CacheableInterface so it makes sense (was: "Make Config objects & Entities implement CacheableInterface + add BubbleableMetadata::createFromCacheableObject()") landed, which added
RendererInterface::addDependency()
, thus allowing me to simplify this.Comment #13
Fabianx CreditAttribution: Fabianx for Drupal Association commentedUh? Should this not be injected into the class?
Should these not check for CacheableDependencyInterface?
Is it ensured that $this always implements CacheableDependencyInterface?
Comment #14
BerdirEntityFormDisplay is an entity so yes, it always implements that interface, and we can't inject dependencies in entities.
Comment #15
Wim LeersFixed my super retarded mistake that caused >3K failures. Rerolling when in a hurry = bad idea.
#13:
Comment #17
Wim Leers#15 had the right interdiff, but the wrong patch. Here's the patch I meant to upload. Sorry for the noise.
Comment #18
Fabianx CreditAttribution: Fabianx for Drupal Association commentedI think the logic is not correct here:
Unless I miss something the field storage definition should be added regardless of the interface of the $field_definition.
E.g.:
The rest looks good!
Comment #19
Wim LeersOh, good point!
(Also fixed a rebase conflict; the changes to
WebTestBase
for improved debug output are no longer necessary, another issue landed them already.)Comment #20
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC, looks great now!
Comment #21
alexpottThis issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 4553018 and pushed to 8.0.x. Thanks!