Baby clean-up step for #1541298: Remove Node module dependency from Testing profile

Problem

  • Many test modules are using the 'access content' permission for their routes, even though they have nothing to do with Node module.

Proposed solution

  1. Replace all _permission: 'access content' with _access: TRUE in test module routes.
  2. Do not introduce new ones. ;)
CommentFileSizeAuthor
#16 2177093.16.patch19.94 KBalexpott
#16 14-16--interdiff.txt570 bytesalexpott
#14 drupal8-remove_access_content_perm_test_modules-2177093-14.patch19.94 KBInternetDevels
#9 drupal_2177093_9.patch20.18 KBXano
#1 drupal8.test-route-access.1.patch19.93 KBsun
drupal8.test-route-access.0.patch19.82 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal8.test-route-access.1.patch19.93 KB

_access: true yields

Routing requirement for "_access" must be a string.

→ only _access: 'TRUE' works.

Ugh.

The last submitted patch, drupal8.test-route-access.0.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Shouldn't we also remove the "access content" permission from created users in tests? This would be certainly harder to find them all.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Those tests rely on the fact that testing profile grants that permission to the anon and authenticated user role, so they all have it by default, just like the standard profile used to.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Removing the permission assignments remains to be done for #1541298: Remove Node module dependency from Testing profile

For this issue here, I just want to get rid of access permission requirement in all test module routes, because that makes up ~60% of the patch in #1541298.

The earlier this gets in, the smaller is the chance that someone will copy/paste an existing test module route with this permission. Historically, that copy/pasting behavior is the root cause for why we have all these bogus access requirements.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs review » Reviewed & tested by the community

I confirmed that this patch only removes the access checks from routes that belong to testing modules, and that testing_install() indeed grants the removed permission to anonymous and authenticated users, as @Berdir said in #4.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

1: drupal8.test-route-access.1.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: drupal8.test-route-access.1.patch, failed testing.

Xano’s picture

Xano
he/they
Primary language English
Location Southampton
CreditAttribution: Xano commented
Status: Needs work » Needs review
FileSize
drupal_2177093_9.patch20.18 KB

Re-roll.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

back to rtbc

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

9: drupal_2177093_9.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: drupal_2177093_9.patch, failed testing.

InternetDevels’s picture

InternetDevels CreditAttribution: InternetDevels commented
Status: Needs work » Needs review
FileSize
drupal8-remove_access_content_perm_test_modules-2177093-14.patch19.94 KB

Added new patch.

Status: Needs review » Needs work

The last submitted patch, 14: drupal8-remove_access_content_perm_test_modules-2177093-14.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
14-16--interdiff.txt570 bytes
2177093.16.patch19.94 KB

Fix the remaining test failures.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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