New revs - Public comments

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: New revs - Public comments
« Reply #30, on August 2nd, 2011, 01:13 AM »
That's why I said the status field didn't have to just have two values -- unapproved and approved.
0: unapproved, unmoderated
1: approved
2: deleted (recycled)
3: postponed for later?
Posted: August 2nd, 2011, 12:33 AM

Hmm, I've noticed a strange behavior on the demo site that isn't on my local site.
When doing floating point manipulations in the wecss class, any floating point number is stored internally as "1,23" instead of "1.23". It always retains that comma.

I have to go, but maybe someone can tell me what's happening? I suspect it's somewhere in the setlocale stuff, but I don't know how to fix for now. The problem is that I'm relying on the periods as they're compatible with the CSS syntax, unlike the commas.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #31, on August 2nd, 2011, 01:21 AM »
It's almost certainly the setLocale call that's been made which uses LC_ALL from what I remember, which means numbers will be formatted to the locale specified if possible.

Not sure how to fix that other than temporarily resetting the locale to the English default when entering the parser and resetting it back to the normal language on exit.
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
Re: New revs - Public comments
« Reply #32, on August 2nd, 2011, 07:04 AM »
Oh.

But this means there will be bugs in plenty other areas to come.
Are you sure that's necessary?

Drunken Clam

  • Drool, drool, drool....!
  • Posts: 154
Re: New revs - Public comments
« Reply #33, on August 2nd, 2011, 07:10 AM »
I love this thread - even if most of it sounds like 'Swahili' to me!

Great minds at work, thinking aloud. You two rock!  :cool:

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: New revs - Public comments
« Reply #34, on August 2nd, 2011, 07:25 AM »
There are dozens of topics like this one in private :P
I find myself a bit antisocial these days, especially on this topic. Meh. (Sorry Pete. bit ashamed.)
Re: New revs - Public comments
« Reply #35, on August 2nd, 2011, 08:45 AM »
Quote from Arantor on August 2nd, 2011, 01:21 AM
It's almost certainly the setLocale call that's been made which uses LC_ALL from what I remember, which means numbers will be formatted to the locale specified if possible.
Yes, that's what I thought. If I change it to LC_TIME, it starts working again.
Quote
Not sure how to fix that other than temporarily resetting the locale to the English default when entering the parser and resetting it back to the normal language on exit.
It's a possibility, but is there any reason for using LC_ALL really?

This changed was applied in rev 750. You added a centralized setlocale(LC_ALL) call (which includes LC_NUMERIC for the decimal separator), and removed the setlocale(LC_TIME) calls in Load.php and Subs-Media.php. So this basically breaks the 'normal' SMF behavior for localization.
I'm the first surprised because this didn't happen to me at all on the demo site. It only started happening on my second demo site, which I installed yesterday to test the importer. It's on the same server...!

So, I guess there are two possible solutions:

- setlocale(LC_TIME) in Load.php, instead of setlocale(LC_ALL). Or setlocale(LC_ALL & ~LC_NUMERIC) if that's even possible (I'm not sure LC_* is a bitwise thing.) I'd rather we use this one because at least it's going to be prooftested through SMF.

- $ex_locale = setlocale(LC_ALL, 0); setlocale(LC_ALL, 'en_US.utf8'); do our stuff; setlocale(LC_ALL, $ex_locale). That's what you suggest but I'm not sure it'll work flawlessly. It may break on some server types, etc... And it doesn't ensure it'll work in non-Subs-Cache pages.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #36, on August 2nd, 2011, 09:13 AM »
Quote
It's a possibility, but is there any reason for using LC_ALL really?
Yes: some versions of the locales database don't set everything properly when using just LC_TIME, going off the comments in the setLocale manual page. http://www.php.net/manual/en/function.setlocale.php#77795 mentions it for example.

Windows aside, which is buggy anyway and I wouldn't expect serious users to be running PHP on Windows as a consequence, setLocale is threaded, meaning that there are no outbound consequences for doing the switch as suggested; once it enters the CSS parser, it's going to remain there until it's done.

