New revs - Public comments

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #225, on October 17th, 2011, 03:52 PM »
Quote
I believe there are a few items remaining from the OpenID code... Not a lot really. Just a few lines.
Just search for 'authenticate' (single word) and 'auth_method'. Because the authentication method is only 'password' now, the variable is useless.
Odd, I searched for 'authenticate' and 'auth_method' but I guess I somehow missed those :/
Quote
On another note. I'm looking into loadMemberData() and it reminds me of the discussions about queries and making them hookable.
How about turning the stuff into something like this...?
I thought we were going to use something like Dragooon's wesqlQuery structure to make those hookable?
Quote
I'm thinking we should be adding a param to the function, telling Wedge not to bother with the 'heavy' processing and just fill in the blanks. I mean, if you have 50 users in the Memberlist, that means we potentially parse 50 signatures... for nothing!
Hmm. What I'd rather do is do the minimal stuff (user name/profile/group) and optionally then add extra stuff if needed, e.g. array('avatar', 'signature', 'custom fields'). Then I'd feel more comfortable extending loadMemberContext into general use (see in a moment).
Quote
And while we're at it -- I think it'd make sense to also have a param saying whether we want the member link to use group colors. They're not used in many areas and I'm not sure why. Actually, it could even make sense to always use the group colors. They're loaded at this point... Even in the topic pages it would make sense to show group colors (although I'm not sure about that.)
I've been wanting to add group colours for a while, just never got around to it. The real problem is that a surprising number of places do not reference loadMemberData/loadMemberContext for getting things like names, but do a raw join from whatever table into the members table to get the name. PMs is by far the worst offender for this from memory.

As I said above, it seems to me that making loadMemberContext more generally used would be advantageous, but at the same time that comes with an overhead in performance and memory. If it were better able to be segmented (both in lMC and lMD), as to what information you're looking for, that can be avoided.
Quote
- $txt['profile_of'] is used in things like $memberContext as a title. I'd rather have it say just 'View profile' than 'View profile of...' followed by a name that is *already* in the link... Totally silly. Plus, we already have $txt['profile_of_username'] which is correctly handled by a more logical sprintf.
Opinions?
Works for me. Is there anywhere $txt['profile_of'] is used as a link description where it wouldn't have the name available to it (and so wouldn't make sense if used that way)?
Quote
- $txt['time_am'] and $txt['time_pm']...
Crap, I thought I'd nailed all instances, guess I didn't search so thoroughly :/
Quote
I personally always type it in lowercase, and actually earlier versions of SMF used am and pm in lowercase, too, in timeformat()...)
'am' and 'pm' is probably nicer looking and more consistent with what everyone else seems to use.
Quote
I've written a more 'accurate' hack for the year removal. Please review it and tell me if that's okay. ($year_shortcut is a static array, while $format is simply =& $user_info['time_format'])
Looks fine to me.
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,082
Re: New revs - Public comments
« Reply #226, on October 17th, 2011, 04:59 PM »
Quote from Arantor on October 17th, 2011, 03:52 PM
Odd, I searched for 'authenticate' and 'auth_method' but I guess I somehow missed those :/
Do you want to remove them, or me?
Quote
I thought we were going to use something like Dragooon's wesqlQuery structure to make those hookable?
Yes/no. I have no idea, this hasn't been discussed in a while AFAIK. I'm not well up to date on what Dragooon is up to -- sorry. What I know, though, is that we can always change it later... but if we don't implement a generic hook for queries by the first release, at least we'll have a hookable member query. It's not particularly hard to retool into something generic anyway...

