Blocks #606840: Enable internal page cache by default.

This fixes the 15 failures o/t parent issue in:

  • Drupal\config\Tests\ConfigDependencyWebTest (4)
  • Drupal\config\Tests\ConfigEntityFormOverrideTest (2)
  • Drupal\config\Tests\ConfigEntityTest (9)
CommentFileSizeAuthor
#11 interdiff.txt4.78 KBwim leers
#11 config_test_routes-2460677-11.patch7.65 KBwim leers
#9 interdiff.txt2.96 KBwim leers
#9 config_test_routes-2460677-9.patch5.06 KBwim leers
#1 config_test_routes-2460677-1.patch2.16 KBwim leers

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new config_test_routes-2460677-1.patch2.16 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC, Config Entity Test s should not rely on data being available as anoymous user.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Can we have a quick explanation *why* it fixes those tests? What does page cache have to do with it?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Sure.

The config_test routes have _access: 'TRUE' set in their requirements. This means any user can access them, including the anonymous user. Since the tests currently use the anonymous user, the pages end up in the page cache.

The better fix would be to replace _access: 'TRUE' with an actual permission check. But that also means creating a custom permission, setting up a role with the right permissions, creating a user, etc.
Hrm, actually, no, the root user is user 1, which has all permissions by default. So it'd only require setting up a custom permissions for this test-only module. Which still seems overkill. But if somebody has a good reason to prefer that, then I'd be okay with that :) Feel free to mark NW if you think we should do that.

fabianx’s picture

fabianx commented

In the end however the we still would need to login. It is just that the tests would fail with page cache off, too and anonymous user, which I don't think is needed to fix this.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

It's not needed to fix this - but it is need to prevent us adding any new usages of these routes without logging in in the meantime. Let's replace _access: 'TRUE' with a perm.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new config_test_routes-2460677-9.patch5.06 KB
new interdiff.txt2.96 KB

Done.

First, I added a new permission. Then I realized we can simply reuse 'administer site configuration', that's sufficient for this test AFAICT.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: config_test_routes-2460677-9.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new config_test_routes-2460677-11.patch7.65 KB
new interdiff.txt4.78 KB

Three additional tests now had failures, because I updated all config_test routes to require a permission.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Looks good now :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed a739166 and pushed to 8.0.x. Thanks!

  • alexpott committed a739166 on 8.0.x 
    Issue #2460677 by Wim Leers: Tests testing config_test routes should use...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.