As described by @tiage in https://www.drupal.org/project/token_or/issues/3519448#comment-16117712 Microsoft Office email tags are treated as tokens and replaced with an empty string / removed.

<tr>
  <td><!--[if mso | IE]><table><tr><td><![endif]-->
    <div>
      <table>
        <tbody>
          <tr>
            ...more tables...
          </tr>
        </tbody>
      </table>
    </div>
  <!--[if mso | IE]></td></tr></table><![endif]--></td>
</tr>

is ending up as:

<tr>
  <td><!--><table><tr><td><![endif]-->
    <div>
      <table>
        <tbody>
          <tr>
            ...more tables...
          </tr>
        </tbody>
      </table>
    </div>
  <!--></td></tr></table><![endif]--></td>
</tr>

Issue fork token_or-3591289

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

milanbombschliip created an issue. See original summary.

milanbombschliip opened merge request !25

pianomansam’s picture

pianomansam commented
Status: Active » Needs review

@milanbombschliip thank you for the bug report and patch. I have some thoughts.

First, does the reported issue occur when the Token module is enabled but Token OR is not enabled? In other words, is this issue specific to Token OR?

Second, I would prefer a general solution versus adding code for a specific circumstance, like Microsoft Office email tags. Approaching this issue by making an exception for MSO is not scalable. We should get to the root cause.

Finally, we will need tests in order for any patch to be considered.

mukeshaddweb’s picture

mukeshaddweb commented
Status: Needs review » Needs work

Thanks for working on this and identifying the issue.

I reviewed the current approach and the fix works for the reported case. Building on that, I think we can make it more robust by avoiding hardcoded checks for only `[if mso | IE]` and `[endif]`, since there are several other common MSO conditional variants used in email templates, for example:

* `[if mso]`
* `[if gte mso 9]`
* `[if lt mso 16]`
* `[if !mso]`
* `[if mso 12]`

A regex-based approach would help cover all of these cases more reliably.

In `src/TokenOrTokensPreAlter.php`, we could consider replacing the hardcoded check with:

```php id="cw4ywg"
foreach (reset($matches) as $match) {
// Skip MSO/IE conditional comment syntax (e.g. [if mso], [if gte mso 9], [endif]).
if (preg_match('/^\[(?:if\b|endif\b)/i', $match)) {
continue;
}

if (strpos($match, '|') !== FALSE) {
```

This would cover all MSO conditional variants without needing to hardcode specific strings.

milanbombschliip’s picture

milanbombschliip commented

@pianomansam Thank you for the input!

I tested with token_or disabled and the error does not happen.

I agree with the general solution, but I could not think of one, so I was: Better have this than no solution and maybe later in another ticket it can be done more general.

I also added a test and realized, 'clear' => true also needs to be set. So was definitiv a good idea to add a test :-)

milanbombschliip’s picture

milanbombschliip commented

@mukeshaddweb Yes, good point. If @pianomansam agrees feel free to push this change to my MR.

pianomansam’s picture

pianomansam commented

@milanbombschliip if the work from @mukeshaddweb is added, we will need extensive test coverage for it.

milanbombschliip’s picture

milanbombschliip commented

@pianomansam then probably merging and letting @mukeshaddweb create a new ticket is probably the better approach.

pianomansam’s picture

pianomansam commented

@milanbombschliip I appreciate your work on this. Your issue fork is valuable because it provides a temporarily fix with the patch that can be applied with composer. However, I still prefer a more generalized approach like @mukeshaddweb suggests. This ticket remains the best place to do that.