Still, I'd like to know where I'm going to put the post-query hook :P
Right now it's set to just after the query...
Quote
Quote
I'm thinking we should be adding a param to the function, telling Wedge not to bother with the 'heavy' processing and just fill in the blanks. I mean, if you have 50 users in the Memberlist, that means we potentially parse 50 signatures... for nothing!
Hmm. What I'd rather do is do the minimal stuff (user name/profile/group) and optionally then add extra stuff if needed, e.g. array('avatar', 'signature', 'custom fields'). Then I'd feel more comfortable extending loadMemberContext into general use (see in a moment).
Quote
I've been wanting to add group colours for a while, just never got around to it. The real problem is that a surprising number of places do not reference loadMemberData/loadMemberContext for getting things like names, but do a raw join from whatever table into the members table to get the name. PMs is by far the worst offender for this from memory.
Not a big problem, I mean, we can always update them later... Or just don't bother.

PM example: the PM sender's name is specified with the group color. The PM recipient list isn't. Hmm...

Oh, and PT Sans + red for admin group = terribly ugly. Need to do something about that... Red is suitable for small Verdana fonts, but as soon as you make it a big font, it tends to catch attention too much.
Quote
As I said above, it seems to me that making loadMemberContext more generally used would be advantageous, but at the same time that comes with an overhead in performance and memory.
Definitely...
Quote
If it were better able to be segmented (both in lMC and lMD), as to what information you're looking for, that can be avoided.
You mean, like specifying the list of fields we want...? Or maybe a 'thematic' load or something? Like, $set is too restrictive... It'd probably benefit from being turned into one big array where items are added individually after tests are performed (if (isset($set['sig']) $select_columns[] = 'mem.signature'...?)
And then in lMC we test whether said field is present before we process it.

I'd tend to say that lMD and lMC are two of the most important functions in Wedge, and they should be handled with special care.
Quote
Works for me. Is there anywhere $txt['profile_of'] is used as a link description where it wouldn't have the name available to it (and so wouldn't make sense if used that way)?
Nope... It's always the same. <a title="View profile of Arantor">Arantor</a>. (Plus, as you can see, it's perfectly natural English... ::))
Removed in 1116.
Quote
Crap, I thought I'd nailed all instances, guess I didn't search so thoroughly :/
Don't sweat it, that's what I'm here for...! It may take me a long time to go through your code changes but I always try to catch this kind of stuff... And I hope you do the same for me :P Even though I tend to triple-check my commits, I'm never sure I'm doing the right thing.
Quote
'am' and 'pm' is probably nicer looking and more consistent with what everyone else seems to use.
Done in 1115, but what about %p..? We can't really transform AM to am magically... Unless we specifically try to do so...
Quote
Quote
I've written a more 'accurate' hack for the year removal. Please review it and tell me if that's okay. ($year_shortcut is a static array, while $format is simply =& $user_info['time_format'])
Looks fine to me.
Changed it a bit in the meantime, but the idea stays the same.

I think I'm gonna go with the $txt solution where I provide the full year shortcut in the language files, and then if user doesn't use the default txt, we'll fall back to that test.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #227, on October 17th, 2011, 05:12 PM »
Quote
Do you want to remove them, or me?
They can all go as far as I'm concerned. Even as a plugin I don't see OpenID as *replacing* the current login, but working alongside it. If FB Connect can be done as a plugin, so can OpenID, and FBC doesn't need the auth method setup AFAIK.
Quote
Yes/no. I have no idea, this hasn't been discussed in a while AFAIK.
Well, for the most part I was pretty happy to use it, and I was curious to know what you thought of it because it would require rewriting the more important queries to make them hookable directly.
Quote
Still, I'd like to know where I'm going to put the post-query hook
What, exactly, is it for? That's the question to ask. If it's for pushing things into $user_profile, it shouldn't be needed because $user_profile just gets the entire row as-is so providing it's in the query, it's pulled into PHP-land.
Quote
Not a big problem, I mean, we can always update them later... Or just don't bother.

