XOOPS / XoopsCore25

XOOPS Core 2.5.x (current release is 2.5.11: https://github.com/XOOPS/XoopsCore25/releases)
GNU General Public License v2.0
71 stars 59 forks source link

Link in notification messages #1468

Open GregMage opened 3 months ago

GregMage commented 3 months ago

It seems to me that this problem has existed since the switch to smarty 3. Before, when one received a notification message, the link was clickable. Now it's not!

It's not practical...

image

mambax7 commented 3 months ago

You screenshot is from 2017 I check on my email notifications from this year (after we moved to Smarty 3) and all of them had links in them.

image
GregMage commented 3 months ago

Okay, we're not talking about the same thing.

I'm not talking about e-mail notifications, but private messages. Before, the link was clickable. The date of the message is not important in my example. The monxoups.fr site was in version 2.5.11-Beta2 until June 6, 2024. The links were clickable. I upgraded to version 2.5.11-Stable and all the notification links in my private message box are no longer clickable! We've lost this feature since the switch to smarty 3, we were supposed to have a system that transformed urls into links automatically. I think Richard removed this feature for security reasons, but I'm not sure!

geekwright commented 3 months ago

Did some quick testing on xoops.org with pm module.

A message with just a URL only, all as one line, became clickable as expected.

https://github.com/XOOPS/XoopsCore25/pull/1445

The same URL inside of other lines displayed as plain text.

blah, blah ..

https://github.com/XOOPS/XoopsCore25/pull/1445

testing

TextSanitizer::makeClickable() appears to have a regex issue in handling line breaks.

mambax7 commented 3 months ago

@geekwright OK, I'll look into it this weekend and fix it

mambax7 commented 3 months ago

@GregMage please check out the fix and test if it works for you: https://github.com/mambax7/XoopsCore25/commit/91c236013143f6e2a61bbf4654d1ce0a52a22d84 If it works, please close the issue.

@geekwright The PhpUnit tests for makeClickable() are here: https://github.com/mambax7/XoopsCore25/blob/feature/2512/tests/unit/htdocs/class/MyTextSanitizerTest.php

Please let me know if we need to add any extra tests for this function

GregMage commented 3 months ago

Hi, there,

It's not fixed! Before your modifications:

image

No links are clickable. After your modifications:

image

The links in the signature work, but the main link does not!

mambax7 commented 3 months ago

Can you email me the whole link, so I could test it here locally? Thanks!

GregMage commented 3 months ago

They're just notification messages. In this case it's newbb. You can even test on xoops.org, the problem is there.

mambax7 commented 3 months ago

I just tested locally by sending a private message to myself with various links, and all of them worked, incl. a link to NewBB on XOOPS.

image

Can you test if you can send a PM to yourself with the link that didn't work (but makeClickable() with my changes)? If the PM works, then it's not the TextSanitizer, but probably the notification template.

GregMage commented 3 months ago

The first thing is that I don't write a PM but it's a notification message. The problem comes from the file “module.textsanitizer.php” because if I put the old one (before the upgrade to xoops 2.5.11-Stable) it works! I wrote the following message in a mp to test: ` test:

https://www.monxoops.fr/modules/newbb/viewtopic.php?topic_id=139&post_id=1440#forumpost1440

Bye `

And the link is not clickable...

GregMage commented 3 months ago

If I make a PM like you I get clickable links! Take exactly the MP above and you'll see that in this case it doesn't work!

mambax7 commented 3 months ago

I tested it now on XOOPS and the content (URL link and email address) works fine, but the link to the original post is not working:

image

1) This is not related to the change by Luciorota (https://github.com/XOOPS/XoopsCore25/pull/1445), as his change was related only to email and not URL

2) The makeClickable() method works fine for content, because it worked in your PM and in this content

3) I suspect, it has something to do with the notification templates and how they are being handled. I'll look into later tonight and will keep you posted...

If you're still awake, let me know what code did you exactly change that made it work again, was it the whole module.textsanitizer.php file? Please provide me with a link to that exact file, because files change over time, so I need to know which file exactly is working. Thanks!

mambax7 commented 3 months ago

@GregMage

It seems to be fixed now.

Copy the code of these two methods:

protected function makeClickableCallbackEmailAddress($match)
public function makeClickable($text)

from here: https://github.com/mambax7/XoopsCore25/blob/feature/2512/htdocs/class/module.textsanitizer.php

And let me know if it's working for you.

GregMage commented 3 months ago

Perfect, it works now! Thanks a lot! Why do you put this problem as closed? I don't see any Pull requests that fix this here.

mambax7 commented 3 months ago

It was fixed in the upcoming XOOPS 2.5.12

Michael

On Mon, Jun 10, 2024 at 02:35 Grégory Mage @.***> wrote:

Perfect, it works now! Thanks a lot! Why do you put this problem as closed? I don't see any Pull requests that fix this here.

— Reply to this email directly, view it on GitHub https://github.com/XOOPS/XoopsCore25/issues/1468#issuecomment-2157441574, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEV2NQN55LPJKOS6XQECSLZGVCK5AVCNFSM6AAAAABI62LATSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJXGQ2DCNJXGQ . You are receiving this because you modified the open/close state.Message ID: @.***>

GregMage commented 3 months ago

No code fixes the problem at the moment. This issue will be closed when a PR is done!

mambax7 commented 3 months ago

@GregMage That's exactly how the process works: if something was closed, but you discover that a new bug appears, and the old was not fully fixed, just reopen it, no big deal!

GregMage commented 3 months ago

I didn't reopen it for this reason! The exit is closed with a PR proposed here and not on your repository. Otherwise people think that the version here no longer contains bugs.

This issue will be closed with the PR that fixes the problem.