Problem/Motivation

When the History module is enabled without the Comment module, Drupal 11.3.0-rc1 throws a ServiceNotFoundException for comment.manager during token processing, resulting in a site error. This appears to be an unconditional dependency on the comment.manager service within History’s token hooks, which should be guarded or made optional when Comment is not installed.

Steps to reproduce

  1. Install Drupal 11.3.0-rc1.
  2. Enable the History module.
  3. Ensure the Comment module is NOT enabled.
  4. Visit a node page or any route that triggers token generation (e.g., with Metatag enabled).
  5. Observe the error.
    6. The website encountered an unexpected error. Try again later.

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "comment.manager". Did you mean one of these: "theme.manager", "config.manager"? in Drupal\Component\DependencyInjection\Container->get() (line 157 of core/lib/Drupal/Component/DependencyInjection/Container.php).
Drupal::service() (Line: 30)
Drupal\history\Hook\HistoryTokensHooks->tokenInfo()
call_user_func_array() (Line: 389)
Drupal\Core\Extension\ModuleHandler->Drupal\Core\Extension\{closure}() (Line: 340)
Drupal\Core\Extension\ModuleHandler->invokeAllWith() (Line: 388)
Drupal\Core\Extension\ModuleHandler->invokeAll() (Line: 40)
Drupal\token\Token->getInfo() (Line: 100)
Drupal\token\Token->getTokenInfo() (Line: 57)
Drupal\token\TokenModuleProvider->resolveCacheMiss() (Line: 149)
Drupal\Core\Cache\CacheCollector->get() (Line: 49)
Drupal\token\TokenModuleProvider->getTokenModule() (Line: 1058)
token_tokens()
call_user_func_array() (Line: 389)
Drupal\Core\Extension\ModuleHandler->Drupal\Core\Extension\{closure}() (Line: 340)
Drupal\Core\Extension\ModuleHandler->invokeAllWith() (Line: 388)
Drupal\Core\Extension\ModuleHandler->invokeAll() (Line: 364)
Drupal\Core\Utility\Token->generate() (Line: 245)
Drupal\Core\Utility\Token->doReplace() (Line: 195)
Drupal\Core\Utility\Token->replace() (Line: 66)
Drupal\metatag\MetatagToken->replace() (Line: 791)
Drupal\metatag\MetatagManager->processTagValue() (Line: 634)
Drupal\metatag\MetatagManager->generateRawElements() (Line: 573)
Drupal\metatag\MetatagManager->generateElements() (Line: 510)
metatag_get_tags_from_route() (Line: 264)
_metatag_remove_duplicate_entity_tags() (Line: 224)
metatag_entity_view_alter() (Line: 460)
Drupal\Core\Extension\ModuleHandler->alter() (Line: 306)
Drupal\Core\Entity\EntityViewBuilder->buildMultiple() (Line: 240)
Drupal\Core\Entity\EntityViewBuilder->build()
call_user_func_array() (Line: 107)
Drupal\Core\Render\Renderer->doTrustedCallback() (Line: 910)
Drupal\Core\Render\Renderer->doCallback() (Line: 441)
Drupal\Core\Render\Renderer->doRender() (Line: 230)
Drupal\Core\Render\Renderer->render() (Line: 242)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 634)
Drupal\Core\Render\Renderer::Drupal\Core\Render\{closure}()
Fiber->start() (Line: 635)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 235)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare() (Line: 131)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse() (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray() (Line: 246)
Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}() (Line: 206)
Symfony\Component\EventDispatcher\EventDispatcher->callListeners() (Line: 56)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch() (Line: 188)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 118)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 92)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 54)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 745)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Proposed resolution

Update the code in HistoryTokensHooks.php to check for the service's existence before using it. This prevents the exception when the comment module is disabled.

User interface changes

N/A

Release notes snippet

History module triggers ServiceNotFoundException for "comment.manager" when Comment module is not enabled

Issue fork drupal-3562753

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

phoang created an issue. See original summary.

phoang’s picture

phoang commented
Assigned: Unassigned » phoang

phoang opened merge request !14094

phoang’s picture

phoang commented
Status: Active » Needs review
phoang’s picture

phoang commented
Assigned: phoang » Unassigned
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thank you for reporting. So issues need to land in 11.x first as the development branch.

