Problem/Motivation

There are a few test modules with .module files to be converted.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3625015

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

nicxvan created an issue. See original summary.

nicxvan’s picture

Status: Active » Needs review
amitgoyal’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed MR !17215 for RTBC.

Changes:
- Removed deprecation_test.module and the deprecation_hook_attribute_test module: deprecation_test_function() moved to DeprecatedController::testDeprecation(), and the deprecated hook/alter implementations from deprecation_hook_attribute_test merged into deprecation_test's DeprecationTestHooks class. Updated expected deprecation messages accordingly in ModuleHandlerDeprecatedHookTest and BrowserTestBaseTest.
- Removed system_test.module: _system_test_first_shutdown_function() and _system_test_second_shutdown_function() converted to static methods SystemTestController::firstShutdown() / ::secondShutdown(), with drupal_register_shutdown_function() calls updated to the new callables.
- Removed the two updated_module.module fixture files under package_manager's build test projects (1.0.0 and 1.1.0). No functional replacement needed here since the only test that exercises this fixture, PackageUpdateTest::testPackageUpdate, is currently skipped pending #3508109.
- Updated .phpstan-baseline.php entry to match the new deprecated method's identifier (staticMethod.deprecated instead of function.deprecated) and message text.

Verification:
- CI pipeline #972049 passed with warnings -- the only failed jobs are PHPUnit Unit (Core/Component): [8.6-ubuntu], which is the intentionally allowed-to-fail "next PHP major" lane (exit_codes: 100 in .gitlab-ci.yml); failures there are pre-existing/unrelated to this diff.
- No open unresolved threads on the MR.
- Checked out the branch locally and ran the affected test classes in DDEV (PHP 8.5):
- ModuleHandlerDeprecatedHookTest -- 3/3 pass
- ShutdownFunctionsTest -- 1/1 pass
- BrowserTestBaseTest::testDeprecationTriggeredInSystemUnderTest -- 1/1 pass
- Ran PHPCS (Drupal, DrupalPractice) on all changed files. SystemTestController.php and BrowserTestBaseTest.php show pre-existing violations (camelCase method names, \Drupal:: calls, missing docblocks, unserialize() warning) -- confirmed these are present on unmodified main too, so not introduced by this change.
- No public API change, so no Change Record needed.

Moving to RTBC.

berdir’s picture

It is very rarely necessary to run tests manually to verify them. We have CI for that.

What is required, beside code review, is to verify that nothing is missing.

I did this to see all test .module files:

find core -name "*.module" | grep "tests"
core/modules/system/tests/fixtures/HtaccessTest/access_test.module
core/modules/system/tests/modules/module_autoload_test/module_autoload_test.module
core/modules/system/tests/modules/legacy_hook_test/legacy_hook_test.module
core/modules/system/tests/modules/hook_collector_on_behalf_procedural/hook_collector_on_behalf_procedural.module
core/modules/system/tests/modules/hook_collector_skip_procedural/hook_collector_skip_procedural.module
core/modules/system/tests/modules/hook_collector_skip_procedural_attribute/hook_collector_skip_procedural_attribute.module
core/modules/system/tests/modules/HookOrder/aaa_hook_order_test/aaa_hook_order_test.module
core/modules/system/tests/modules/HookOrder/bbb_hook_order_test/bbb_hook_order_test.module
core/modules/system/tests/modules/HookOrder/ccc_hook_order_test/ccc_hook_order_test.module
core/modules/system/tests/modules/HookOrder/ddd_hook_order_test/ddd_hook_order_test.module
core/modules/system/tests/modules/container_initialize/container_initialize.module
core/modules/system/tests/modules/module_test_procedural_preprocess/module_test_procedural_preprocess.module
core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test/module_handler_test.module
core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test_added/module_handler_test_added.module
core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test_all1/module_handler_test_all1.module
core/tests/Drupal/Tests/Core/Extension/modules/module_handler_test_all2/module_handler_test_all2.module
core/tests/fixtures/empty_file.php.module

Many are for legacy hook tests, access test is just to check if the file can be accessed directly, we want to keep this part, even after we remove support for .module files because we want to keep protection for them as modules will continue to ship those files for BC even on D13.

module_handler_test is a bit special, but we want to keep that for \Drupal\Tests\Core\Extension\ModuleHandlerTest::testLoadModule(). it's not actually a hook, it just wants to verify that the file and the function in it was loaded. The _added variant is for the same test.

The last one I wasn't sure is all1/2. These are used by two unit tests. One is \Drupal\Tests\Core\Extension\ModuleHandlerTest::testLoadAllModules, which is loadAll(), so still needed and again, for that one, not really hooks, just to make sure the functions exist. But they are also used by \Drupal\KernelTests\Core\Hook\HookCollectorPassTest::testOrdering, there they are used as hooks and used to test the order behavior of oop and legacy hooks. I guess. Looking at that, I'm not entirely sure what should become of that in D13. without legacy hooks, the test will be meaningless like that. But that's a problem for D13 to solve and OK to keep for now.

So, +1 to RTBC.

nicxvan’s picture

  • catch committed 37ee1353 on main
    task: #3625015 Convert final test .modules
    
    By: nicxvan
    By: berdir
    

  • catch committed b82b04c6 on 11.x
    task: #3625015 Convert final test .modules
    
    By: nicxvan
    By: berdir
    (...

  • catch committed fc8c9c11 on 12.0.x
    task: #3625015 Convert final test .modules
    
    By: nicxvan
    By: berdir
    (...
catch’s picture

Version: main » 11.5.x-dev
Status: Reviewed & tested by the community » Fixed

@amitgoyal I feel the need to ask, #4 includes a lot of detail that is not relevant to an RTBC here, for example we would not normally mention that CI passes because if it didn't, the issue should not even be at needs review (except for rare cases with a disclaimer). Similarly, as @berdir mentions checking out the branch locally and running tests is not necessary because we have CI to do that, but it is something that Claude might do if pointed to an issue.

If you're using an LLM to review issues, please note the LLM usage and disclosure policy on Drupal.org https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett...

Committed/pushed to main, 12.0.x and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.