I'm happy to try it as suggested, especially since now I read up on it again, I don't see how we can make use of LC_COLLATE or LC_CTYPE because that has implications for users migrating from other systems (since those affect how strtolower is done and how string comparisons may be made, though equivalence checks shouldn't be affected) though some things like LC_MONETARY might be useful in places.

It's, unfortunately, a case of swings and roundabouts :/

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: New revs - Public comments
« Reply #37, on August 2nd, 2011, 10:33 AM »
Quote from Arantor on August 2nd, 2011, 09:13 AM
Yes: some versions of the locales database don't set everything properly when using just LC_TIME, going off the comments in the setLocale manual page. http://www.php.net/manual/en/function.setlocale.php#77795 mentions it for example.
Hmm... Only says "some versions of Windows". Never heard of that, really.
Quote
Windows aside, which is buggy anyway and I wouldn't expect serious users to be running PHP on Windows as a consequence, setLocale is threaded, meaning that there are no outbound consequences for doing the switch as suggested; once it enters the CSS parser, it's going to remain there until it's done.

I'm happy to try it as suggested,
Which solution then?

(I'm really pro-LC_TIME here. I don't see why we should deviate from the SMF implementation, apart from the fact that we're only executing setlocale once per page call now.)
Quote
especially since now I read up on it again, I don't see how we can make use of LC_COLLATE or LC_CTYPE because that has implications for users migrating from other systems (since those affect how strtolower is done and how string comparisons may be made, though equivalence checks shouldn't be affected) though some things like LC_MONETARY might be useful in places.
I don't think it's of much importance. Well, my opinion.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #38, on August 2nd, 2011, 10:38 AM »
Quote
Hmm... Only says "some versions of Windows". Never heard of that, really.
It's more the locales database than the OS version, to be precise.
Quote
(I'm really pro-LC_TIME here. I don't see why we should deviate from the SMF implementation, apart from the fact that we're only executing setlocale once per page call now.)
At the time I thought it would make it easier to internationalise through using LC_ALL since I couldn't see a good reason not to but you've found an excellent reason not to.

Considering also that the number format is not managed through the locale anyway, the only reason remaining that actually matters is the possible incompatibility - but even then... it hasn't bothered SMF that I could find, so run with LC_TIME.

Nao

  • Dadman with a boy
  • Posts: 16,079
Re: New revs - Public comments
« Reply #39, on August 2nd, 2011, 01:12 PM »
Quote from Arantor on August 2nd, 2011, 10:38 AM
At the time I thought it would make it easier to internationalise through using LC_ALL since I couldn't see a good reason not
And it made sense indeed.
Quote
to but you've found an excellent reason not to.
Really by chance.
Still, we're in a situation that reminds us that, sometimes, the amount of servers and configurations makes it hard to make software work everywhere without tweaking. Somehow it reminds me of my 'terrific' days of debugging Direct3D code to make it work on both ATI and NVIDIA cards... Ahhhh.
Quote
Considering also that the number format is not managed through the locale anyway, the only reason remaining that actually matters is the possible incompatibility - but even then... it hasn't bothered SMF that I could find, so run with LC_TIME.
I understand that LC_CTYPE could be a problem with case manipulation. We'll have to keep that in mind... And yes, it never seemed to matter in the SMF world.
Posted: August 2nd, 2011, 01:06 PM

Okay, with rev 904 I added the template hooks. They probably won't work out of the box though, but I figured if I didn't add them, we'd forget about them eventually. We can always remove them later. The point here is: how do we make it as easy as possible for modders and themers to add these template hooks? Should we have 'proper' functions in a separate file, should we load a file actually...?

Here's how I'm considering the approach:
- mod adds an add_hook('some_source_hook', my_function, my_file)
- in my_file, mod adds a template_something_subtemplate_before function that executes before template_something_subtemplate
- after executing some_source_hook, Wedge proceeds to load Something.template.php and execute the subtemplate, and then call the _before hook that's precisely in the my_file file.

Does it make sense, or should it be made easier?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #40, on August 2nd, 2011, 01:22 PM »
Quote
Still, we're in a situation that reminds us that, sometimes, the amount of servers and configurations makes it hard to make software work everywhere without tweaking.
Only last night I encountered a change in behaviour between my local PC and the server that my code was deployed onto; it was a schoolboy error on my part for forgetting it (call_user_func_array expects the callee to be specifying its parameters explicitly as references, rather than values, especially given that the caller was using references) - on my 5.2 setup it worked as expected, on a 5.3 box it broke quite interestingly.
Quote
I understand that LC_CTYPE could be a problem with case manipulation. We'll have to keep that in mind... And yes, it never seemed to matter in the SMF world.
The reason it doesn't matter in the SMF world is because so much of what goes on is DIY coding rather than letting PHP's own functions do the job. Number formatting, timezone offsets, time generally, etc.
Quote
Here's how I'm considering the approach:
Interesting. Having actually done something lately[1] that would be applicable under this discussion, let me step through what I did and why - then we'll see if it was the right decision for the case in point.

So, I needed something that would operate during the SD ticket load phase and then proceed to add a template in. Since I'm slightly lazy and had no desire to force yet another file to be loaded when the template was a very simple one, I just put the source and template in the same file.

I actually figure most mod authors would do the same if they could get away with it. Most mods don't bother trying to support all different themes, concerning themselves with template edits, or for bigger/independent stuff, a separate template file.

Embedding the template itself into the mod would prevent creating a separate file just for separation's sake because the point of MVC isn't about putting files randomly in different places to separate the code, it's a pattern: that you keep presentation and logic separate as far as possible, and even if you're mixing them in the same file, you're still logically separating them into separate functions; it would only be an issue semantically if you had just the template with the logic in it.

So, getting back to the point: I'd suggest we don't have it load files in template hooks. Any new template will require code to back it up anyway, which means you're going to be loading a file to engage the logic-powering hook anyway, and that new file can load a template if it wants, or it can embed the template if it wants. Then if the mod wants/needs theme specific changes for some random reason, it can be left to the mod author to make it happen if they so desire.
 1. A custom SD plugin, as it happened.
Re: New revs - Public comments
« Reply #41, on August 7th, 2011, 01:48 PM »
r913 is something I had been planning to do a bit back but what always stopped me is the type conversion because I wasn't sure what the upper bounds of anything were supposed to be.

It may be version specific but int(4) should do something different to tinyint(4) - in theory, an int(4) should consume at least twice the space of a tinyint(4) and potentially quadruple it; int defines a 4 byte integer, tinyint a 1 byte integer, but even if MySQL is a bit smarter, and assumes the number as a size of digits, you still have to allocate two bytes (i.e. a smallint) because a potentially-4 digit number is invariably going to consume more than one byte. (The reason tinyint defaults to 4 is because of the leading sign, e.g. -127)

I don't believe MySQL uses that value for type hinting though, because of the above fact that it could be unreliable, meaning that I think it's always going to allocate 4 byte integers for everything even if it's only a one byte column, so it might be worth glancing through the code sometime to see if we do actually need 4 byte ints everywhere, since I'm not sure.

In other news, at some point we need to make sure that the list of tables that we end up with is in the listed of reserved tables in the DB class(es).
Re: New revs - Public comments
« Reply #42, on August 17th, 2011, 04:42 PM »
Quote
@ Pete> I'm not sure it's semantically all right to use a theme menu string in a member menu... ^^;
Huh? Which one did I do that for then?
Re: New revs - Public comments
« Reply #43, on August 18th, 2011, 03:30 PM »
Quote from Nao/Gilles on August 18th, 2011, 03:21 PM
Wow, we committed in the same minute! Did you get a conflict? :P
Nope, because I updated before committing and got your changes first, but mine happened to be nowhere near yours.

Incidentally, at some point recently we broke the installer somehow, it never gets onto step 2 :/
Posted: August 18th, 2011, 03:25 PM

More specifically, if there's any kind of error that would force it to fall out of DatabaseSettings(), it's just going back to the start rather than actually showing an error :/
Re: New revs - Public comments
« Reply #44, on August 18th, 2011, 03:41 PM »
OK, so this is really, really weird.

Whatever happens, it's forcing a 302 redirect back round to the installer >_<