ExtensionDiscovery returns testing profiles even when you ask it not to

The problem appears to be that the decision about whether something is a "test" extension depends on it being under a directory called "tests". But these aren't. They're just marked "hidden" in their info files. This filtering is done in
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

I actually also ran into problems scanning for modules and themes, in that modules/themes underneath these testing profiles were returned, although they are definitely test modules/themes and some of them cannot even be installed. I got around this in the test I was writing by trying to say I didn't want any modules/themes underneath profiles at all, and I ran into #3076098: ExtensionDiscovery documentation is wrong about scanning profile directories, which I was able to work around by telling ExtensionDiscovery that I only wanted modules/themes under the nonexistent 'foo' profile.

But then I ran into this issue, so I went back to my original scheme for making a list of "relevant" modules/themes/profiles on #3066512: Add checks for syntax and display of help topic Twig template files and just used strpos like this, after getting the extension's path:

      if ((strpos($path, '/tests') === FALSE) &&
        (strpos($path, '/testing') === FALSE)) {

Anyway. It seems like excluding profiles as being "testing" based on them being underneath a "tests" directory is not very useful.

OK.

So... I guess the question is where to put the tests subfolder. Options I can think of:

a) core/profiles/tests -- people could find them easily if they knew where they were previously. But it's not all that consistent with where we put test modules/themes.

b) core/tests/profiles -- that would be a central location. However, there is no core/tests/modules or core/tests/themes directory.

c) core/[something]/tests/profiles -- might make sense for profiles that are only used by tests in a core/[something]/tests directory, like we do for test modules and themes.

So... Here is patch that is a combination of (a) and (c). Profiles that are for particular modules' tests I put into core/thatmodule/tests/profiles [that only applied to two that were for core/modules/config], and the others I moved into core/profiles/tests so they would still be easy to find.

Assuming the tests still pass (let's see what the bot says), let the bikeshedding begin!

Well, I think that the first fail actually only indicates that we need to make a small change in a test that had the path to the profile hard-wired in it. I'll see if I can get the rest of the tests to pass. I think it may be possible. Those directories are definitely being scanned for profiles...

I managed to get all of the tests that failed to pass again. Most of them were failing due to hard-coded directories, which changed because the patch moved those profiles.

The last one was a bit different: Drupal\Tests\Core\Command\QuickStartTest

The reason that one was failing was that in both includes/install.core.inc and core/lib/Drupal/Core/Command/InstallCommand.php, there was an implicit assumption that ExtensionDiscovery::scan() would locate test install profiles (which it was doing, subject of this issue report). But now because those are under "tests" directories, that method doesn't find them unless you ask it to include tests (that's the whole point of this issue).

So I changed a few lines in those files to tell it to scan including test profiles. I'm assuming that we want the QuickStart and anything else that relies on includes/install.core.inc to work if someone actually wants to install with the "testing" profile.

Anyway... let's see what happens this time...

So, this will change the behavior of ExtensionDiscovery::scan('profile') if you don't pass in a 2nd argument.

Previously, it would always find test install profiles (the subject of this bug). But now, if you don't pass in a 2nd argument, it will try to detect whether you wanted tests or not, and respect that choice. So when running within a test, it will find test profiles, but when running outside a test, unless you have that setting in your settings.php file, it won't. (Just like scanning for modules has always worked.)

So... just to make sure we aren't artificially missing a problem while running tests... I grepped the codebase to find out who might be calling scan('profile').

I found:
a) Several tests calling it, to figure out what profile you wanted to install with or things like that. That should be OK if they leave out the 2nd argument, because their behavior wouldn't change during testing.

b) The lines I already updated in this patch.

c) Other stuff:

./lib/Drupal/Core/Update/UpdateRegistry.php:    $profile_extensions = $extension_discovery->scan('profile');
./lib/Drupal/Core/Config/ExtensionInstallStorage.php:        $profile_list = $listing->scan('profile');
./lib/Drupal/Core/Config/ExtensionInstallStorage.php:            $profile_list = $listing->scan('profile');
./lib/Drupal/Core/Config/InstallStorage.php:        $profile_list = $listing->scan('profile');
./lib/Drupal/Core/Extension/ModuleExtensionList.php:    $all_profiles = $discovery->scan('profile');
./lib/Drupal/Core/DrupalKernel.php:      $all_profiles = $listing->scan('profile');

These probably need to be looked at. The behavior while running inside tests hasn't changed from before, but behavior when running outside of a test environment of these functions will change. I'll take a look tomorrow...

I took a look at the other existing calls to ExtensionDiscovery::scan('profile') in Core (see comment #10, list in (c)).

UpdateRegistry:

    // Scan the module list.
    $extension_discovery = new ExtensionDiscovery($this->root, FALSE, [], $this->sitePath);
    $module_extensions = $extension_discovery->scan('module');

    $profile_extensions = $extension_discovery->scan('profile');

Since this code is also scanning modules using the default argument (ignore testing modules, unless the thing in settings.php is set or you're running a test), I think it's OK if it also ignores testing profiles for real.

ExtensionInstallStorage, InstallStorage:
What's going on there is similar. It's scanning modules, themes, and profiles in the same way, with the default argument.

ModuleExtensionList:
This should be fine too, I think... It's in ::getProfileDirectories(), which in turn is used to set the profile directory that can be scanned for modules. This class is used to make lists of modules, so it makes sense that if you want testing modules, you also want testing profiles, and if you don't, you don't. (Again, being consistent between the scan behavior of modules and profiles makes sense.)

DrupalKernel:
This is a copy/paste of the code in ModuleExtensionList, essentially. So again, being consistent between modules and profiles makes sense.

So. I think we're OK to go ahead and move the test profiles to "tests" directory, and have them only returned from ExtensionDiscovery under the same circumstances as we would return test modules and themes.

Anyone want to give this patch a review?

I didn't mean for this to be a child issue.

Updating issue summary. Also made a draft change notice
https://www.drupal.org/node/3076852

I think this is going to need to be reviewed by the Framework managers, even though the patch is @larowlan's suggestion...

fix typo in issue summary