Problem/Motivation

CommentHelperCase does not support the two ways two pass $modules that DrupalWebTestCase does. You can see how that leads to awkwardness in CommentNodeAccessTest::setUp() where DrupalWebTestCase::setUp() is called statically (?!) instead of parent::setUp()

Proposed resolution

Implement CommentHelperCase::setUp like the following:

function setUp() {
  $modules = func_get_args();
  if (isset($modules[0]) && is_array($modules[0])) {
    $modules = $modules[0];
  }
  $modules[] = 'comment';
  parent::setUp($modules);

  // Here comes the rest of the function as it was before.
  ...
}

Also change CommentNodeAccessTest::setUp() to use parent::setUp('search', 'node_access_test'); instead of DrupalWebTestCase::setUp('comment', 'search', 'node_access_test');

Remaining tasks

Commit the patch. :-)

User interface changes

None.

API changes

None, really. Technically this is an API change for people who sub-class CommentHelperCase, but it's rather fixing something that people expect to work anyway. I'm marking "needs change notification" anyway, because I guess it can't hurt to document this publically, even though few people will care. :-)

CommentFileSizeAuthor
#29 make_commenthelpercase-1569856-1.patch1.13 KBfaline
#23 support_modules_var_1569856_23.patch1.41 KBIvoryTierra
#12 support_modules_var_1569856_12.patch1.56 KBwizonesolutions
#10 support_modules_var_1569856_8.patch1.57 KBwizonesolutions
#8 support_modules_var_1569856_8.patch1.57 KBwizonesolutions
#5 support_modules_var_1569856_5.patch1.33 KBwizonesolutions
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Oh, and also CommentNodeAccessTest needs to be changed to use parent::setUp(). Edited issue summary accordingly.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

For D8, this duplicates: #1380958: Replace $modules list for WebTestBase::setUp() with ::$modules class properties

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
Assigned: Unassigned » wizonesolutions

Twin Cities sprint.

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
Issue tags: +tcdrupal2012
wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
Assigned: wizonesolutions » Unassigned
Status: Active » Needs review
FileSize
support_modules_var_1569856_5.patch1.33 KB

