Smiley parsing is handled in parsesmileys by way of an enormous regular expression that matches and handles certain types of replacements.
In the old setup, prior to PHP 5.5, this was handled by an /e expression and this was not problematic in PCRE even when throwing obscene numbers of smileys at it - say, 2000 unique smileys.
With the revised 2.0 and 2.1 code, that many smileys will produce a very large regexp which exceeds the limits set internally in the PCRE engine. Before, it got partially parsed before going to PCRE, but now it creates an atomic regexp, which when compiled appears to exceed the 64KB stack size.
In the test case we were examining, there were 2026 smileys creating a 44KB regular expression, which parses out way beyond 64KB.
Unfortunately there's no hard and fast rule about how many smileys are safe; more smileys with shorter filenames may work under some conditions. If it fails to work and there's a crazy number of smileys, try bringing the number down a bit.
Symptomatically: this manifests as a parse_bbc that fails to work, with all the apparent symptoms of a mismatch of encodings. Posts that have no smileys but do have code blocks will render as expected, and this can be used as a suitable diagnostic test to differentiate between encoding failure and parsing failure.
This should also throw an error on the following line in 2.0.x Subs.php:
$message = preg_replace_callback($smileyPregSearch, 'smileyPregReplaceCallback', $message);
I suspect the same behaviour would apply in 2.1 because even though there's now a use of closures, the failure still applies at the callback stage before closures get involved.
I do not think this is fixable in a permanent way without a complete rewrite of the parsing engine and this is completely out of any scope prior to 3.0 IMNSHO.
Who has 2000+ smileys in a post? If you do that, you SHOULD get an error :P ;D
Eheheheh D
No, it's not in one post.
2026 smileys configured in the admin panel causes *everything* to break because they end up being compiled into a single massive preg_replace_callback call that amounts to smiley1|smiley2|smiley3|smiley4|smiley5 etc. Even if posts don't *have* any smileys they break because they're pushed through the same parse step.
The only exception is for code blocks, where the content inside a code block is not pushed through that step.
Justyne and I were seriously confused the more we looked at this, because we had more and more symptoms that didn't add up and it looked to start with like a content mismatch but after poking and prodding the DB and various other things, it appeared the DB was fine being UTF-8.
$db_show_debug was our friend however since that also turns off certain error suppressions.
I'll stick with my thought. If you add 2026 smileys in admin, you deserve to have your forum shut down ;D ;D
Fun thing is, this works fine in 1.1.x because the preg_replace still uses /e. It's only on 2.0.7+ that this breaks.
Ugh..... ok. I say the fix is to limit the max number of smilies to 1000 or even 500...
I think we can do that in the smilies admin...
In the case we found, 1500 seemed acceptable without breakage.
Yes, you can do it in the smileys admin, but be careful about breaking user expectations.
Why not go to 11, as in 2025? :) Nigel Tufnel approves of this message.
Because 2025 breaks as spectacularly as 2026 does ;)
1500 is about the highest we could get away with while still working :(
And that's 1490 more than absolutely necessary :P
Yet I agree with this
Quote from: Arantor on October 20, 2014, 09:04:54 PM
but be careful about breaking user expectations.
Still, a fixed limit of... 1000?! Would be more than enough for 99.9% of all users. If the limit is easy to increase, better ;)
Yes, 1000 is probably safe for most cases.
Quote from: margarett on October 20, 2014, 09:33:07 PM
And that's 1490 more than absolutely necessary :P
Yet I agree with this
Quote from: Arantor on October 20, 2014, 09:04:54 PM
but be careful about breaking user expectations.
Still, a fixed limit of... 1000?! Would be more than enough for 99.9% of all users. If the limit is easy to increase, better ;)
I agree... How would you even name 1000 emotes differently? /emote1, /emote2, etc? None of the members would even bother trying to know or look more than 200 or something ;D
If its simple to modify, just leave it with a warning saying it may break your forum if you add more.
if we are going to support 1000 or more smileys, we need to consider the layout in 2.1. we are no longer using a popup and IMO what we have currently in 2.1 will not work if we have that many smileys.
https://github.com/SimpleMachines/SMF2.1/issues/2169#issuecomment-51769114
umm.... are you sure? that github does not say that the popup was removed.....
yes i am sure. the popup that did exist in 2.1 before on github is gone and we have this new version that will cover the whole post area if you have too many smileys.
that's a problem then....
it's bad UI.
I admit that we don't have to cover edge cases like 500+ smilies... but we should enable people to have 20-40 smilies without a bad display (and, per your comment here, that is not possible, even though the github thread that you linked to does not mention REMOVING the pop-up)
Removing the pop-up was a poor UI choice and should not have been done...
it was removed prior to that issue being opened/commented on. i had opened a topic about this in the old beta board with id of 527436 if you want to read up further. according to that issue it seems like no devs want to look into reverting back to what we had before...
I get the removal from a simplification viewpoint and it is definitely good for forum members as leafing through pages of more is just tedious. I guess admin desire collides with usability here.
If we revive the popup or really want to make the interface better I wonder if categorising smileys is something that can be done. If I really have 200 I might want to split them up like the apple emoji interface for example. All "love" related ones in one place, all the "angry" ones in a another.
i think at this point that is beyond what can be done in 2.1. maybe something to consider for 3.0.
Oh, definitely.
It just occurred to me while reading this that navigation tabs might be useful in communities with such a large amount of smileys.
basically, by removing the pop-up, we have preemptively limited the smilies to a max of 10-20 standard sized ones... as I said, it is a bad UX (and not something I realized that we had done)
:(
Why everyone always looking for user-side, I mean okay yes we are doing this for our community/users but if you dig to code you see that new pop-only smiley box is the default action came directly from SCEditor itself, in old builds there was an extra code to hide pop and show list. This changes (using more core stuff from SCEditor) allowed us to upgrade SCEditor more easily in the future. I think its better not to depend on extra code when we can avoid. Yet I must remind, I'm not a real developer people like Suki, Oldiesmann are... my opinions are mostly based on my Marketing side.
So, you're more interested in limiting user experience in favour of developer convenience, yes?
You realise most of the bone-headed UI in the admin panel is directly descended from that mindset, right? (Hint: not clever marketing to snub your users to make your devs' lives easier)
Well, I am looking at the UI/UX, because that is what people who use our software judge it on.
the functionality of the system is important, yes... but the UI/UX is just as important (that's why companies have entire departments dedicated to UX these days)
Actually, for 3.0, I was considering that we might want to do a full IA/UX analysis with user roles, card sorts, and all that jazz....
but, back to the issue at hand...
I am confused. I finally had a chance to return to my test site and confirm some things.
in the sceditor, I see the "pop-up" window for smilies... this is what confused me when Illori said that the pop-up was removed.
The row of smilies, like we have now, is gone... that is fine.
The pop-up that is in sceditor, and then duplicated in the BBC editor is fine.
but, we do need to consider the no-so-edge cases of up to 100 smilies and then 100-200 smilies... I admit that 200-500 and 500-1000 are likely to be edge cases... but up to 100, while obnoxious, IMO, is not an edge case and is actually quite likely... even up to 200 is still realistic, given the admins....
So, we need to consider how (up to at least 200) smilies get displayed... especially if they are not all 16x16 icons.
'
Quote from: Kindred on October 21, 2014, 09:43:36 AM
in the sceditor, I see the "pop-up" window for smilies... this is what confused me when Illori said that the pop-up was removed.
The row of smilies, like we have now, is gone... that is fine.
The pop-up that is in sceditor, and then duplicated in the BBC editor is fine.
yes you see a popup, but in 2.0 and earlier 2.1 we had a popup when you clicked more which IMO took care of the issues we now have with smileys. what we have now is a mess.
we also dont have a matching layout in the admin panel for how the smileys look when viewing them for posting. which is another issue.
Quote from: Arantor on October 21, 2014, 09:35:17 AM
So, you're more interested in limiting user experience in favour of developer convenience, yes?
You realise most of the bone-headed UI in the admin panel is directly descended from that mindset, right? (Hint: not clever marketing to snub your users to make your devs' lives easier)
Yes, because efficiently level its more useless, then why we removed Karma? or left support of PHP 3? I can say more examples about that. Maybe some of the other non-teamed developers needs to consider this as well, so they won't spend years on releasing products to users. Also I don't see any limitations in this usage, it makes my UI more clear. Maybe some mind-sets stops being selfish and tries to adapt new changes and have some
respect mercy on decision taken by team. People can find old code in history and they can add the trick back (or make mod out of it). I took the decision to remove those lines and Lead Developer agreed to include it.
Its a clever move to make release/upgrade easier with removing/changing something little (and yes Its little for me).
Quote from: Illori on October 21, 2014, 10:05:17 AM
Quote from: Kindred on October 21, 2014, 09:43:36 AM
in the sceditor, I see the "pop-up" window for smilies... this is what confused me when Illori said that the pop-up was removed.
The row of smilies, like we have now, is gone... that is fine.
The pop-up that is in sceditor, and then duplicated in the BBC editor is fine.
yes you see a popup, but in 2.0 and earlier 2.1 we had a popup when you clicked more which IMO took care of the issues we now have with smileys. what we have now is a mess.
we also dont have a matching layout in the admin panel for how the smileys look when viewing them for posting. which is another issue.
and the problem is ?
reported here: https://github.com/SimpleMachines/SMF2.1/issues/2169
QuoteYes, because efficiently level its more useless, then why we removed Karma?
Because it was a user problem, not a technical one. There were no technical issues with karma.
Quoteor left support of PHP 3?
Because PHP 3 era code doesn't work on PHP 5.3+. Again: a user requirement, not a technical one.
QuoteMaybe some of the other non-teamed developers needs to consider this as well, so they won't spend years on releasing products to users
They need to build what the users want. Otherwise it's a waste of time.
Interestingly, Antes... despite the lead dev approving your change... Oldiesmann clearly says
Quote
Again, why are we changing how smileys are displayed? SMF has always displayed them either inline on the editor or in a popup when a "[more]" link is clicked. It should remain that way in my opinion.
Problem here you skip, which I'll remind you is the "competition" if you spend more time than its needed on products you become late supplier which is an ultimate fail for your product. I'm sure you already know those, but its still a
friendly reminder from this cat ;)
So I kinda find it wrong to call "not cleaver" is not cleaver. After all if you keep loosing your user-base there is no point of making further development on such product (unless you are really a big looser to feed your ego daily). So I ask you not to bring all this directly to "User" level see more in general product life cycle. Yet It wasn't me all talking about the decline.
Quote from: Kindred on October 21, 2014, 10:48:31 AM
Interestingly, Antes... despite the lead dev approving your change... Oldiesmann clearly says
Quote
Again, why are we changing how smileys are displayed? SMF has always displayed them either inline on the editor or in a popup when a "[more]" link is clicked. It should remain that way in my opinion.
I need date on that, I didn't see any comment from Oldiesmann about that in IRC, he wants to keep this in that way from now on. I'll ofc respect the decisions taken by development team no discussion needed about that :)
*shrug* What good is being first to market if what you bring to market is considered mediocre at best?
Hint: Apple is almost never first to market in anything. It doesn't appear to be a significant problem for them.
Quote from: Arantor on October 21, 2014, 10:59:00 AM
*shrug* What good is being first to market if what you bring to market is considered mediocre at best?
I don't see any reference about being first in my post, I was talking about being late... I suggest before further *shrugs* try to understand the point not dictate your owns as right ones.
QuoteHint: Apple is almost never first to market in anything. It doesn't appear to be a significant problem for them.
After Watergate scandal Richard Nixon said I/we didn't do anything yet resigned.
Data taken from: http://www.idc.com/prodserv/smartphone-os-market-share.jsp (http://www.idc.com/prodserv/smartphone-os-market-share.jsp)
I'm sorry, I shouldn't have reported this. I shouldn't have tried to make this a thing that needed fixing and I certainly shouldn't have bothered trying to suggest this be reviewed for 2.1.
Hope it works out for you.
Antes,
It's often a trade-off... yes.
However, it is better to be a little bit later with a good UX than to be "early" or even "on time" with UX issues.... and things which the user is going to look at and say "that looks terrible" is a UX issue.
It's one thing to force users to limit smilies to 500 or even 200... it's quite another to tell them that they get 20 smilies, maybe more - but all of them have to be 16x16 and standard in order to fit the design. (additionally, as Illori points out, the admin interface for smilies now doesn't match the user-side implementation)
So - I understand the reasoning behind the original change now... I think there is a trade-off between "ease of upgrade" and user experience.
Just because we have always done something one way does not make it a requirement to continue doing so... I agree with that statement... but before we remove a piece of functionality and layout that MANY systems use, we need to consider the impact.
(and, just look at how many people link to and steal k@'s smilie sets for some idea on how prevalent the non-standard smilie use actually is)
BTW: Arantor, I appreciate the report - although it is an edge case, it does need to be considered... (even if it is discounted in the end) and the fact that it raised an issue with how 2.1 was designed is actually important as well (and not your fault at all -- if I had noticed the 2.1 stuff before this, I would have raised the same complaint that I just did :) )
hmmm.... not sure who locked this...
(no entry in the moderation log...)
Quote from: Kindred on October 21, 2014, 01:42:19 PM
hmmm.... not sure who locked this...
(no entry in the moderation log...)
that i what happens when the op locks their own thread.
ah... I'll respect the lock then... relocking.
hmmm.... that seems like another bug, (albeit a minor one). Mod log should track actions on the user's own topic