May be good to also get a test or see about expanding an existing one within history

phoang opened merge request !14097

phoang closed merge request !14094

phoang’s picture

phoang commented
Status: Needs work » Needs review
quietone’s picture

quietone commented
Version: 11.3.x-dev » 11.x-dev
Priority: Normal » Major
Issue tags: +11.3.0 release priority

@phoang, thanks for reporting this.

This will be fixed on 11.x and then backported, so changing version.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Probably need some simple test coverage behind this

smustgrave’s picture

smustgrave commented
Issue tags: +Needs tests

phoang closed merge request !14097

phoang opened merge request !14100

phoang’s picture

phoang commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Needs work

Thank you for your work on this issue, @phoang. I reviewed your changes and have a concern about the test. I notice that you created a Functional test, which has a high processing cost. The thing being tested is a public function on a class. It seems possible to test this with a Unit test instead, particularly since we don't care about the output. We only care that the condition doesn't cause an error.

I also left a separate comment on the MR.

mingsong made their first commit to this issue’s fork.

mingsong’s picture

mingsong
Location 🇦🇺
commented

I made the following changes as required in #16:

  1. A new unit test instead of the functional test.
  2. Move the service call outside of the iteration loop.
mingsong’s picture

mingsong
Location 🇦🇺
commented
Status: Needs work » Needs review

All tests passed.

Ready for review.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I left some additional feedback on the MR. Thank you for putting in the effort to make a new test, @mingsong.

cilefen’s picture

cilefen commented
Priority: Major » Critical
phoang’s picture

phoang commented
Status: Needs work » Needs review
dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

My feedback was addressed.

dww’s picture

dww
we/he/they
commented
Issue tags: +Bug Smash Initiative

Mostly looks great, thanks! Tagging to be smashed.

Left a few nit suggestions on the MR, but it's RTBC either way.

I ran the test-only job, and it failed as expected:
https://git.drupalcode.org/issue/drupal-3562753/-/jobs/7699055

There was 1 error:
1) Drupal\Tests\history\Unit\HistoryTokensHooksTest::testTokenInfoWithoutCommentModuleInstalled
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "comment.manager".

The actual fix is small, clean, and expected. The Unit test looks good, and works as advertised.

Thanks again!
-Derek

dww’s picture

dww
we/he/they
commented

My suggestions have all been applied. I see nothing else to improve. Pipeline is still all green. Still RTBC.

Thanks!
-Derek

mlncn’s picture

mlncn commented

There is no way to mark something super-critical, i suppose, and it is already on the release page's known issues, but it was a major oversight to release 11.3 without bringing in the fix for a critical (WSOD-level) bug introduced since the last stable release. It was tagged as a release priority and everything! Will there be an extra release tomorrow?

But hey, it is the first bug all (very long) day that is not my fault, and testing caught it before production, so, i'll take the easy fix.

  • catch committed c6954e6e on 11.3.x 
    fix: #3562753 History module triggers ServiceNotFoundException for...

  • catch committed dbb19e2a on 11.x 
    fix: #3562753 History module triggers ServiceNotFoundException for...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.3.x-dev
Status: Reviewed & tested by the community » Fixed

@mlncn this was reported the day after the original release window for Drupal 11.3.0, we decided to delay the release one week for other, unrelated reasons but otherwise there's no way the release could have been held on it.

I think if someone had pinged the release managers in slack to ask us to hold the release on this issue, we could have done that and tried to help get it fixed faster - for example I would have committed a hotfix without test coverage if this was RTBC on Monday. But I don't think that happened. Unfortunately we have a lot of critical bugs in the issue queue, and there's also dozens of issues tagged 11.3.0 release priority that will become 11.4.0 release priorities - the tag isn't a guarantee that something will get in.

On the MR I would slightly prefer a moduleExists('comment') check over the service check, but this is a prime case for #3524377: Allow to skip OOP hooks and services for modules that are not installed so we could refactor it to rely on that when it's in for 11.4. Will link this issue from there.

Committed/pushed to 11.x and cherry-picked to 11.3.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.

dcam’s picture

dcam commented
Related issues: +#3563898: Status report page and Configuration page on error when comment is disabled

Status: Fixed » Closed (fixed)

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