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

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new 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
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
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
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
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
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
commented
Status: Needs work » Needs review
StatusFileSize
new drupal_2177093_9.patch20.18 KB

Re-roll.

sun’s picture

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

dawehner
Primary language German
commented

back to rtbc

sun’s picture

sun
Primary language German
Location Karlsruhe
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 commented
Status: Needs work » Needs review
StatusFileSize
new 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 🇪🇺🌍
commented
StatusFileSize
new 14-16--interdiff.txt570 bytes
new 2177093.16.patch19.94 KB

Fix the remaining test failures.

alexpott’s picture

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

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

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
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.