Replace $modules list for WebTestBase::setUp() with ::$modules class properties

Using a static property makes it hard to extend a base class and not have to manually specific the base class's modules. This is especially useful in contrib, not core.

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1387,6 +1387,14 @@ class DrupalWebTestCase extends DrupalTestCase {
+    $class = $this;
+    while ($class) {
+      if (property_exists($class, 'modules')) {
+        $modules = array_merge($modules, $class::$modules);
+      }
+      $class = get_parent_class($class);
+    }

If what we actually want here is a class name, from $class, then we should probably do $class = get_class($this); instead, to be more precise.

Static anything tends to make me queasy, but in this case we're already using a static getInfo() method as a declaration mechanism so it's no worse than that, and the advantages sun lists are valid. For completeness I would rather a static getModules() method to a public property, but the concept I'm OK with.

Failing that, option 2 from the OP would be my next choice. (You want an array, pass an array, fatal if not.)

-27 days to next Drupal core point release.

I don't get the hidden-feature comment. It's good OO principle to default to an accessor unless there's a solid reason not to; in this case, I don't see a compelling argument to make it a public property rather than a method.

A method would also allow us to skip the if(property_exists()) check, and just call getModules() in every case. The base class implementation returns an empty array, so nothing happens. Child classes can then override it to return something as needed. Also, that means the method can be defined and documented as a method, and included in an interface, whereas a member variable cannot.

If I'm not mistaken, because we traverse the class tree like that, calling $modules += parent::getModules() from your getModules() implementation would add the modules twice. Now, I imagine that wouldn't be *terrible*, but with a simple property, you really can't do anything wrong. Of course we could document not to call the parent implementation manually, but, again, what's the point?

Eh? How would it add elements twice?

+++ b/core/modules/simpletest/drupal_web_test_case.php
@@ -1401,12 +1401,19 @@ class DrupalWebTestCase extends DrupalTestCase {
+    $class = get_class($this);
+    while ($class) {
+      if (property_exists($class, 'modules')) {
+        $modules = array_merge($modules, $class::$modules);
+      }
+      $class = get_parent_class($class);
+    }

This turns into...

$class = get_class($this);
while ($class) {
  $modules = array_merge($modules, $class::getModules());
  $class = get_parent_class($class);
}

The only reason that would return duplicates is if you call parent::getModules() in your getModules() method, which is by no means a requirement. We'd just document "well don't do that."

Or, now that you mention it, that would be even better. Replace that entire code block with:

$modules = static::getModules();

(static being the late-static-binding version of self, introduced in PHP 5.3, that makes static methods work intelligently when doing inheritance.)

And the if you specify your own getModules() method, you DO call parent::getModules() yourself:

public static function getModules() {
  return array_merge(parent::getModules(), array('views'));
}

OO working as it should. :-) (I haven't tried running the above code, but give or take a syntax error it should all work, and work the way the language wants to work.)

Yeah, that's kind of what I said...
Anyhow, I guess we have to decide if not having to call parent::getModules() is worth the traversal in DrupalWebTestCase or not. I personally would say yes, but either way is better than now, I guess.

sun, let's back up a moment. Isn't there ongoing discussion of switching out Simpletest in favor of PHPUnit anyway? Shouldn't we resolve that question first before we spend time on heavy architectural changes of our custom testing framework?

If we're investing that much time, I think it would be better spent in migrating to a more standard and capable system than continuing to evolve our own, knowing that there's a good chance we'll replace it.

Currently Upal extends PHPUnit classes so that DrupalWebTestCase can be used as is - afaik the only thing that's incompatible with current tests at the moment is that PHPUnit doesn't return from assertions, and we have a few core tests that rely on that behaviour. So at least in the first instance, even with a switch to PHPUnit, all of this stuff likely wouldn't change that much.

catch: Yes, but isn't the intent to then start removing our extra intermediary layers where what's underneath is good enough? Ideally we would be able to just use vanilla PHPUnit directly in time. (At least that's my ideal.)

I'm going to be bold and mark this RTBC.
If we decide to change this to a getModules() or whatever later on, this patch will actually make that easier (because it separates the modules from the rest of the code). More importantly, this allows to remove the hackish func_get_args() dance from the dozen or so helper test classes in core (see the op).

Since we're only at #20 comments, I think it's OK to handle the conversion to the static $modules property in this issue, but in a follow-up patch.

catch: Yes, but isn't the intent to then start removing our extra intermediary layers where what's underneath is good enough? Ideally we would be able to just use vanilla PHPUnit directly in time. (At least that's my ideal.)

Well we might get there eventually, but that's got absolutely nothing to do with switching to PHPUnit.

For example to remove DrupalWebTestCase we'd need Drupal to be at the point where you never actually install it - rather you'd simply start with a blank slate (apart from config directory and database connection), then start enabling modules from there. That's a very long way off (optional system and user modules will not be easy), so it's very likely improvements to DrupalWebTestCase will last at least a full release cycle.

Today was my 5th time explaining simpletest to those new to core at a sprint, and far and away the biggest WTF was how setUp() was used to enable modules.

I think this is great.

Thanks. Committed/pushed to 8.x, this will need a change notification.

In addition to writing the change notice, do we want to repurpose this as a meta for tracking conversions, or should that be a new issue?
I opened #1710300: Convert node tests to use ::$modules property instead of parent::setUp($modules) just to see how easy/hard it was.

grep -nr "parent::setUp([^)]" * | wc -l shows 279 places this will need changing, node module was 19 tests worth. That's about a 250K patch if it's done all at once.

I've added a change record: http://drupal.org/node/1710766

I added #1711070: Convert tests to use ::$modules property instead of parent::setUp($modules) as the followup meta.