Problem/Motivation

If override is enabled for "Simplenews" but not for "Simplenews newsletter" then emails for "Simplenews newsletter" will be routed instead to "Simplenews". The email fails to send with this error message:

Error: Call to a member function getMail() on null in Drupal\symfony_mailer\Plugin\EmailBuilder\SimplenewsEmailBuilderBase->build() (line 25 of modules/contrib/symfony_mailer/src/Plugin/EmailBuilder/SimplenewsEmailBuilderBase.php).

A similar problem occurs if override is enabled for "Personal contact form" but not for "Contact form".

Steps to reproduce

See above

Proposed resolution

The email builder id is currently simplenews which picks up all simplenews emails. The only solution I can see is to change the email builder IDs:

  • simplenews => simplenews_subscriber
  • contact => contact_user

Unfortunately this is non-BC. The update hook renames existing mailer policies. However sites needs changes to templates and email alter hooks.

Remaining tasks

User interface changes

API changes

Change to the email builder IDs in email alter hooks.

Data model changes

Change to the email builder IDs in mailer policy.

Comments

AdamPS created an issue. See original summary.

adamps’s picture

Status: Active » Needs review
StatusFileSize
new8.86 KB
adamps’s picture

adamps’s picture

Issue summary: View changes
adamps’s picture

This can't be fixed until 2.x. Created #3367314: Warn about unsupported override combinations to warn about it in the meantime.

huzooka’s picture

We just run into this issue with 1.3.0 (issue still seems to be present in 1.x too).
We have custom module named contact_form. If we try to send an email on behalf of this module, Drupal Symfony Mailer won't find a matching email builder (nor a dynamically created instance using the LegacyEmailBuilder class).

How we worked around this temporarily

We prefixed the mail module param with an extra string, and we added an email builder plugin with the mathcing ID.

Analysis

This happens because we won't have any builder plugin definition for this module. Why? Let's see what happens during the discovery phase under the hood:

  1. The discovery specified in DefaultPluginManager discover all the available email builder plugins.
  2. EmailBuilderManager::processDefinition() processes the definitions. In case of the ContactPageEmailBuilder plugin, the plugin ID contact_form also must contain the config entity type ID associated with the email builder plugin (the has_entity annotation is set to TRUE), ::processDefinition checks whether there is a entity type with ID contact_form available or not. It isn't (we don't have contact module being enabled), so the provider in the definition is set to _.
  3. symfony_mailer_mailer_builder_info_alter start to alter these definitions.
  4. Our module's hook_mail() implementation gets discovered, but since Drupal Symfony Mailer already defines a plugin with ID contact_form, the logic inside the foreach loop won't generate builder plugin instance for our module - because there is already a builder plugin with that ID!
  5. Since the contact module (what contact_form builder plugin provider is '_', it gets removed in DefaultPluginManager::findDefinitions().
  6. So when we try to send an email through that module with MailManagerReplacement::mail(), EmailBuilderManagerInterface->createInstanceFromMessage() wont't find any matching email builder plugin, so it doesn't return anything (which is actually equal to returning a NULL in this case) ➡️ we get the same error message as in the IS (but for a bit different reason).

Suggestion for a BC-safe fix

Separate module name / builder plugin ID / ID of the related entity type:

  • Instead of assuming that plugin ID === module name, utilize the provider annotation property: the provider could be added to the plugin annotations: provider => "contact". Any module can define plugin on behalf of other modules! This could replace the problematic parts in point 2.

    If this turns to be trickier than ideal, introduce a 'provider_module' or 'module' annotation instead, but I think that using provider is enough.

  • Instead of assuming that the related entity type is the same as the plugin ID (as I see this happens if has_entity is set to TRUE), add a 'content_entity_type_id' (or something similar) annotation property (and maybe deprecate 'has_entity'). BTW, does this module use this info for anything else than calculating the provider? (Didn't checked yet, just thinking loudly.)

  • Change logic behind EmailBuilderManager::createInstanceFromMessage()::
    • Instead of assuming that plugin IDs are matching $module or $module.$key pattern, find the corresponding plugin based on the provider annotation of the builder plugin (and also checking the sub_types if any). But! The current approach could be kept as fallback (so we can still use plugin IDs like mymodule.my_email_key, but it won't be a must.) AND
    • Make EmailBuilderManager using FallbackPluginManagerInterface, and convert LegacyMailBuilder into a real (annotated) plugin - which then can be used as the fallback builder plugin (e.g. this is what the Block(Plugin)Manager does in Drupal core). This can eliminate the requirement on the logic implemented in symfony_mailer_mailer_builder_info_alter.

@AdamPS, what do you think?

adamps’s picture

@huzooka interesting, thanks for the report. I agree that the problem you see has the same symptoms as this issue. However it's a separate bug as it has different steps to reproduce, and will have a different fix. Therefore I raised a new issue #3379529: Disabled overrides should not cause conflicts - please continue the discussion there.

LegacyEmailBuilder doesn't have @EmailBuilder annotation because we want to create multiple instances of it with different id, label and provider - see symfony_mailer_mailer_builder_info_alter(). These instances are used by the mailer policy form and perhaps other places.

adamps’s picture

@huzooka Please can you test the patch on #3379529: Disabled overrides should not cause conflicts?

adamps’s picture

adamps’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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