Fix Email address being removed in the link text and fix every second link not being obfuscated

Uploading a patch for composer patching based on the opened MR.

@joevagyok thanks! Also needs tests, please. Shouldn't break again.

@Anybody Tests are implemented, see patch and Spamspan FunctionalJavascript test class.

Thanks @joevagyok this looks excellent to me and is indeed a major problem. So let's call this a bug.

I think we need to be aware of certain cases and test that, especially with the "Convert URLs into links" filter! See https://www.drupal.org/project/spamspan/issues/3373844#comment-15145179 📌 Remove wrong filter order assumption in the README. Needs work for details.

We need to ensure that the following text entered into the Text Editor and filtered by "Convert URLs into links" filter works as expected and is not double-obfuscated or something like that.

First question is: How does the core "Convert URLs into links" filter handle the following examples:
<a href="mailto:info@example.com">info@example.com</a>
<a href="mailto:info@example.com">Contact us: info@example.com</a>
Contact us: info@example.com

(without using spamspan). Can someone try that, please?

If one of them ends up in:
<a href="mailto:info@example.com"><a href="mailto:info@example.com">info@example.com</a></a>
I guess that would be a bug in that filter, and I hope (and think) it doesn't.

Once that's safe, we can proceed with the examples from the issue summary.

I even think this is a major bug. The examples listed in #10 are very typical. We should also have tests for these with and without JS.

Normally you would set it up in a way that "Convert URLs into links" filter is executed first on the text, then the spamspan.
We are testing this case in our platform. There is no double obfuscation happening. I can confirm it is working fine. I will send screenshot to @Grevil on slack.

Here I upload 3 screenshot with the examples mentioned in #10:

1. The body field content in the WYSIWYG
2. The result text without JavaScript enabled, in obfuscated format
3. The result text with JavaScript enabled, in normalized format

This was actually a very good exercise because I have noticed another bug:
Using the following example:

<a data-link-style="default" href="mailto:first@example.com">first@example.com</a></p>

<p><a data-link-style="default" href="mailto:second@example.com">second@example.com</a></p>

<p><a data-link-style="default" href="mailto:third@example.com">third@example.com</a></p>

<p><a data-link-style="default" href="mailto:fourth@example.com">fourth@example.com</a></p>

Every second link a tag was not obfuscated, as you can see in the previous comment's image → , the markup contains a link making the second example show as link even though JavaScript is not enabled. This should not happen!

It is due to the SpamspanDomTrait::processAsDom() functions logic where replaceDomNode function replaces the existing DOM node with a new one. Because of the way PHP's DOMDocument works, this operation modifies the DOMDocument in place. So the next time the loop in processAsDom continues, it's iterating over a modified document, which may not contain the same elements in the same order as before resulting in the problem presented above.

I will push a fix and a test covering this case in the MR. I will upload a patch too for composer.

Nice find @joevagyok let me know when it is ready to review! :)

Changes concerning the original email adress link text removal LGTM! Same with the iteration bug code! Let's see what the finished tests have to say, once @joevagyok finishes them!

@Anybody are you fine with tackling the "iteration bug" mentioned in #14 in this issue (after renaming the issue)? Or would you prefer to add a follow-up issue?

I pushed the fix for the iteration problem and ported the test into the new test structure.

Very nice @joevagyok! RTBC from my side! Let's wait for @Anybody for the final approval, concerning the extra setting and whether or not to split this issue.

In order to comply with the request to test this without JS too, I adapted the kernel test in my last commit.

All green! And kernel test looks good. Just talked with @Anybody internally and he agrees with fixing both issues here!

Thanks a LOT @joevagyok!

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