Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link in notification messages #1468

Open
GregMage opened this issue Jun 7, 2024 · 18 comments
Open

Link in notification messages #1468

GregMage opened this issue Jun 7, 2024 · 18 comments
Labels

Comments

@GregMage
Copy link
Contributor

GregMage commented Jun 7, 2024

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

@GregMage GregMage added the bug label Jun 7, 2024
@mambax7
Copy link
Collaborator

mambax7 commented Jun 8, 2024

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
Copy link
Contributor Author

GregMage commented Jun 8, 2024

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
Copy link
Contributor

geekwright commented Jun 8, 2024

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
Copy link
Collaborator

mambax7 commented Jun 8, 2024

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

mambax7 added a commit to mambax7/XoopsCore25 that referenced this issue Jun 9, 2024
@mambax7
Copy link
Collaborator

mambax7 commented Jun 9, 2024

@GregMage please check out the fix and test if it works for you: mambax7@91c2360
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
Copy link
Contributor Author

GregMage commented Jun 9, 2024

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
Copy link
Collaborator

mambax7 commented Jun 9, 2024

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

@GregMage
Copy link
Contributor Author

GregMage commented Jun 9, 2024

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

@mambax7
Copy link
Collaborator

mambax7 commented Jun 9, 2024

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
Copy link
Contributor Author

GregMage commented Jun 9, 2024

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
Copy link
Contributor Author

GregMage commented Jun 9, 2024

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
Copy link
Collaborator

mambax7 commented Jun 9, 2024

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 (update regex for makeClickable($text) #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
Copy link
Collaborator

mambax7 commented Jun 10, 2024

@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.

@mambax7 mambax7 closed this as completed Jun 10, 2024
@GregMage
Copy link
Contributor Author

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
Copy link
Collaborator

mambax7 commented Jun 10, 2024 via email

@GregMage GregMage reopened this Jun 13, 2024
@GregMage
Copy link
Contributor Author

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

@mambax7
Copy link
Collaborator

mambax7 commented Jun 13, 2024

@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
Copy link
Contributor Author

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.

mambax7 added a commit to mambax7/XoopsCore25 that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants