Feel free to discuss anything interesting you see about the changelog updates!
Nao
- Dadman with a boy
-
-
- Posts: 16,079
Arantor
-
-
- As powerful as possible, as complex as necessary.
- Posts: 14,278
* encodeURIComponent() has been supported as far back as IE 5.5, so there's no reason to use SMF's way of emulating it
* php_strtr uses charAt when it could use [] instead. It's a bit faster, which is good in long loops. (script.js)
I'm letting them be for now because they're testing for %u in the URL, which only happens in escape()'d strings
When we unite against a common enemy that attacks our ethos, it nurtures group solidarity. Trolls are sensational, yes, but we keep everyone honest. | Game Memorial
Nao
- Dadman with a boy
-
-
- Posts: 16,079
Do they behave differently? Specifically, the php_* functions in JS are not necessarily exact analogues of existing or later-defined JS structures, they are implemented specifically to replicate how PHP does things especially where character sets are concerned (since .toLowerCase() can behave rather differently to strtolower(), as a typical example)Quote * encodeURIComponent() has been supported as far back as IE 5.5, so there's no reason to use SMF's way of emulating it
Do you remember any reason for using php_strtolower?
As for php_urlencode, yes, encodeURIComponent gives results similar to PHP's urlencode(). I tried a silly combination of random chars and both gave me the same results, so I assume I could use it freely. (Which is why I committed my change without further research. If it's buggy, we'll fix. But I don't know how it will.)
From what you probably saw in my commit, SMF's implementation of the whole thing was kinda bastardized. They used different methods for encoding strings, and added useless crap here and there.
* php_strtr uses charAt when it could use [] instead. It's a bit faster, which is good in long loops. (script.js)
Is that all they're actually doing?Quote I'm letting them be for now because they're testing for %u in the URL, which only happens in escape()'d strings
That's one of the slightly maddening things about the codebase - stuff like that has been in there for years and it is a little tricky at times prising out the specific reason for it.
Arantor
-
-
- As powerful as possible, as complex as necessary.
- Posts: 14,278
The reason for using strtolower done like that is for accuracy. JavaScript's one is way more aware of things like UTF-8 than PHP's has been for a long time, and I suspect with PHP 5.4 this will have to be revisited anyway.
But ultimately the two have to be done exactly the same for password hashing to actually work, so JS has to then proceed to use the exact same case adjuster even if the browser could do a "better" job of it.
I suspect the different methods were used because browsers of the day couldn't be relied upon to be consistent. Heck, there's whole bushels of pre IE6 code even in 2.0 (like the popup crap for quoting posts)
Quote I had actually fixed that locally, though I've not committed what I have because I'm not happy with it yet, concerning how users select timezones. Still not sure about that, maybe I should just commit what I have (which is to use a timezone column in the user's table and fallback to the offset for the time being)
But ultimately the two have to be done exactly the same for password hashing to actually work, so JS has to then proceed to use the exact same case adjuster even if the browser could do a "better" job of it.
I suspect the different methods were used because browsers of the day couldn't be relied upon to be consistent. Heck, there's whole bushels of pre IE6 code even in 2.0 (like the popup crap for quoting posts)
Posted: July 28th, 2011, 07:10 PM
! If a timezone has no slash in it (such as 'UTC'), the timezone retrieval code will generate a warning. (ManageSettings.php)
live627
-
-
- Should five per cent appear too small / Be thankful I don't take it all / 'Cause I'm the taxman, yeah I'm the taxman
- Posts: 1,670
A confident man keeps quiet.whereas a frightened man keeps talking, hiding his fear.
Nao
- Dadman with a boy
-
-
- Posts: 16,079
The reason for using strtolower done like that is for accuracy. JavaScript's one is way more aware of things like UTF-8 than PHP's has been for a long time, and I suspect with PHP 5.4 this will have to be revisited anyway.
But ultimately the two have to be done exactly the same for password hashing to actually work, so JS has to then proceed to use the exact same case adjuster even if the browser could do a "better" job of it.
Which is why it's only used in sha1.js... :)
I had actually fixed that locally, though I've not committed what I have because I'm not happy with it yet, concerning how users select timezones. Still not sure about that, maybe I should just commit what I have (which is to use a timezone column in the user's table and fallback to the offset for the time being)
Another question.
Your number convert function -- isn't he comma_format stuff going to kill performance if used on a larger scale...?
Do you plan to convert more strings into using that? Or should I do it? :P
Arantor
-
-
- As powerful as possible, as complex as necessary.
- Posts: 14,278
Is my fix okay for you? Feel free to rewrite it... I only fixed the error, really.
Your number convert function -- isn't he comma_format stuff going to kill performance if used on a larger scale...?
The biggest culprit is the board index but that ran comma_format anyway, so it's one function wrapper and an if+isset overhead on top per value per board on the board/message index (1 per redirect board, 2 per normal board), it'll be another 2 call+if+isset on the message index for the counts of views/replies, plus another for the view count inside a topic. The actual add-on load is really not worth being too concerned about, IMO.
Nao
- Dadman with a boy
-
-
- Posts: 16,079
I haven't looked (been way too busy with stuff) but I suspect it's similar to the one I have on my local copy, which also has the ability to load the timezone from the members table as well.Quote Is my fix okay for you? Feel free to rewrite it... I only fixed the error, really.
Not really, no, because it's not actually used all that often in the scheme of things.Quote Your number convert function -- isn't he comma_format stuff going to kill performance if used on a larger scale...?
Posted: August 1st, 2011, 10:21 AM
So, who's going to convert the rest of the plural thingies?
Arantor
-
-
- As powerful as possible, as complex as necessary.
- Posts: 14,278
Again, I didn't touch the timezone code. I only fixed the loop that retrieves them, to ensure it won't generate a warning on the list (,) code (since UTC doesn't have a comma and thus throws out an error when splitting it.)
I suppose we could have used it more, though
So, who's going to convert the rest of the plural thingies?
Nao
- Dadman with a boy
-
-
- Posts: 16,079
Yeah, that's what I meant; I've fixed the loop myself separately, but I'll fix it when I commit what I do have.
Yes, it can be used more, because it's not heavy in itself, it's only heavy if comma_format wasn't being used already, but the places that occurs are not common.
That one's on my todo list.Quote So, who's going to convert the rest of the plural thingies?
Note to self: merge aeva_utf2entities() into westr::cut().
Arantor
-
-
- As powerful as possible, as complex as necessary.
- Posts: 14,278
I've dealt with the existing _n plurals in rev 900, too. And added one that was in the index language file. Should be a good start
Nao
- Dadman with a boy
-
-
- Posts: 16,079
Yeah, that's what I'm thinking too. The admin area doesn't need as much attention as the front end, at least when it comes to these details.
Small remark on rev 901. ManageSettings.php got screwed up during your merge operation. You cancelled my changes in the timezone retrieval loop, but didn't cancel the line just before it that did an array_flip(). As a result, it's broken. I've restored my copy (which uses isset() instead of in_array() and should thus be faster), I'll commit it again when I have something else to commit (other than minor details in the ManageMedia template.)
Small remark on rev 901. ManageSettings.php got screwed up during your merge operation. You cancelled my changes in the timezone retrieval loop, but didn't cancel the line just before it that did an array_flip(). As a result, it's broken. I've restored my copy (which uses isset() instead of in_array() and should thus be faster), I'll commit it again when I have something else to commit (other than minor details in the ManageMedia template.)
Arantor
-
-
- As powerful as possible, as complex as necessary.
- Posts: 14,278
Bah, I missed the array_flip, thinking you were just doing the isset check on it at the end and that it wouldn't work as expected then...
Nao
- Dadman with a boy
-
-
- Posts: 16,079
Bah, I missed the array_flip, thinking you were just doing the isset check on it at the end and that it wouldn't work as expected then...
Arantor
-
-
- As powerful as possible, as complex as necessary.
- Posts: 14,278
No... the original code had an in_array to select only the regions of interest, and IIRC there was an isset for something else at the end before actually adding it to the loop but off hand I can't remember what.
Right now am dealing with the fact that the imperative scheduled system is completely broken (and in fact I'm dubious about how it ever worked when I tested it originally because it shouldn't have done), will commit that when I've fixed it and finished the feature that actually uses it.
Right now am dealing with the fact that the imperative scheduled system is completely broken (and in fact I'm dubious about how it ever worked when I tested it originally because it shouldn't have done), will commit that when I've fixed it and finished the feature that actually uses it.