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)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
2.16 KB
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Fabianx’s picture

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

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

Wim Leers’s picture

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

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

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

Status: Needs work » Reviewed & tested by the community
FileSize
5.06 KB
2.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

Status: Needs work » Needs review
FileSize
7.65 KB
4.78 KB

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

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Looks good now :)

alexpott’s picture

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.