PM example: the PM sender's name is specified with the group color. The PM recipient list isn't. Hmm...
I understand the logic of why it was done that way, it's primarily to avoid an extra query when that query (probably) isn't necessary, but it doesn't make it flexible. I'd almost be inclined to add an option in the admin panel for 'use member group colours', and make everything actually use that (doing the relevant join(s) to the membergroups table as appropriate). If nothing else, it's one more that's highly desirable that really, to me, should be in core.
Quote
Oh, and PT Sans + red for admin group = terribly ugly. Need to do something about that... Red is suitable for small Verdana fonts, but as soon as you make it a big font, it tends to catch attention too much.
*nods* I have no preference, but whatever is used, the rank images should adhere to it.
Quote
You mean, like specifying the list of fields we want...? Or maybe a 'thematic' load or something? Like, $set is too restrictive... It'd probably benefit from being turned into one big array where items are added individually after tests are performed (if (isset($set['sig']) $select_columns[] = 'mem.signature'...?)
And then in lMC we test whether said field is present before we process it.

I'd tend to say that lMD and lMC are two of the most important functions in Wedge, and they should be handled with special care.
Pretty much, at least. 'minimal', 'normal' and 'profile' are useful but not really *that* useful. Normal is used far more often than it's really needed to be, I find.

Now, if it were done thematically, you could really tighten up what's loaded when, though it would make bolting things into that query a lot more complicated.

I think figuring out where minimal and profile are used would be the first step, and making minimal a shade bigger to allow for things like member-group colour (and then making it used consistently!) would help - though I can't get the idea out of my head of splitting it into three to cope with the varied uses therein; places like PMs only need the minimal version, places like Display need the modest version and places like Profile need everything.
Quote
Removed in 1116.
Awesome.
Quote
Don't sweat it, that's what I'm here for...! It may take me a long time to go through your code changes but I always try to catch this kind of stuff... And I hope you do the same for me
I try to go through them, but I still miss stuff occasionally. Before our gold release, I'd like us to go through the language strings - at a minimum - to look for stray things that should have been removed.
Quote
Done in 1115, but what about %p..? We can't really transform AM to am magically... Unless we specifically try to do so...
Hrm. This smells like a partial reversion to me :/ If the format contains %p, I think we'll have to reinject am/pm instead.
Quote
I think I'm gonna go with the $txt solution where I provide the full year shortcut in the language files, and then if user doesn't use the default txt, we'll fall back to that test.
Works for me :)

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #228, on October 17th, 2011, 06:53 PM »
Quote from Arantor on October 17th, 2011, 05:12 PM
They can all go as far as I'm concerned. Even as a plugin I don't see OpenID as *replacing* the current login, but working alongside it. If FB Connect can be done as a plugin, so can OpenID, and FBC doesn't need the auth method setup AFAIK.
Yeah... Makes sense. But considering the user won't have to choose a password (because all of this will be handled by OpenID or Facebook...), there are a few things that will still need to be added or removed... e.g. the password reminder won't work.
Quote
Quote
Yes/no. I have no idea, this hasn't been discussed in a while AFAIK.
Well, for the most part I was pretty happy to use it, and I was curious to know what you thought of it because it would require rewriting the more important queries to make them hookable directly.
My opinion...? Too complicated. I read through the topic and then I forgot it all.
I suppose that given we won't use this in all queries, it's okay to use it. But I'm not the one who'll implement it ;)
I simply did what I felt I needed to do with lMD while I was studying the changes after you removed OpenID... Now if you could just tell me whether you want me to commit it or not, I promise I won't mind either way. I'll just post my diff here for you to use if it inspires you.
Quote
What, exactly, is it for? That's the question to ask. If it's for pushing things into $user_profile, it shouldn't be needed because $user_profile just gets the entire row as-is so providing it's in the query, it's pulled into PHP-land.
Yeah, I suppose you're right... I did notice the $row stuff, but I thought that plugins might wanna change some settings for the rest of lMD, such as removing users from $users (although I guess it could be done by adding an INNER JOIN to filter people out directly from within the query), or processing user names and changing the value of $is_name accordingly. Hmm... Yeah it's a bit overdone. So, no post-processing hook.
Quote
I understand the logic of why it was done that way, it's primarily to avoid an extra query when that query (probably) isn't necessary,
An extra query are you sure? I have a feeling everything can be done with a LEFT JOIN on the membergroups table...
Quote
but it doesn't make it flexible. I'd almost be inclined to add an option in the admin panel for 'use member group colours', and make everything actually use that (doing the relevant join(s) to the membergroups table as appropriate). If nothing else, it's one more that's highly desirable that really, to me, should be in core.
Certainly something we should explore.

