Spin-off from #913086: Allow modules to provide default configuration for running tests

Patch literally extracted from that issue; has been peer-reviewed plenty of times already.

Problem

  • drupalCreateUser() throws an error when Comment module is not installed.

Goal

  • Remove dependency on Comment and Node modules from testing API.

Details

  • drupalCreateUser() attempts to create a new user with a set of default permissions pertaining to Node and Comment modules.
  • When Comment or Node module is not installed, drupalCreateUser() fails to create a user role with the default permissions, because the permissions do not exist.
  • Those default permissions are superfluous anyway, because they are assigned by default to the authenticated user role, which applies to all authenticated users. Thus, it is pointless to create a new user role for the default permissions.

API changes

None. As detailed above, the removed permissions are the default permissions either way.

Impact

This patch is contained in several other patches already, which will have to be re-rolled after this lands:

#375397: Make Node module optional
#913086: Allow modules to provide default configuration for running tests

Files: 
CommentFileSizeAuthor
#5 drupal8.test-drupalcreateuser.5.patch1.87 KBsun
PASSED: [[SimpleTest]]: [MySQL] 37,163 pass(es).
[ View ]
drupal8.test-drupalcreateuser.0.patch1.88 KBsun
PASSED: [[SimpleTest]]: [MySQL] 34,164 pass(es).
[ View ]

Comments

FWIW, +1 on the RTBC. :)

Updated the issue summary to explain the change.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Yeah this looks completely fine to me, the fact no tests need to be changed at all shows these permissions as redundant.

The only caveat here is that if we change the default permissions for the authenticated user role, then tests might need to be updated, but I doubt we'll do that for comments anyway, and really only tests in comment module should break in that case (and rightly so). So committed/pushed to 8.x. fwiw I think it looks like a good change for 7.x as well but not sure what we gain from the backport? That might need fleshing out a bit, but up to webchick of course.

Status:Patch (to be ported)» Reviewed & tested by the community
StatusFileSize
new1.87 KB
PASSED: [[SimpleTest]]: [MySQL] 37,163 pass(es).
[ View ]

Thanks!

Identical patch for D7, only change is /core dir.

Status:Reviewed & tested by the community» Fixed

At first glance, this seemed like a no-go for D7 since it looked like it would change the default permissions assigned to users out from under various test authors' feet. However, as explained both in this issue and in the PHPDoc of drupalCreateUser(), what it's actually doing is avoiding the work of creating additional roles if there are no additional permissions, and just defaulting them to whatever auth user has. Makes sense.

So this seems like a harmless change for D7, although I guess if anyone is using their own custom testing profile, and they've customized the default permissions for auth user to not include the comment permissions, and they are nevertheless depending on all users having comment-related permission already in their tests, they'll see breakage. That seems pretty edge-casey to me. It seems far more likely that the code as it is would create a WTF for them since they never specified comment permissions, and so would have to explicitly undo that. I guess in any case, we still have enough time before the release next month for someone to catch that on their dev server, if it bites anyone. Let's give it a shot.

Committed and pushed to 7.x. Thanks!

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

Updated issue summary.