OMDocumentFragment::appendXML(): Entity: line 1: parser error : EntityRef: expecting ';' in Drupal\glossify\GlossifyBase->parseTooltipMatch()

I created a patch to convert special characters to HTML entities.

Patch #2 has security implications, so this should be checked with care.
Thank you for the patch so far! :)

It seems like this can be reproduced by creating a Glossify term containing an ampersand ("&"). And then running the replacement.

Could someone please try to reproduce this and then write a test?
Maybe also for other special characters?

Okay I at least found a way to fix the errors, but I think the replacement may not work propely in all cases now, but that's less important.

Hotfix attached in MR. For further implementation, this should first have tests.

Okay I at least found a way to fix the errors, but I think the replacement may not work propely in all cases now, but that's less important.

Hotfix attached in MR. For further implementation, this should first have tests.

Green tests are not enough here due to missing test coverage, but I think if the hotfix from MR!27 works, we should merge it already to improve the risk for people running into this.

We should keep this major and add tests next to reproduce it.

In my case it happened on taxonomy terms with the title "Lorem & ipsum". It resulted in user session being discarded due to the errors.

Merged the hotfix! See #9 for further steps.

I started with https://git.drupalcode.org/issue/glossify-3351345/-/tree/3351345-test-only

that needs to be completed, but even then it's not yet sure it will fail (but it's definitely a good test to add).
But for the test-only we'll need to revert the fix there!

Has this been resolved? Is there a patch one can apply?

I just experienced this too. It can cause text to be completely left out in a scenario where you have two glossify terms in one text segment, and the second term has an ampersand in it. All the text after the first term is not output at all :(

I'll look at the code, though it does seem pretty complicated.

So, on 3.0.3, with the hotfix from this issue, I have problems with parsing this scenario:

• Glossify term "testing"
• Glossify term "PB&J"
• Text that reads "I am testing a sentence with PB&J in it"

I get output "I am testing". The rest is cut off entirely.

When I downgrade to 3.0.2, everything works as expected, and I don't get the original PHP warning that this issue posted about. I'm on PHP 8.1.x if that matters at all?

Can we have clearer steps to reproduce the problem? As it stands now, the hotfix in 3.0.3 didn't fix anything for my situation, but broke it. Hmm.

bkosborne → changed the visibility of the branch 3351345-omdocumentfragmentappendxml-entity-line to hidden.

I created a new MR that reverts the previous hotfix and implements what I think is simpler solution. The problem is that we're using ->appendXML to attach HTML to the DOM Document. This isn't really correct, because what's valid in HTML may not be valid in XML. That's why we're running into issues with &, which is perfectly valid in HTML but it must be escaped to & in XML.

I think the real solution here is to find a way to properly attach HTML to the DOM Document instead of using ->appendXML. Short term, we can cover most use cases by simply escaping the invalid XML characters. I say this is short term because we're using ->appendXML to append the rendered tooltip, and I don't think we can really mess with the rendered output by escaping it.

Note that for the test, I fixed the mock render code to actually perform a simple version of twig autoescaping.

Hi, I tested with a special character like "O&O," and noticed that the output is incorrect. Some text is entirely missing. The issue occurs because the & in "O&O" is treated as an unescaped entity in XML. Merge Request (MR) #31 resolves this issue for me.

@kushagra.goyal could you please add another test for that in MR!31? Thanks!

@bkosborne which other tests might be helpful to ensure we don't break something in this complexity?

The test I added in MR !31 already covers what kushagra.goyal ran into.

I'm not really sure what other tests could be added. I think that if people currently have an & in their glossary term, things are already broken for them, and this should fix it. They may not even know it's broken if they're not closely paying attention to their output.

@bkosborne thanks for your feedback! So what would you say, how far are we away from RTBC?
The tests are green, so if the tests are fine, it should be fine too, but yeah this so complex, so I'm still afraid to merge it, even if I tested it manually.

One remaining question, otherwise this LGTM!