Other opinions on this...?
Quote
Now, if it were done thematically, you could really tighten up what's loaded when, though it would make bolting things into that query a lot more complicated.
(Hence my custom query to make it easier to add elements according to a variable test.)
Quote
I think figuring out where minimal and profile are used would be the first step,
From a quick look...
- normal: used nowhere.
- minimal: used in ManageErrors to pull only the real_name. And... that's all. Sigh!!
- profile: used in PersonalMessages.php when editing PM settings in the profile area, for use in the subsequently called saveProfileFields(), and in Profile and Profile-Modify, as expected.
- no param (i.e. normal):
* used in SSI (ssi_queryMembers), Display (load post authors), Feed (getXmlProfile), Memberlist (load member list -- serious candidate for 'minimal'), PersonalMessage (used in MessageFolder to load PM authors, MessageSearch2 for the same, MessagePost, and drafts. Used in Aeva Media's admin page to list moderators (strong candidate for deletion.)
* Also used in Profile-Actions.php only to retrieve the list of groups of a member for use in subscriptions(). Candidate for 'minimal', or even 'custom' by specifying only the membergroup data...
* Used in Search2 to get authors for found posts. Used in Who.php for the Who's Online list. Another candidate for 'minimal'...
* Used in Aeva Media to get comment authors, and then immediately after that to get the item author's data... Woot, nice oversight. We definitely should merge the two of these...

minimal is DEFINITELY in dire need of being reused elsewhere. We can safely add more data (who cares about ManageErrors performance!), and do things like making sure we don't retrieve the signature, etc.
Quote
and making minimal a shade bigger to allow for things like member-group colour (and then making it used consistently!) would help
I was thinking... Maybe, *maybe* we could 'simply' have ob_sessrewrite look through the list of links and color them accordingly... i.e. it gets the profile ID from the link, then fetches a list of users and groups. We'd just need to get a (cacheable!) list of membergroups and associated IDs. Then we can build a list of colors associated with member IDs. And voilà. We simply inject the needed CSS at rewrite time... :)

Seems like a very good idea to me. Plus, we can get rid of the existing custom colors, and we have an easy way to simply 'skip' the whole color thing (of course this option should be part of the option that allows to show membergroups in the info center... Or, we simply hardcode colors for these, and make sure ob_sessrewrite won't reparse them.)
Quote
I try to go through them, but I still miss stuff occasionally. Before our gold release, I'd like us to go through the language strings - at a minimum - to look for stray things that should have been removed.
I think there will be some, yeah... It's bound to happen. But I'd rather have too many strings than not enough :P
Quote
Quote
Done in 1115, but what about %p..? We can't really transform AM to am magically... Unless we specifically try to do so...
Hrm. This smells like a partial reversion to me :/ If the format contains %p, I think we'll have to reinject am/pm instead.
I'm not sure I understand. AFAIK SMF has always used %p to show the am/pm bit... Even Wedge uses %H:%I %p everywhere (in English). Maybe you mixed it up with %P, which shows a *lowercase* version, but isn't Mac OS-compatible... (I would have used that instead, otherwise. Heck...)

📎 lmd.rar - 1.78 kB, downloaded 786 times.


TE

  • Posts: 286
Re: New revs - Public comments
« Reply #229, on October 17th, 2011, 07:38 PM »
Quote
Yeah... Makes sense. But considering the user won't have to choose a password (because all of this will be handled by OpenID or Facebook...), there are a few things that will still need to be added or removed... e.g. the password reminder won't work.
That's indeed a very important point. there is the password reminder, the admin-center password verification and the profile page (account settings: password verification while changing important account data & password change in general)

I have some plans for a universal bridge  (webserver authentication, LDAP, external databases), thus I'd need a proper solution for these things...
Thorsten "TE" Eurich - Former SMF Developer & Converters Guru

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #230, on October 17th, 2011, 08:21 PM »
Quote from TE on October 17th, 2011, 07:38 PM
That's indeed a very important point. there is the password reminder, the admin-center password verification
I suspect that no one in their right mind would use Facebook to register/log into a forum they're administrating...
Quote
and the profile page (account settings: password verification while changing important account data & password change in general)
Password change would be disabled if no password is recorded in the first place. :)
Quote
I have some plans for a universal bridge  (webserver authentication, LDAP, external databases), thus I'd need a proper solution for these things...
Ah, LDAP... I've never understood what this is about ;)

spoogs

  • Posts: 417
Re: New revs - Public comments
« Reply #231, on October 17th, 2011, 08:31 PM »
I use facebook to login to a forum I admin. I use SA's facebook mod for SMF which allows creating a forum username and password when registering... it is done this way so if there are any issues with facebook (which there has been in the past.. the reason this feature was requested and added) a user can still login into the forum using their user name and pass. You cannot however use your facebook info to access the ACP.
Stick a fork in it SMF

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #232, on October 17th, 2011, 09:50 PM »
Allowing a third party login system to be able to admin the forum is a security risk.

But all the third party logins should really create a true forum account, in which case whatever auth method can bootstrap the reminder process; OpenID had its own reminder page to remind you what OpenID login you used.

LDAP is something I thought about actually but even then you'd still authenticate against the LDAP server (eg Windows Server Active Directory) then create a forum account if necessary as the old LDAP bridge did.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #233, on October 17th, 2011, 11:26 PM »
Pete, there's a long post above for you :P

(Okay, okay, I've got one waiting for me, too...)

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #234, on October 18th, 2011, 12:13 AM »
OK, so for the rest of the stuff...
Quote
Yeah... Makes sense. But considering the user won't have to choose a password (because all of this will be handled by OpenID or Facebook...), there are a few things that will still need to be added or removed... e.g. the password reminder won't work.
See above. The reminder functionality should be rediverted to however the plugin wants to support it.
Quote
My opinion...? Too complicated. I read through the topic and then I forgot it all.
I suppose that given we won't use this in all queries, it's okay to use it. But I'm not the one who'll implement it
Heh, that's really why I wanted your input since it would require converting queries and 'big' queries would need this treatment later.
Quote
An extra query are you sure? I have a feeling everything can be done with a LEFT JOIN on the membergroups table...
That's precisely my point. There are two routes: bolt on the left join on top of everything, every single place where member names are pulled directly (and trust me, there's a lot), or route it through loadMemberData which is an extra query.

Ideally everything should have gone through lMD/lMC for consistency but it's not done that way for performance.
Quote
I was thinking... Maybe, *maybe* we could 'simply' have ob_sessrewrite look through the list of links and color them accordingly... i.e. it gets the profile ID from the link, then fetches a list of users and groups. We'd just need to get a (cacheable!) list of membergroups and associated IDs. Then we can build a list of colors associated with member IDs. And voilà. We simply inject the needed CSS at rewrite time...
Works for me. But note that it's essentially the same query that I mentioned above, just run at a different time.

There are some interesting side-effects to doing that in the replace hook to beware of. Firstly, we'd have to disengage it if XML output is being used. Secondly, if there's a link in the middle of a post or something to someone's profile, that's going to be hit too.

Also, I'm wondering whether it wouldn't be cleaner[1] to put the different replace processes in their own buffer, to be called at the appropriate time. Just for the sake of putting code together and making it easier to follow (as opposed to one monolith function that makes it harder to understand, especially since its original purpose is slowly being outweighed by all its extra work)
Quote
- profile: used in PersonalMessages.php when editing PM settings in the profile area, for use in the subsequently called saveProfileFields(), and in Profile and Profile-Modify, as expected.
*nods* Profiles do actually need everything.
Quote
* used in SSI (ssi_queryMembers), Display (load post authors), Feed (getXmlProfile), Memberlist (load member list -- serious candidate for 'minimal'), PersonalMessage (used in MessageFolder to load PM authors, MessageSearch2 for the same, MessagePost, and drafts. Used in Aeva Media's admin page to list moderators (strong candidate for deletion.)
SSI use makes sense, though it could become a 'minimal' use without too much effort IIRC.

Display - naturally needs to be pretty thorough given what's displayed.

Feed - should be minimal, right?

Memberlist - it sort of depends what we put in the memberlist, and whether we add things like custom profile fields and avatars. (I've been meaning to add an option to CPFs to display in the memberlist, but I keep also meaning to think about doing something with its layout and making it less sterile)

PM - it uses lMD? I'm surprised, I'd actually thought there wasn't a use within PMs to do so, especially given how many other places within the PMs *don't* use it.[2]

Post I can understand for displaying the form back to users, but I don't remember it in drafts. There must have been a reason, very likely to find usernames, but I don't remember.
Quote
* Used in Search2 to get authors for found posts. Used in Who.php for the Who's Online list. Another candidate for 'minimal'...
Yup, though it's nice having the member's name in the right colour there as it should currently be.
Quote
I think there will be some, yeah... It's bound to happen. But I'd rather have too many strings than not enough
Sure, but I'd rather have the *right* strings for the job, even if that means adding or stripping some.
Quote
I'm not sure I understand. AFAIK SMF has always used %p to show the am/pm bit... Even Wedge uses %H:%I %p everywhere (in English). Maybe you mixed it up with %P, which shows a *lowercase* version, but isn't Mac OS-compatible... (I would have used that instead, otherwise. Heck...)
Hmm. The point I was getting at is that I didn't realise there was a %P for lowercase, and that I assumed %p was the only one out there, so that if it were found and we wanted to use lowercase, we'd have to manually replace - as was originally done by SMF...
 1. It won't be faster, and it probably will use more memory, though.
 2. I once wrote a 'slash through banned members' mod, which naturally means extending the places where a user's name is used to pull is_activated, plus obviously changing the displayed name. I recall there being 20+ changes I had to make to PersonalMessage to achieve this.

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
Re: New revs - Public comments
« Reply #235, on October 18th, 2011, 12:54 AM »
Quote
On another note. I'm looking into loadMemberData() and it reminds me of the discussions about queries and making them hookable.
How about turning the stuff into something like this...?
That looks familiar... :whistle:
Quote
I thought we were going to use something like Dragooon's wesqlQuery structure to make those hookable?
But everything should be converted to arrays like that first.
A confident man keeps quiet.whereas a frightened man keeps talking, hiding his fear.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #236, on October 18th, 2011, 01:34 AM »
Quote
That looks familiar...
There are only so many ways you can make that beast hookable in its present form. Hardly surprising that both we and folks on sm.org were discussing it (theoretically independently) at the same time. Had I seen discussion there about bootstrapping a query object around it like we already talked about (and publicly), then I'd suggest all was not well in the garden of plenty. But hooking it conventionally... not so amazing.

TE

  • Posts: 286
Re: New revs - Public comments
« Reply #237, on October 18th, 2011, 07:28 AM »
Quote from Nao on October 17th, 2011, 08:21 PM
I suspect that no one in their right mind would use Facebook to register/log into a forum they're administrating...
why not? We have implemented a smartcard authentication in our company, thus my account has been validated during Windows logon to our Active Directory. All following systems (e.g. a webserver) are using a passthrough authentication via kerberos ticket / ntlm.

The backend webservers know me from my kerberos ticket ($_SERVER['PHP_AUTH_USER'] is defined) but I don't have a password, therefore $_SERVER['PHP_AUTH_PW'] isn't set. Sure, I can generate some kind of random password but that's against the idea of SSO (SingleSignOn).
Quote from Nao on October 17th, 2011, 08:21 PM
Ah, LDAP... I've never understood what this is about ;)
An LDAP-Directory isn't  a miracle, just a "database" with user /account information designed for central authentication.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #238, on October 18th, 2011, 08:56 AM »
Authenticating to a specific third party under your control is one thing, authenticating to a separate third party that you have absolutely no control over is something else.

The former is one place where I can see the logic, though surely all that's needed in that case is to turn over authentication over to the separate backend and if authenticated through that method, silently turn on 'disable admin security' (but only for the life of that page)?

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #239, on October 18th, 2011, 09:10 AM »
Quote from Arantor on October 18th, 2011, 12:13 AM
Heh, that's really why I wanted your input since it would require converting queries and 'big' queries would need this treatment later.
Well, if you and Dragooon are into it, then you should do it... I don't have the perfect answer to everything ;)
Quote
That's precisely my point. There are two routes: bolt on the left join on top of everything, every single place where member names are pulled directly (and trust me, there's a lot), or route it through loadMemberData which is an extra query.
Yeah...
Or, as I said: select m.id_member, g.online_color from members as m, membergroups as g where m.id_group = g.id_group
Something like that... (?)
Then get the entire list of people, push it into the cache for a few minutes, and use that cache internally to associate people with a color. It might represent a large cache file but if we ensure the option can be disabled, it's okay I guess... (?)
Quote
Works for me. But note that it's essentially the same query that I mentioned above, just run at a different time.
Sure... And in a cacheable fashion.
Quote
There are some interesting side-effects to doing that in the replace hook to beware of. Firstly, we'd have to disengage it if XML output is being used.
Why? I can't think of an example where adding a class to an anchor would be detrimental to XML functionality...?
Quote
Secondly, if there's a link in the middle of a post or something to someone's profile, that's going to be hit too.
Hmm yeah, I suppose... Although it's unlikely to happen.
But we could look through the entire anchor tag and reject it if it has bbc_ in it or something...
Quote
Also, I'm wondering whether it wouldn't be cleaner[1] to put the different replace processes in their own buffer, to be called at the appropriate time.
 1. It won't be faster, and it probably will use more memory, though.
Definitely cleaner. But easier to understand? I don't know, I've never been much into 'nested' output buffers, they never made much of a sense to me, I'd rather have it all in the same place...
Quote
Just for the sake of putting code together and making it easier to follow (as opposed to one monolith function that makes it harder to understand, especially since its original purpose is slowly being outweighed by all its extra work)
Well, we could simply turn these into sub-functions and call them all from ob_sessrewrite...
Quote
Feed - should be minimal, right?
Or 'custom', in my mind.
Quote
Memberlist - it sort of depends what we put in the memberlist, and whether we add things like custom profile fields and avatars. (I've been meaning to add an option to CPFs to display in the memberlist, but I keep also meaning to think about doing something with its layout and making it less sterile)
Definitely a 'custom' candidate then...
Quote
PM - it uses lMD? I'm surprised, I'd actually thought there wasn't a use within PMs to do so, especially given how many other places within the PMs *don't* use it.
It only uses lMD when you're setting PM options in your profile...
Quote
I once wrote a 'slash through banned members' mod, which naturally means extending the places where a user's name is used to pull is_activated, plus obviously changing the displayed name. I recall there being 20+ changes I had to make to PersonalMessage to achieve this.
Looks interesting, though.
Quote
Post I can understand for displaying the form back to users, but I don't remember it in drafts. There must have been a reason, very likely to find usernames, but I don't remember.
I'll let you check the files independently... Yeah IIRC it's to find usernames for recipients that didn't go through the auto-suggest.
Quote
Yup, though it's nice having the member's name in the right colour there as it should currently be.
I think that's the only place you get it apart from the info center... A real bummer.
Quote
Hmm. The point I was getting at is that I didn't realise there was a %P for lowercase,
It's technically valid only in Linux. It doesn't work in Windows and Mac OS.
Quote
and that I assumed %p was the only one out there, so that if it were found and we wanted to use lowercase, we'd have to manually replace - as was originally done by SMF...
You know, you COULD revert this and no one would blame you for that... ;)