minetest-mods / irc

IRC Mod for Minetest
BSD 2-Clause "Simplified" License
43 stars 31 forks source link

fix crash that prevents joining channel #69

Closed MisterE123 closed 2 years ago

MisterE123 commented 2 years ago

The mod soft-crashes with: 2022-01-08 06:53:03: ERROR[Server]: IRC: Connection error: irc.freeirc.org: /home/ubuntu/minetest/bin/../mods/irc/hooks.lua:174: attempt to index local 'user' (a nil value) -- Reconnecting in 600 seconds...

this fixes the error and allows the bot to join the channel.

SmallJoker commented 2 years ago

user is not nil in other callbacks. If there's a notice from an unknown user, the action is invalid and the callback should not be run in first place.

MisterE123 commented 2 years ago

ah yes, the {} is because when used with the matterbridge irc->discord/matrix bridge, the player names are filtered out on matrix, if <> is used, but with {} they still are shown.

On Tue, Jan 18, 2022, 10:02 AM SmallJoker @.***> wrote:

@.**** commented on this pull request.

In messages.lua https://github.com/minetest-mods/irc/pull/69#discussion_r786847060:

@@ -13,5 +13,5 @@ function irc.sendLocal(message) end

function irc.playerMessage(name, message)

  • return ("<%s> %s"):format(name, message)
  • return ("{%s} %s"):format(name, message)

Unrelated change?

— Reply to this email directly, view it on GitHub https://github.com/minetest-mods/irc/pull/69#pullrequestreview-855551376, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOSK5KAMM6IYDPY6REVP6NTUWV6KFANCNFSM5LQJEPYQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

MisterE123 commented 2 years ago

I believe that user refers to the irc user, specified in minetest.conf.

regardless, without this change, the mod does not even join the irc channel, and is useless. my fork works, yours doesn't. feel free to test to confirm. btw, I am using nickserv if that makes a difference

On Wed, Jan 19, 2022, 8:11 AM Gaudeor Rudmin @.***> wrote:

ah yes, the {} is because when used with the matterbridge irc->discord/matrix bridge, the player names are filtered out on matrix, if <> is used, but with {} they still are shown.

On Tue, Jan 18, 2022, 10:02 AM SmallJoker @.***> wrote:

@.**** commented on this pull request.

In messages.lua https://github.com/minetest-mods/irc/pull/69#discussion_r786847060:

@@ -13,5 +13,5 @@ function irc.sendLocal(message) end

function irc.playerMessage(name, message)

  • return ("<%s> %s"):format(name, message)
  • return ("{%s} %s"):format(name, message)

Unrelated change?

— Reply to this email directly, view it on GitHub https://github.com/minetest-mods/irc/pull/69#pullrequestreview-855551376, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOSK5KAMM6IYDPY6REVP6NTUWV6KFANCNFSM5LQJEPYQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

MisterE123 commented 2 years ago

whenever I have used the irc mod, I have been forced to make this change to get the bot to join the irc channel.

its up to you if you want to merge the change or not, I made the pr as a help to others; I will use my fork because it actually works.

here is my minetest.conf:

secure.trusted_mods = irc
irc.server = irc.freeirc.org
irc.channel = #blockbomber-ingame
irc.interval = 0.0
irc.nick = IG
irc.username = IG
irc.realname = IG
irc.NSPass = ****

if that helps you confirm the bug

On Wed, Jan 19, 2022, 8:16 AM Gaudeor Rudmin @.***> wrote:

I believe that user refers to the irc user, specified in minetest.conf.

regardless, without this change, the mod does not even join the irc channel, and is useless. my fork works, yours doesn't. feel free to test to confirm. btw, I am using nickserv if that makes a difference

On Wed, Jan 19, 2022, 8:11 AM Gaudeor Rudmin @.***> wrote:

ah yes, the {} is because when used with the matterbridge irc->discord/matrix bridge, the player names are filtered out on matrix, if <> is used, but with {} they still are shown.

On Tue, Jan 18, 2022, 10:02 AM SmallJoker @.***> wrote:

@.**** commented on this pull request.

In messages.lua https://github.com/minetest-mods/irc/pull/69#discussion_r786847060:

@@ -13,5 +13,5 @@ function irc.sendLocal(message) end

function irc.playerMessage(name, message)

  • return ("<%s> %s"):format(name, message)
  • return ("{%s} %s"):format(name, message)

Unrelated change?

— Reply to this email directly, view it on GitHub https://github.com/minetest-mods/irc/pull/69#pullrequestreview-855551376, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOSK5KAMM6IYDPY6REVP6NTUWV6KFANCNFSM5LQJEPYQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you authored the thread.Message ID: @.***>

mckaygerhard commented 2 years ago

user is not nil in other callbacks. If there's a notice from an unknown user, the action is invalid and the callback should not be run in first place.

another proof of the same error: the error was never really solved.. its just that is not happened so much anymore.. but still present in some servers..

as i send to https://github.com/minetest-mods/player_monoids/pull/7#issuecomment-1016582274 @SmallJoker :laughing:

mckaygerhard commented 2 years ago

its up to you if you want to merge the change or not, I made the pr as a help to others; I will use my fork because it actually works.

Thanks @MisterE123 as must be.. same happened to me as i posted before

Miniontoby commented 2 years ago

Check my Pull Request (https://github.com/minetest-mods/irc/pull/71) for also this sort of fix, but with explanation for the fix

SmallJoker commented 2 years ago

Thank you for the explanation. Yes, this makes sense. Merged #71 because it contains a precise error description and no unrelated changes. As for the name formatting changes: what would you think about a setting to specify the string.format format argument? This way it would be possible to keep the current notation where wanted. After all this mod was designed for IRC.

MisterE123 commented 2 years ago

Yes that would work. Thank you miniontoby for investigating and explaining the problem. Unfortunately, I didbnot invest the time to really understand the code; I found the issue and made an educated guess at how to fix it, and it worked. Maybe not the best method, but sufficient for my purposes. I understand not wanting to merge it without a detailed explanation; though I would think that this error is quite reproduceable (I have reproduced it every time I used the mod lol) and could have been investigated more thoughoughly reasonably easily.

I also understand being stretched thin (that is why I did not investigate this much more than to fix it), so I dont blame you.

Anyhow, regarding the string.format... Up to you. I think that the reason for the string change is fairly niche, and by now there is a matterbridge mod that connects directly to matterbridge instead of needing the irc relay; so there may be a better solution now. Personally, I wouldnt waste time changing it unless someone else has a usecase that requires it or you just feel like doing it.