Problem/Motivation

The usages of forum need to be removed so the module can be deprecated.

Steps to reproduce

$ grep --exclude-dir={fixtures,forum,node_modules,vendor,themes} -ri forum core  | grep -vi migrat |  grep Test | awk -F: '{print $1}' | sort -u | nl
     1  core/modules/comment/tests/src/Functional/CommentFieldsTest.php
     2  core/modules/node/tests/src/Functional/NodeTypeTest.php
     3  core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php
     4  core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php

There is also a usage in core/modules/system/tests/modules/url_alter_test/url_alter_test.install

Proposed resolution

  • core/modules/comment/tests/src/Functional/CommentFieldsTest.php - remove deletion of comment_forum field
  • core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php - replace with a new test module
  • core/modules/node/tests/src/Functional/NodeTypeTest.php - this is a comment, fixed in #3409388: Remove usage of forum module from comments and strings
  • core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php - no change, An update test using forum that will be removed in 11.0

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3409385

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:

About issue forks

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Issue summary: View changes

quietone opened merge request !6101

quietone’s picture

quietone commented
Issue summary: View changes

Still need to investigate core/modules/system/tests/modules/url_alter_test/url_alter_test.install

quietone’s picture

quietone commented
Status: Active » Needs review

This function,

function url_alter_test_install() {
  // Set the weight of this module to one higher than forum.module.
  module_set_weight('url_alter_test', 2);
}

was added in #320331-69: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks. This is executed in \Drupal\Tests\path_alias\Functional\UrlAlterFunctionalTest and forum is no longer installed in the test. If the hook is deleted the test still passes. So, is this really unneeded code or is the test not doing what it should be. I don't know enough about this area of core to say.

And, because of the use of forum this test was copied to the forum module, \Drupal\Tests\forum\Functional\UrlAlterFunctionalTest with a new test module, forum_url_alter_test. In that test module the hook is absent So, perhaps this really is dead code.

catch’s picture

catch
he/him
Primary language English
commented

I did the following:

git log -S "forum" core/modules/path_alias/tests/src/Functional/UrlAlterFunctionalTest.php

git show 8a924f778d5 core/modules/path_alias/tests/src/Functional/UrlAlterFunctionalTest.php

And the answer is - we took the forum bits of the test out in a previous patch, and this bit got missed.

If the forum tests pass without this hook_install() too, I think it's fine to remove as dead code. It was added a very, very long time ago.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Tried following the best I could

I commented out url_alter_test_install. Both UrlAlterFunctionalTest (path_alias) and UrlAlterFunctionalTest (forum) still pass.

catch’s picture

catch
he/him
Primary language English
commented

Yep I think we can just remove it.

quietone’s picture

quietone commented
Status: Needs work » Needs review

I removed the file as the hook was the only thing it contained.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Know I posted in slack leaving tickets in review longer. But based on the importance of this to get Forum deprecated by D11 going to go ahead and mark.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Remove usage of forum module from tests » Remove usage of Forum module from tests
xjm’s picture

xjm
she/her
Primary language English
commented

Just documenting my own results while I review.

Before