My first core patch evar attached.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work 
+++ b/core/modules/comment/comment.test
@@ -16,7 +16,17 @@ class CommentHelperCase extends WebTestBase {
+    // Since this is a base class for many test cases, support the same
+    // flexibility that DrupalWebTestCase::setUp() has for the modules to be
+    // passed in as either an array or a variable number of string arguments.
+

This comment and following blank line can be removed.

+++ b/core/modules/comment/comment.test
@@ -16,7 +16,17 @@ class CommentHelperCase extends WebTestBase {
+    $modules += array('comment');

This needs to be $modules[] or comment module won't be added if $modules is not empty.

+++ b/core/modules/comment/comment.test
@@ -16,7 +16,17 @@ class CommentHelperCase extends WebTestBase {
     // Create users and test node.

@@ -1516,7 +1526,7 @@ class CommentNodeAccessTest extends CommentHelperCase {
     // Create users and test node.

How much of the following setup code is further duplicated in CommentNodeAccessTest?

Note: It's possible that there are differences, so it might not be duplicated at all and needs to be retained then.

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
Assigned: Unassigned » wizonesolutions
wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
FileSize
support_modules_var_1569856_8.patch1.57 KB

Here's a modified patch that should address your comments. The user in CommentNodeAccessTest was unique, so I left that alone. Removed the other two lines.

Also changed the += to an = since it now uses $modules[].

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
Assigned: wizonesolutions » Unassigned
Status: Needs work » Needs review

Unassigning, status change.

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
FileSize
support_modules_var_1569856_8.patch1.57 KB

Guess I've gotta attach the patch again for testbot.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Needs work 
+++ b/core/modules/comment/comment.test
@@ -16,7 +16,13 @@ class CommentHelperCase extends WebTestBase {
+    $modules[] = array('comment');

This needs to be

$modules[] = 'comment';

Currently this would result in:

$modules = array(
  'foo',
  'bar',
  'baz',
  array('comment'),
);
wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented
Status: Needs work » Needs review
FileSize
support_modules_var_1569856_12.patch1.56 KB

Ah, good point. Fixed and attached.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Needs work 
+++ b/core/modules/comment/comment.test
@@ -1516,13 +1522,11 @@ class CommentNodeAccessTest extends CommentHelperCase {
-    $this->node = $this->drupalCreateNode(array('type' => 'article', 'promote' => 1, 'uid' => $this->web_user->uid));

Since this references $this->web_user, which is a different user than in the parent implementation, this line should not be removed.

Alternatively, we could just grant the user the 'node test view' permission. E.g. user_role_grant_permissions(DRUPAL_AUTHENTICATED_RID, array('node test view')). I haven't seen that in tests before, though, so maybe that's not wanted.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

There's nothing wrong with overriding $this->web_user from the parent class in this test.

This looks RTBC, but no idea why testbot doesn't come back green. Sending for re-test.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Well, since in this case only view permissions are required I guess it's OK do that, but it's still kind of weird, because:

$this->web_user->uid !== $this->node->uid === $this->web_user->uid

I can live with that, though.

sun’s picture

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

Good point + finding. But yeah, as long as the particular test doesn't rely on $this->node->uid, there's no harm here and this is good to go.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes

Updated issue summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue summary: View changes

Updated issue summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Updated issue summary, so that the code example actually resembles the latest patch.

RTBC++

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Issue tags: -Novice, -Needs backport to D7, -Needs change record, -tcdrupal2012

#12: support_modules_var_1569856_12.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Needs backport to D7, +Needs change record, +tcdrupal2012

The last submitted patch, support_modules_var_1569856_12.patch, failed testing.

wizonesolutions’s picture

wizonesolutions
he/him
Primary language English
Location Norway
CreditAttribution: wizonesolutions commented

The structure of the comment tests seems to have changed. Any guidance on what the changes were (seems like more abstraction) and how I could re-roll my patch appropriately if it's still necessary?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Yes the change are still needed.
What was CommentHelperCase before is now CommentTestBase and it no longer lives in core/modules/comment/comment.test but in code/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.php. The actual code changes should be (I think) the same as before.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: -tcdrupal2012 +Needs reroll

Tagging.

IvoryTierra’s picture

IvoryTierra CreditAttribution: IvoryTierra commented
Status: Needs work » Needs review
FileSize
support_modules_var_1569856_23.patch1.41 KB

Hi folks.... I am new to drupal and I have gotten lots of help from the awesome kbasarab with this patch.

It was not possible to re-roll so I have created a new patch. Please review and all comments are appreciated. Thanks!!!

--Tierra

PS The guys at mediacurrent are so great and knowledgeable! Thanks for the help! @drupalcampatlanta

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Version: 8.x-dev » 7.x-dev
Issue tags: -Needs backport to D7

Since #1380958: Replace $modules list for WebTestBase::setUp() with ::$modules class properties is now in D8, this is obsolete for D8. We should still do this for D7, though.

@IvoryTierra: I'm very sorry that your work on porting this to D8 was in vein. I didn't realize we still had this one open. Thank you very much for your contribution, though. If you still would like to bring this issue forward, you could back-port your new patch to D7 (or alternatively, use the patch in #12 as a starting point, I'm not sure which is easier).

Marking for D7

Gaelan’s picture

Gaelan CreditAttribution: Gaelan commented
Issue tags: -Needs reroll
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: -Novice, -Needs change record

#23: support_modules_var_1569856_23.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Needs change record

The last submitted patch, support_modules_var_1569856_23.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: +Needs reroll
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue summary: View changes

Updated issue summary.

faline’s picture

faline CreditAttribution: faline at CI&T commented
Status: Needs work » Needs review
FileSize
make_commenthelpercase-1569856-1.patch1.13 KB

Create the patch as described in the updated summary

cebasqueira’s picture

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

It's works for me!

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Issue tags: -Needs change record, -Needs reroll +Pending Drupal 7 commit

Why not :).

This is tests only and hence should be an eligible change for D7.

Marking for commit.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Pending Drupal 7 commit

Committed and pushed to 7.x. Thanks!

  • Fabianx committed 33013d3 on 7.x 
    Issue #1569856 by wizonesolutions, faline, IvoryTierra, tstoeckler, sun...
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented

FYI, Drupal 7.53 wound up just being a small bugfix release (to fix a single regression from Drupal 7.51) so this issue was not included there. It will be in the next non-security release instead.

Status: Fixed » Closed (fixed)

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