« Need opinions on a variable name...
More constant names to consider.
Stupid git? »

Nao

  • Dadman with a boy
  • Posts: 16,082
More constant names to consider.
« on December 13th, 2013, 12:33 PM »
After the ASSETS/IMAGES thing (which is yet undetermined, I'm waiting for a few more votes), I'd like to ask whether you think my terminology for new constants is all right...

'theme_url' => TEMPLATES (because eventually, the folder will only have templates in it)
'theme_dir' => TEMPLATES_DIR
'images_url' => IMAGES or ASSETS (see other topic)
'images_dir' => IMAGES_DIR or ASSETS_DIR (the variable doesn't actually exist, it's for consistency reasons.)
$scripturl => SCRIPT[1]
$boardurl => BOARD (this might be a problem, as $board is already a global var, and unrelated to it...)
$boarddir => BOARD_DIR (underscore might be a problem..? ElkArte uses BOARDDIR)
$sourcedir => SOURCES_DIR (again, underscore, too much?)[2]
$pluginsdir => PLUGINS_DIR
$pluginsurl => PLUGINS
$cachedir => CACHE_DIR (I'd like CACHEDIR better, honestly, but I can't fathom a TEMPLATESDIR either, and I want to harmonize everything...)

Also, $cssdir => CSS_DIR and $jsdir => JS_DIR, but these are not used in a lot of places ($jsdir is really only used in Subs-Cache), so I'm thinking maybe I'll just pass on the constants. Or maybe later.

I'm not going to put these for a vote (too complicated), but I'd like opinions on the naming scheme. Should I do with or without the underscore? Maybe go for a lowercase version of constants? (Nah, it's actually parsed faster with uppercase, yes I did benchmarks.) Maybe move 'DIR' at the beginning of the constant names? Maybe force adding '_URL' on $...url variables?

$boarddir, $boardurl and $sourcedir are all used between 170 and 250 times across the entire codebase, so it makes a lot of sense to use constants for these, so I don't have to global these. $scripturl is still used 500+ times, and that's even after turning hundreds of these into <URL> HTML. Wow... Well, this one definitely warrants a constant, once I can ensure the variable is not changed during the page's lifetime.

Opinions welcome -- really! Even if you're not a programmer.
 1. Also, there's the alternative of using <URL> in your HTML. Should I then name the constant URL..?
 2. I think the plural is important because the folder is named that way.

Powerbob

  • Posts: 151
My Alpha test site; http://www.pplb.net/wedge/index.php

emanuele

  • Posts: 125
Re: More constant names to consider.
« Reply #2, on December 14th, 2013, 11:52 AM »
Do you already have some constants?
If so I'd say follow the same naming scheme, if you don't and you are deciding it now... mmm... it mostly depends on your preferences I think.
I don't like underscores much ..well, that's not true, my mind changes every two lines of code I write LOL

Anyway, as long as the name is not confusing, I'd say be consistent with what you pick and live with it. ;D

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: More constant names to consider.
« Reply #3, on December 14th, 2013, 01:50 PM »
Quote from emanuele on December 14th, 2013, 11:52 AM
Do you already have some constants?
No. Well, I do now... But it's not pushed to the repo, that is.
Quote from emanuele on December 14th, 2013, 11:52 AM
If so I'd say follow the same naming scheme, if you don't and you are deciding it now... mmm... it mostly depends on your preferences I think.
My preference is for we::$scripturl and things like that. I find '::' to be so leet. No, seriously, the problem with :: is that it's really, really much slower, and can't be realistically used everywhere (only on things that run from time to time.) That's why I'm also considering changing we::$id to MID or UID (member/user, still haven't settled on a name. What do you prefer? :P)
Quote from emanuele on December 14th, 2013, 11:52 AM
I don't like underscores much ..well, that's not true, my mind changes every two lines of code I write LOL
(See my topic at Elk.)
Quote from emanuele on December 14th, 2013, 11:52 AM
Anyway, as long as the name is not confusing, I'd say be consistent with what you pick and live with it. ;D
Well, that's the idea!

emanuele

  • Posts: 125
Re: More constant names to consider.
« Reply #4, on December 14th, 2013, 02:30 PM »
Member is usually more "friendly" I think, user more a kind of relation like provider/client.
But for example in the Italian translation are both mixed up because member doesn't really fit in certain situations (and also because every time I read "member" in Italian without a context I can't stop thinking about its second meaning... lol).

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: More constant names to consider.
« Reply #5, on December 14th, 2013, 03:42 PM »
I wasn't about determining if member or user should be used, more about MID or UID, that's all. ;) Member, in SMF terminology, is a logged in user. And a user is either a guest, or a member. $user_info['id'] determines its status, but it's still a 'user'.
I guess MID works because I can always do if (!MID) to determine if the user is a guest -- it's less self-explained than if (we::$is_guest), but it's also a bit faster to execute.

One of my harmonization problems with these constant names is really SOURCES_DIR. Considering that there's no matching SOURCES (you don't need to access that folder by URL), it would make more sense to use SOURCES for the path, but then it's in conflict with all other constants, which use _DIR to denote a path. This is really the only reason stopping me from adopting that naming scheme, although I could also just as well keep using $sourcedir everywhere. I don't know. It's still used 231 times in Wedge, but mostly in non-Sources and non-Templates files.
Re: More constant names to consider.
« Reply #6, on December 14th, 2013, 07:53 PM »
Regarding MID, I looked into something I feared -- the possibility that we::$id was being reset after being set once.