[ayrton:drupal | Sun 10:33:07] $ grep -ri "forum" * |  grep -v "node_modules" | grep -v "vendor" | grep -i "tests" | grep -vi "migrat" | grep -v "core/modules/forum" | grep -v "fixtures"
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:      case 'field.field.node.forum.comment_forum':
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->enableModules(['field', 'node', 'comment', 'taxonomy', 'forum']);
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->assertNull(FieldConfig::load('node.forum.comment_forum'));
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->installConfig(['forum']);
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:        $this->assertNotNull(FieldConfig::load('node.forum.comment_forum'));
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:    yield 'Dynamic type with [%parent.%parent]: field.field.node.forum.comment_forum:default_value.0' => [
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php:      'field.field.node.forum.comment_forum',
core/modules/system/tests/modules/url_alter_test/url_alter_test.install:  // Set the weight of this module to one higher than forum.module.
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:  protected static $modules = ['forum'];
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:    'forum_index' => ['created', 'last_comment_timestamp'],
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:      // enabled modules: forum, language, locale, statistics and tracker.

After

[ayrton:drupal | Sun 10:35:06] $ grep -ri "forum" * |  grep -v "node_modules" | grep -v "vendor" | grep -i "tests" | grep -vi "migrat" | grep -v "core/modules/forum" | grep -v "fixtures"
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:  protected static $modules = ['forum'];
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:    'forum_index' => ['created', 'last_comment_timestamp'],
core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php:      // enabled modules: forum, language, locale, statistics and tracker.

Per the IS:

core/modules/system/tests/src/Functional/Update/Y2038TimestampUpdateTest.php - no change, An update test using forum that will be removed in 11.0

  1. Is there an issue for this? If not, we should file it parented to the D11 beta blocker meta or one of its children. (And add to related issues here.)
     

  2. This is out of scope, but: Do we have other tests of 2038 upgrade paths? I guess the idea is that people must upgrade to 10.3/10.4 first regardless, which would already require their timestamps to be updated, and so it's not needed in D11. But it seems weird to have the test of this coupled only to Forum.

I diffed the Forum and new test fixure config:

[ayrton:drupal | Sun 10:49:13] $ diff -uP core/modules/forum/config/optional/field.field.node.forum.comment_forum.yml core/modules/system/tests/modules/config_mapping_test/config/optional/field.field.node.config_mapping_test.comment_config_mapping_test.yml
--- core/modules/forum/config/optional/field.field.node.forum.comment_forum.yml2023-09-25 18:03:35
+++ core/modules/system/tests/modules/config_mapping_test/config/optional/field.field.node.config_mapping_test.comment_config_mapping_test.yml	2024-01-14 10:44:22
@@ -2,14 +2,14 @@
 status: true
 dependencies:
   config:
-    - field.storage.node.comment_forum
-    - node.type.forum
+    - field.storage.node.comment_config_mapping_test
+    - node.type.config_mapping_test
   module:
     - comment
-id: node.forum.comment_forum
-field_name: comment_forum
+id: node.config_mapping_test.comment_config_mapping_test
+field_name: comment_config_mapping_test
 entity_type: node
-bundle: forum
+bundle: config_mapping_test
 label: Comments
 description: ''
 required: true
[ayrton:drupal | Sun 10:50:43] $ diff -uP core/modules/forum/config/optional/field.storage.node.comment_forum.yml core/modules/system/tests/modules/config_mapping_test/config/optional/field.storage.node.comment_config_mapping_test.yml
--- core/modules/forum/config/optional/field.storage.node.comment_forum.yml	2023-01-16 08:10:14
+++ core/modules/system/tests/modules/config_mapping_test/config/optional/field.storage.node.comment_config_mapping_test.yml	2024-01-14 10:44:22
@@ -4,12 +4,12 @@
   module:
     - comment
     - node
-id: node.comment_forum
-field_name: comment_forum
+id: node.comment_config_mapping_test
+field_name: comment_config_mapping_test
 entity_type: node
 type: comment
 settings:
-  comment_type: comment_forum
+  comment_type: comment_config_mapping_test
 module: comment
 locked: false
 cardinality: 1
[ayrton:drupal | Sun 13:14:50] $ diff -uP core/modules/forum/config/optional/node.type.forum.yml core/modules/system/tests/modules/config_mapping_test/config/optional/node.type.config_mapping_test.yml
--- core/modules/forum/config/optional/node.type.forum.yml	2024-01-14 10:14:20
+++ core/modules/system/tests/modules/config_mapping_test/config/optional/node.type.config_mapping_test.yml	2024-01-14 10:44:22
@@ -3,10 +3,10 @@
 dependencies:
   enforced:
     module:
-      - forum
-name: 'Forum topic'
-type: forum
-description: 'A <em>forum topic</em> starts a new discussion thread within a forum.'
+      - config_mapping_test
+name: 'config_mapping_test topic'
+type: config_mapping_test
+description: 'A <em>config_mapping_test topic</em> starts a new discussion thread within a config_mapping_test.'
 help: null
 new_revision: false
 preview_mode: 1
[ayrton:drupal | Sun 13:17:26] $ diff -uP core/modules/forum/forum.info.yml core/modules/system/tests/modules/config_mapping_test/config_mapping_test.info.yml
--- core/modules/forum/forum.info.yml	2023-09-25 18:03:35
+++ core/modules/system/tests/modules/config_mapping_test/config_mapping_test.info.yml	2024-01-14 10:44:22
@@ -1,12 +1,8 @@
-name: Forum
+name: Config mapping test
 type: module
-description: 'Provides discussion forums.'
+description: 'Test configuration mapping.'
 dependencies:
   - drupal:node
-  - drupal:history
-  - drupal:taxonomy
   - drupal:comment
-  - drupal:options
 package: Core
 version: VERSION
-configure: forum.overview
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs followup

I reviewed all the uses of url_alter_test in core and agree that install hook can be removed. The fiddling with Forum's weight was added in #320331-69: Turn custom_url_rewrite_inbound and custom_url_rewrite_outbound into hooks, as in, well before the release of D7, when everything to do with URLs, routing, and module weights/dependencies was wildly different. That comment reads:

Ok this one is almost ready. Still trying to figure out what's going wrong with the wonky forum tests...

Some other quotes from that issue:

In IRC, Dave and I determined the source of the badness is that calling module_list(TRUE, TRUE) to refresh the module list from inside a reduced bootstrap level (we're still just at DRUPAL_BOOTSTRAP_PATH when drupal_path_initialize() runs) results in sheer badness. entity_get_info() is confused, we have an empty list of modules (or something), and we end up caching an empty array in {cache}.cid == 'entity_info', resulting in a completely broken site until you manually clear that cache entry. :(

So:

A) WTF, module_list()? Why can't you be refreshed twice during the bootstrap process?

B) WTF, entity_get_info()? Why are you willing to cache an empty array into the DB? ;)

