Problem/Motivation

A module that implements hook_mail() but provides no EmailBuilder is registered with an auto-generated "legacy" definition whose only sub-type is the wildcard *, labelled "Unknown" (mailer_override, OverrideManager::getLegacyMailerDefinition()). Because of that single wildcard sub-type, it is impossible to create a Mailer Policy scoped to such a module through the Add policy UI: the save always throws a ConfigNameException.

Steps to reproduce

  1. Enable any module that implements hook_mail() and has no EmailBuilder.
  2. Go to /admin/config/system/mailer/policy/add.
  3. Tag 1: select that module.
  4. Tag 2: the only option offered is "Unknown" (no "All" empty option appears).
  5. Click "Add and configure".

Result

The save fails with:

Drupal\Core\Config\ConfigNameException: Invalid character in Config object name mailer_policy.mailer_policy.MODULENAME.* in Drupal\Core\Config\ConfigBase::validateName()

MODULENAME is the module machine name; the final token is the literal * wildcard.

Root cause

  • A legacy module is registered with a single sub-type whose id is * (OverrideManager, line 448: sub_defs: ['*' => $this->t('Unknown')]).
  • In PolicyAddForm::form() the empty "All" option is added to Tag 2 only when there is more than one sub-type (if (count($options2) > 1), around line 86). With a single sub-type the user is forced to select the wildcard.
  • PolicyAddForm::validateForm() then builds the id with implode('.', array_filter($id_array)) (around line 150). array_filter() keeps the truthy *, producing MODULENAME.*.
  • * is not a legal character in a config object name, so the save throws.

Proposed resolution

Treat the wildcard sub-type as "all sub-types" when building the policy id, so a legacy module maps to a clean type-level id (just the module name) rather than MODULENAME.*. The attached merge request changes the id builder in validateForm() to drop empty values and the * wildcard before imploding.

Versions

Reproduced on 2.0.1 and confirmed unchanged on the 2.x branch (HEAD): the same three lines are present.

Issue fork symfony_mailer-3593926

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

mably created an issue. See original summary.

mably opened merge request !208

mably’s picture

mably commented
Status: Active » Needs review
adamps’s picture

adamps commented
Status: Needs review » Needs work

Thanks. I feel that instead of writing code to deal with the problematic sub-def with a *, we can just avoid adding it in the first place.

Idea 1 (not working)

The most obvious way to do this is in in OverrideManager::getLegacyMailerDefinition(). Having written this description, I eventually realised it is isn't the right fix. I've left the explanation in so that you or anyone else can see why it doesn't work. Change the first line to:

$mailer = new MailerInfo($id);

The add form then allows you to optionally type in a "tag 2" if you happen to know the available values, or leave it blank for all. This comes from the code below which first of all creates the field as a textfield, which is changed later to a select if we know the options.

      // Second part of tag.
      $form['tag2'] = [
        '#title' => $this->t('Tag 2'),
        '#description' => $this->t("Second part of the email tag that the policy applies to."),
        '#type' => 'textfield',
      ];

So the correct code is basically all there, it was just obscured by that one bad line.

This does create a minor new problem. If I do manually type a sub-tag, then I can create a policy as expected. However it displays with tag as "Unknown". It should be "Module name >> Unknown". The is because the code in MailerLookup::getTagDefinition() is specifically looking for the subdef ending with a * that we no longer create. This presumably explains how I introduced the main bug in the first place - whilst trying to get the code working for this new problem. We could try to adjust this function, but I don't see a clear simple alternative so perhaps we could instead try a new idea...

Idea 2

In PolicyAddForm::form() around line 75, add an if, with comment

// Ignore a wildcard indicating unknown sub_defs.
if ($id != '*') {
  $options2[$id] = $sub_def['label'];
}

This seems to work.

Could you check it works for you, and if so update the MR?

mably’s picture

mably commented
Status: Needs work » Needs review

Thanks, that's cleaner. I've gone with idea 2.

In PolicyAddForm::form() the wildcard sub-type is now skipped when building the Tag 2 options:

foreach ($definition['sub_defs'] as $id => $sub_def) {
  if ($id !== '*') {
    $options2[$id] = $sub_def['label'];
  }
}

For a legacy hook_mail() module whose only sub-def is "*", $options2 is then empty, so the select overlay is skipped and Tag 2 stays the free text field: leaving it blank means "all" and produces a valid config name. The "*" sub-def is left untouched in the definition, so MailerLookup still resolves a manually typed unknown value to "Module name >> Unknown" rather than just "Unknown".

Since "*" can no longer reach validation, the previous validateForm guard became redundant, so I reverted that method to its upstream version. It's a single additive commit on top of the branch.

Tested locally: adding a policy for a legacy hook_mail() module now shows the free text Tag 2 field, and saving it blank creates the policy without the ConfigNameException.

adamps’s picture

adamps commented
Status: Needs review » Fixed

Thanks that was quick! I've set it to merge.

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.

  • adamps committed db62a673 on 2.x authored by mably 
    Issue #3593926: Fix invalid config name when adding a policy for a...

Status: Fixed » Closed (fixed)

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