It happens, AFAIK, at least twice:

- DoLogin function sets it after you're logged in. My gut feeling is that at the very worst, I could add an extra redirection to ensure MID is set up properly from the database.

- Security code: the ban code ends its run by stripping the user of any rights they have, AND their we::$id variable (it's set up that way in Elk and SMF: $user_info['id'] = 0). I'm thinking that it could be possible to just leave that aside -- we::$is_guest should be enough to guarantee that a page will become inaccessible to the user, and if some page 'only' does a test on we::$id/MID to give access or not to a feature, then it's doing things wrongly, and should be fixed.

What do you think..? Worth it or not?
we::$id is used about 750 times across the entire codebase. I don't want to use a $mid global as replacement, either. And $user_info['id'], ah ah, no thank you, been there done that...
Re: More constant names to consider.
« Reply #7, on December 15th, 2013, 09:56 AM »
I took another look at this (sorry, was busy yesterday). we::$id really is okay to change into MID because the DoLogin function only ends with a redirectexit (meaning I only have to be careful about uses of we::$id within DoLogin and called functions), and the ban code ends (obviously) with a fatal_error, so I only have to check within the ban code and Error file. This should be easy, i.e. only use we::$id within these areas, and use MID across the rest of the codebase. (There's also obExit to take into account, though. But that's all right. we::$id is only used once in ob_sessrewrite, for instance.)
Then, everybody's happy.

Also renamed $boardurl and $boarddir to ROOT and ROOT_DIR, instead of BOARD and BOARD_DIR. Why?
Why, you say?
Because a Wedge website is not a 'board'. It's not ALWAYS a 'board'. So I have something to say about $boardurl meaning 'URL to my website'.
So, that's ROOT_URL from now on. I could have used SITE_URL, but I find ROOT to be more effective in terms of 'relativity' to my ears.

emanuele

  • Posts: 125
Re: More constant names to consider.
« Reply #8, on December 15th, 2013, 10:56 AM »
I would avoid duplication (e.g. if you already have the id in we::$id, it's useless to have it in a constant too). Also, yes the user id is going to change at certain points during the execution, so I would really avoid to make a constant out of it simply because it is not constant.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: More constant names to consider.
« Reply #9, on December 15th, 2013, 11:17 AM »
Quote from emanuele on December 15th, 2013, 10:56 AM
I would avoid duplication (e.g. if you already have the id in we::$id, it's useless to have it in a constant too).
The goal with we::$id was to avoid globaling $user_info. (Also, I wanted to harmonize $user_info and $context['user'], which were a real mess to begin with!)
However, testings at the time showed that calling a static class variable was approx. twice slower than calling a simple globaled variable. I could have done $GLOBALS['user_info']['id'] which is only ~10% slower than using the global, but that's ridiculous.
I looked into constants at the time, but had too many issues with redefined variables, and didn't want to look into it, so I went for the object thing. I'm glad I did, but still, wasn't happy with the performance impact.
This came back to me when I got rid of themes and decided to directly go for constants for the newly created path pointers.
And then I came to we::$id, looked into it again, much harder this time, and as you see... It's very doable, really. In fact, I'd be hard-pressed to find any plausible reason not to do it right now. Yes, sure, we::$id is going to co-exist with MID, but you know what..? That's fine. Because no one but me ever complained about $context['user'] being a duplicate subset of $user_info, I see no reason why people should complain that there's a duplicate variable in memory. I'll probably even give up on it; after all, the only issue I could find was with the use of we::$id once in ob_sessrewrite, and that use is particular to linking to one's own profile. This only comes up if a particular setting is enabled in the forum, AND you're banned. And you know what..? If you're fully banned, and you can't access your own profile, then what does it matter that it's properly linked to, since all SMF (mainline and forks) will always link to it anyway, but prevent you from accessing it after the click? Wedge prevents that, but in the case of a full ban, it won't do it for your own profile. Which, really, would be AFAIK the only issue that happens if I immediately removed we::$id altogether.
There are battles to be fought, and wars to be won. This? It's not even a quarrel.
Quote from emanuele on December 15th, 2013, 10:56 AM
Also, yes the user id is going to change at certain points during the execution, so I would really avoid to make a constant out of it simply because it is not constant.
It only has two reasons to change AFAIK, and it's (1) logging in, (2) being banned. Being banned is easy. Logging in, it shouldn't matter either, because you're being redirected anyway (so that you can take full benefit of your logged status from the beginning of the page load process), and you're only benefiting the user ID from the moment logged status is accepted, to the moment you're being redirected. I'm sure I can follow the entire code path in five minutes, and determine that the user ID is never actually used once in that path, and setting it was useless to begin with.

And then off we go, bye bye $user_info['id']. It's valid for ElkArte just the same as Wedge, really.
Re: More constant names to consider.
« Reply #10, on December 15th, 2013, 05:18 PM »
FYI. MID is implemented and working. Tried logging out and in, fixed bugs, and voilà. :)

I'm close to being able to commit!
« Need opinions on a variable name...
Stupid git? »