1. Since #2188661: Extension System, Part II: ExtensionDiscovery, test extensions are no longer discovered at runtime.

  2. #2198713: Add a settings.php variable to allow to discover/enable test extensions at regular runtime adds a settings.php setting to allow developers to discover test extensions at regular runtime (for debugging).

  3. → We can safely remove the hidden: true property from all test extension .info.yml files, which in turn allows developers to simply enable a test module on the regular Modules page when the setting is enabled.

Comments

sun’s picture

Issue summary: View changes
tstoeckler’s picture

Hmm... if we want to do this, I'm wondering whether we shouldn't expose the setting introduced in #2198713: Add a settings.php variable to allow to discover/enable test extensions at regular runtime in the UI, i.e. turn it into config instead?! I would personally find that very convenient, but I don't know how others feel.

In any case, +1.

tstoeckler’s picture

Status: Active » Needs review
StatusFileSize
new69.39 KB

Here we go. The only hidden:true that remains in core is testing.info.yml from Testing profile. I verified that indeed no test modules are shown on admin/modules.

Status: Needs review » Needs work

The last submitted patch, 3: 2209337-3-remove-hidden.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
StatusFileSize
new687 bytes
new68.72 KB

The locale_test_translation module needs to be hidden for the test to work.

Status: Needs review » Needs work

The last submitted patch, 5: 2209337-5-remove-hidden.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
StatusFileSize
new587 bytes
new0 bytes

locale_test itself needs to be hidden, as well. That's a bit weird, but hey...

tstoeckler’s picture

StatusFileSize
new68.15 KB

Rolling patches is hard... :-) (Interdiff is correct, though)

tstoeckler’s picture

StatusFileSize
new3.24 KB
new70.97 KB

This needed a re-roll. Anyone want to RTBC?
The interdiff is for all the test modules that have been added in the meantime.

Status: Needs review » Needs work

The last submitted patch, 9: 2209337-9-remove-hidden.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
StatusFileSize
new1.24 KB
new69.73 KB

Oops I inadvertantly reverted the interdiffs from 5 and 7. This should be green again.

sun’s picture

FWIW:

This would allow us to make InstallerTestBase use the Testing profile by default instead of Minimal profile, which would speed up the core test suite.

InstallerTestBase tests the interactive installer. Because the Testing profile is hidden, it cannot be selected in the UI. Therefore, it has to select Minimal profile.

tstoeckler’s picture

Re @sun: Is that an RTBC? :-)

sun’s picture

11: 2209337-11-remove-hidden.patch queued for re-testing.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2209337-11-remove-hidden.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
StatusFileSize
new1.24 KB
new70.97 KB

Hmm... that one passes locally.

Here's a re-roll with two more modules that have been introduced in the meantime.

tstoeckler’s picture

Priority: Minor » Normal

Per #12 marking normal.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

RTBC per #15.

alexpott’s picture

Just pondering if anyone has use hidden for anything other than test modules?

berdir’s picture

Yep, I do, but as I understand it we're not removing the ability to use hidden but just remove it from test modules? I think we should keep the ability to use hidden.

One example use case is that we started providing demo/set-up modules for simplytest.me for contrib modules, which provides support for enabling modules automatically. We don't want those to be visible because they mess with your site configuration and you shouldn't enable them on a normal site.

alexpott’s picture

So now I'm wondering if we need to add a test :)

sun’s picture

There is indeed no explicit test coverage for the (purely UI-facing) 'hidden' property in System module's tests yet.

Note: The property really has effects on the Extend/Modules page only. The extension system, module system, ModuleHandler doesn't care.

However, that's off-topic/out-of-scope for this issue. Adding test coverage for the expected 'hidden' behavior sounds like a good idea to me, but this issue/patch does not touch any code that would be related to that behavior.

sun’s picture

FWIW, I was mistaken in #12 — since this patch retains the hidden property for the Testing profile, the interactive installer tests still won't be able to use it (because they cannot select it in the UI). But we can address that differently.

Regardless of that, I still think this change makes sense and simplifies things. There's no need for testing modules to be hidden, if they're not discovered in the first place. And when the system is told to discover them, it would be useful if they'd actually appear in the UI, too.

Double-DX-win in my book.

sun’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit bf3ba52 on 8.x by catch:
    Issue #2209337 by tstoeckler: Remove hidden property from test...

Status: Fixed » Closed (fixed)

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