For now, this patch is going to work-around it by calling module_list(FALSE, TRUE) to only get bootstrap modules, but *not* refresh the list.

Forum.module is very broken. Very upset and frustrated.

And to properly compare old vs new, make sure we are using worst-case scenario. Locale.module enabled and altering language prefixes (a non-default language path), and also using taxonomy_term_path() with forum.module.

So overall I think this is ready, but NW for the followup I mentioned in #12.1.

quietone’s picture

quietone commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs followup

@xjm, thanks for the review.

#12.1. The followup issue for that is #3414563: Add new 10.3.x database dump fixtures, without modules deprecated for removal in 11.x. It is already a related issue but it can't be a child because the plan is to do the dumps once for all the deprecated modules.

I am setting back to RTBC because I think that followup was the only item.

catch’s picture

catch
he/him
Primary language English
commented

For #12.1 there is also #3401188: [policy] Remove update hooks added until 10.3 from Drupal 11 for removing updates prior to 10.3.0, so we're covered whether updates or forum module itself get removed first.

#12.2 there's explicit test coverage for field items and 2038+ timstamps being added in #2885413: Timestamp field items are affected by 2038 bug. We've only fixed the one-off schema entries around core so far, some work is still outstanding on the 2038 bug.

  • catch committed 16a22b98 on 11.x 
    Issue #3409385 by quietone, catch, xjm, smustgrave: Remove usage of...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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

quietone closed merge request !6101

xjm’s picture

xjm
she/her
Primary language English
commented

Amending attribution.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Any idea why the config_mapping_test module has been added in the "Core" package?