Wedge

Public area => The Pub => Topic started by: Dragooon on March 30th, 2013, 04:46 PM

Title: Don't update log_online for certain actions
Post by: Dragooon on March 30th, 2013, 04:46 PM
Certain actions (mostly notifications polling) shouldn't update log_online table (Subs.php, writeLog function) since it doesn't make sense as the user actually isn't online and it hogs the Who's Online area.
Title: Re: Don't update log_online for certain actions
Post by: Arantor on March 30th, 2013, 04:54 PM
There are already some actions where it doesn't, I guess notifications can be added to that list. It's... complicated but I'll look at it today.
Title: Re: Don't update log_online for certain actions
Post by: Arantor on March 30th, 2013, 06:34 PM
OK, so I've done this, but this does have some other interesting consequences.

Someone could still be visiting the site, keeping the tab open - but not actually 'online' because their entry to who's online doesn't get renewed, though I guess if they're not actually 'online', it's all good.

Note that logging is per-action not per-subaction, so if there's any part of action=notification that needs to be logged, we need to fix that. But I'll commit what I have for this to be examined.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 3rd, 2013, 11:52 AM
Yes, I suppose not logging these would make more sense...

A few notes that might warrant comments, though.

- I only checked your latest rev out just now (and am about to cancel a few modifications I made on my local install and was planning to commit later today or this week :^^;:), so please bear with me.

- Before checking out this topic, which I kinda remembered was floating around (and I guess it was a good idea to check it out before committing anything :P), I made some changes to fix sudden bugs in the Who's Online page that were mentioned elsewhere. The reason was actually due to the fact that when updating wedge.org to the latest rev, I forgot to update index.php as well (it's not very frequently updated, and while I usually keep an eye on its latest modification date, it looks like I forgot to do it the one time it was needed.)

- So that's the reason why there were more 'unknown action' entries... These were from both 'notifications' and 'feeds'. It's interesting to know that there are so many feed requests, BTW...!

- But call me crazy, I STILL have to ask the question: what difference does it make that a bot is downloading a feed or a page...? It's still checking out whether I have updates in some particular area (overall, board, topic, etc.) The only point in logging online data for bots is that we know what they're indexing... If they're indexing one of my feeds, I'm interested in knowing it. What do you think? Does it make sense..?

- Also, there was still a media_wo_feed entry, which should (logically) not be recorded either, if main feeds aren't...?

- I've noticed that Who.english-uk.php has a use of 'license' in one string and 'licence' in another. I've changed it to 'licence', I suppose that was the intention...?

- Also changed an 'analyzer' to 'analyser'. Is that correct? Finally, removed plenty of language strings that were actually identical to Who.english.php's...

- My guess is that you (@Arantor) started working on it, and became busy on something else, and forgot to check when you got back. It's harder to check one's code when you're committing new code rather than code updates -- I've myself often fallen in some traps because of that.
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 3rd, 2013, 03:06 PM
Quote
- So that's the reason why there were more 'unknown action' entries... These were from both 'notifications' and 'feeds'. It's interesting to know that there are so many feed requests, BTW...!
Given how many places it's linked, not really a surprise.
Quote
- But call me crazy, I STILL have to ask the question: what difference does it make that a bot is downloading a feed or a page...?
SMF blocked them showing up in the online log. We just inherited that behaviour.

As for whether it is meaningful, I would say it is. Given how often feeds get requested, not running additional (and not entirely cheap) queries is quite a nice thing.
Quote
- Also, there was still a media_wo_feed entry, which should (logically) not be recorded either, if main feeds aren't...?
The deal is that anything hitting action=feed will not be logged. If the media feed is handled through the feed handler it will also not be logged. If it's handled through the media area it will be logged.
Quote
- I've noticed that Who.english-uk.php has a use of 'license' in one string and 'licence' in another. I've changed it to 'licence', I suppose that was the intention...?
Yeah, I didn't check that one as heavily as I should. It's really, really, really boring to go through English US to look for differences especially when you see the alternative spellings so often.
Quote
- Also changed an 'analyzer' to 'analyser'. Is that correct? Finally, removed plenty of language strings that were actually identical to Who.english.php's.
Only in English UK. Oh and you mean the credits items. I had a feeling they were left in for a reason but I can't remember what.
Quote
became busy on something else, and forgot to check when you got back.
Pretty much.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 3rd, 2013, 04:04 PM
Bugger... I'm currently trying out Maxthon 4 (Chrome 22 engine), and the little guy is not liking Wedge... -_-
Basically, from what I could gather: if a div is using the flex model, its children will ignore margin collapsing. i.e., my recent fix for quote margins is fucked up by Maxthon. Great...
Since my main browser is currently Sleipnir (Chrome 24), and it behaves correctly, I'd love to know if anyone noticed that issue... (It's pretty simple, post text is 'stuck' to the gray line that separates it from the post header, "Posted at ... by ...", you know.)
Quote from Arantor on April 3rd, 2013, 03:06 PM
Given how many places it's linked, not really a surprise.
Yeah... And we can't really hide these links from bots, ah ah...
Quote
As for whether it is meaningful, I would say it is. Given how often feeds get requested, not running additional (and not entirely cheap) queries is quite a nice thing.
I see...
Quote
The deal is that anything hitting action=feed will not be logged. If the media feed is handled through the feed handler it will also not be logged. If it's handled through the media area it will be logged.
Yes, it's using the media action. I'll look into it.

Oh, a suggestion for $action_no_log... Why not, instead, make this an optional third parameter in the $action_list array..?

$action_list = array(
  'action' => array('Function', 'File.php', $no_log)
);

Isn't that... 'cleaner'?
It seems technically doable, and IIRC $action_list can be altered by plugins anyway, can't it..?
The only issue is with extra_actions... It's no longer the same 'format'. But I don't see the point of this one either... Why not, simply add to $action_list..? Isn't this a global already? Yes it is...
Quote
Yeah, I didn't check that one as heavily as I should. It's really, really, really boring to go through English US to look for differences especially when you see the alternative spellings so often.
I thought you'd be using wingrep or equivalent, with a set of common used words.. No?
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 3rd, 2013, 04:24 PM
Quote
Isn't that... 'cleaner'?
No it isn't, seeing that there can already be three parameters (plugin added actions will add function, file, plugin id) which means it would become 3 or 4 parameters depending on plugin.
Quote
It seems technically doable, and IIRC $action_list can be altered by plugins anyway, can't it..?
The only issue is with extra_actions... It's no longer the same 'format'. But I don't see the point of this one either... Why not, simply add to $action_list..? Isn't this a global already? Yes it is...
I explained all this when I wrote it into the plugin manager.

The first question: "Is it truly and completely sane to load a function to run a function, every page load, just to add one line to an array?" (Rinse, repeat for every plugin that adds an action.)

Of course it's not. Which is why the plugin manager can add actions directly. The whole point of plugins declaring actions up front in plugin-info.xml is so that they don't have to be hooked in the conventional sense but can be added on to the list via the point when hooks are loaded (when $settings is loaded)

That's why we don't add directly to $action_list. But if you really believe it would be better to have files loaded just to run hooks to extend the actions list, like SMF does, we can remove it. I'd really prefer you didn't, though, since it would also mean rewriting part of the plugin system (both loader and manager)

This is also why we have the third parameter in the first place. Plugins do not and cannot use loadSource which is what $action_list expects. The third parameter is the plugin id so that it knows 1) to even call loadPluginSource and 2) it has all the information it actually needs to do so.
Quote
I thought you'd be using wingrep or equivalent, with a set of common used words.. No?
Eh, Notepad++ can do that. And I did start out with that but 'licence' wasn't one of the words I checked for. That, and the fact I wanted to skim through the language files, nice to see things I don't look at very often.
Quote
(It's pretty simple, post text is 'stuck' to the gray line that separates it from the post header, "Posted at ... by ...", you know.)
Not observed in Chrome 26. I still have issues with the horizontal scrollbars though.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 3rd, 2013, 06:34 PM
Quote from Arantor on April 3rd, 2013, 04:24 PM
No it isn't, seeing that there can already be three parameters (plugin added actions will add function, file, plugin id) which means it would become 3 or 4 parameters depending on plugin.
Hmm.
Quote from Arantor on April 3rd, 2013, 04:24 PM
I explained all this when I wrote it into the plugin manager.
But I didn't read it. I completely skipped most of the conversations about the plugin manager... I have to choose my battles ;)

Anyway, talk about clean code: if I want to prevent media feeds from being logged, instead of doing a define(), I now have to global $action_no_log, and add 'media' to it from within the feed function... :-/

I can do that if you think it's the preferred method, but I don't think it's 'clean' at all.
(Of course, adding a param to $action_list isn't gonna help in this situation, either; but I was thinking of WEDGE_NO_LOG in this case.)
Quote from Arantor on April 3rd, 2013, 04:24 PM
Not observed in Chrome 26. I still have issues with the horizontal scrollbars though.
On this very topic, with Chrome 28 Canary, at my very first post on the page, the contents of the post is limited in height to the height of my user box. Everything in the body that overflows from the assigned height then shows up on top of everything, including the following post... And it does the same for every single post after that. It's completely unreadable.

I fixed it by opening the dev tools, and disabling flex for the post area. Which totally doesn't make sense...

So, it looks like only Sleipnir (Chrome 24) seems to deal with flexbox correctly... :-/ And I don't know how to fix that.
To tell the truth, I'm very tempted to just give up on flex, even though I spent something like a week on that feature, and instead use macros to use, ahem, pure table tags by default in posts... (Which, of course, brings old problems to the table -- responsiveness and all.)

Fun, eh..?
(Oh, and yes, my font is Open Sans... The joys of custom.m1.css 8-))

PS: also, the bottom margin is bodies is eaten up in Chrome 28.
Posted: April 3rd, 2013, 06:30 PM

I'm starting to wonder if this isn't a graphic driver problem... I mean, I never had this problem with flex until today, and now I'm seeing it everywhere (in Chrome), although with different results depending on the version...
Oh, really funny: I'm in quick reply mode, and the text from my previous post is overwriting the textarea.
Let me see....                               (Oh, and yes, my font is Open Sans... The joys of custom.m1.css 8-))

I was only re-typing what was showing up at that exact position, ah ah. Well, gotta go, can't read what I'm typing anymore...
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 3rd, 2013, 06:53 PM
Quote
(Of course, adding a param to $action_list isn't gonna help in this situation, either; but I was thinking of WEDGE_NO_LOG in this case.)
WEDGE_NO_LOG had exactly the same problem, incidentally. The old check looked for existence but what happens is that existence will only occur in the directly included file. Look at the code flow - wedge_main runs, figures out the action to be diverted to, the action is loaded, THEN we do the logging, before actually heading to the action itself. This is because a lot of the time redirectexit() or obexit is called manually (which would prevent code flow returning to index.php)

So we have to do the logging before we go to do whatever the action is.

Consequently even your idea of globalling action_no_log isn't going to work because it will have already been logged by the time you get there. It's at the action level, not subaction level, that it happens. And whatever else, your file with that define simply wouldn't have been loaded.

Either move it to the main feed action (which isn't logged either, or shouldn't be IMHO) and then the no-log rule applies there, or its own action which can be excluded as you need.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 3rd, 2013, 07:12 PM
Hmm... I guess it's a lose-lose situation, then...

I can't change the URL. That's because older installs will still need them to be valid. Or I'll have to add a redirection like I'm doing for old feeds... But that's not 'clean', either.

... :-/
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 3rd, 2013, 07:24 PM
Except that if users use pretty URLs it's going to change anyway (since the PURLs scheme doesn't match what's available in any of the SMF mods)

People do expect URLs to change when software changes and it doesn't tend to upset others, I'd personally be inclined to not worry about it and just have it be whatever it is.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 3rd, 2013, 07:36 PM
No. pretty and non pretty URLs are interoperable...
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 3rd, 2013, 07:41 PM
*shrug* You do what you feel is important.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 3rd, 2013, 07:53 PM
That's the problem.... I feel that everything should be 'natural'.
And in this particular situation, there is no way for that to happen, so I'm asking you what your preferred solution(s) are...

(Would also appreciate some feedback on Chrome + flexbox model from other users...)
Title: Re: Don't update log_online for certain actions
Post by: godboko71 on April 3rd, 2013, 07:57 PM
On Chrome 26 right now will have o update to 28 later to see how it behaves for me.
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 3rd, 2013, 08:05 PM
Quote
And in this particular situation, there is no way for that to happen, so I'm asking you what your preferred solution(s) are...
I outlined two solutions I'd be happy with already.

Short of rewriting how the entire execution flow works, you can't fix it any other way, because what you're asking for is a way to selectively block logging based on a subaction while leaving the main action otherwise logged. But since execution doesn't get to handle subactions until after logging has already occurred, we have a problem in trying to support it like this.

Given that this is very firmly an exception rather than the rule, I don't really want to up-end a lot of code for the sake of an implied backward compatibility that isn't really that important in the grand scheme of things. Normally I'd be more inclined to support it but I'm not that worried here.
Title: Re: Don't update log_online for certain actions
Post by: godboko71 on April 3rd, 2013, 09:33 PM
As an admin and user I am not worried about links changing. If I change software its my job to worry about forwards or letting people know to change their blasted links in their feed readers.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 3rd, 2013, 11:51 PM
Pete..? This is the 'best' I could come up with... -_-

index.php:236 (after $context['subaction'] is declared)

Code: [Select]
if ($action === 'media' && $context['subaction'] === 'feed')
{
global $action_no_log;
$action_no_log[] = 'media';
}

Very ugly. But, well... At least, it works. And it shouldn't have much impact on influence. (The only impact I'm afraid of would be the extra bytes to load for each index.php file if it's not being kept in memory, I'm afraid...)

I tried doing it the hard way -- moving Aeva-Foxy's feed code to Feed.php... First of all, it adds 10KB to the 25KB in Feed.php (meaning parsing the file is a bit longer when it's a fairly often accessed file...), then the Foxy code in itself is too tightly integrated with AeMe to 'make sense' in Feed.php, in my opinion... I may be wrong, but... I felt bad doing this, so I cancelled my changes.

Today sucked.
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 4th, 2013, 12:36 AM
Now you need to add a hook in there for anything else that wants to do the same thing. Creating an exception for the sake of (theoretical) backward compatibility also means creating a precedent and a means for everyone else to do the same.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 4th, 2013, 08:42 AM
I don't want to create a precedent... I just want to be done with this crap!
It's either this, or in QueryString I modify the query on the fly to read like action=feed;sa=media... And still redirect to Aeva-Foxy.php's feed handling somehow.
Oh well... I should probably simply do includes in the feed code. I'll look into doing that.
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 4th, 2013, 04:12 PM
Quote
I don't want to create a precedent... I just want to be done with this crap!
You're going to create a precedent by specifically writing exceptions.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 4th, 2013, 07:09 PM
I don't know, did you look into my latest commit..? Isn't that what you wanted..?
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 4th, 2013, 07:26 PM
I haven't actually looked at the commit yet. I'm still trying to get the infractions system done - it's only been gestating for over a year.
Title: Re: Don't update log_online for certain actions
Post by: Nao on April 4th, 2013, 07:43 PM
Look at it when you have a minute, and tell me if that's okay.
Considering I did multiple concessions in this one (including getting rid of the '.xml' to 'feed' action converter for SMF... That one was hard to part with >_<), if you're not happy, then you need to tell me...
Title: Re: Don't update log_online for certain actions
Post by: Arantor on April 4th, 2013, 07:55 PM
Quote
Considering I did multiple concessions in this one (including getting rid of the '.xml' to 'feed' action converter for SMF... That one was hard to part with >_<)
That sounds like a plan to me. No-one really worries about feed links when moving from other forum software to/from SMF, I see no reason why we are any different - it's not like we're really an SMF version any more.
Quote
Look at it when you have a minute, and tell me if that's okay.
OK, so quick look. Reusing the gallery code makes plenty of sense when pulled under the feed... and I can understand the need also to initialise it in a way the gallery would expect. Though it might mean a gradual overhaul of the gallery code in time, I don't know.

The reorganisation of manage plugins is fine, and the new hook is fine also though personally I might have called it feed_types since that's what it really is. But 'feed' is absolutely fine too.

On the subject of passing variables by reference, yes, technically we should probably update everything to not have & in it. However it is nice to leave them in to indicate what the rest of the code expects will be passed by reference; the problem is that hooks can dictate the rules themselves because the receiver indicates its nature rather than the caller. But if we indicate what should be passed, and how (because that intimates what is safe to change and what isn't) that should be OK too.

I seem to recall the determine_location hook only really being useful for pretty URLs because anything else would naturally just head to index.php through a conventional scheme. I dunno. It doesn't hurt to have it where it is though it does mean that even non-PURLs cases would go there too.

The feed compatibility with SMF was always one of those tricky things, as was the mgallery->media change. I personally don't feel a burning need to retain compatibility but that's just me. While SMF->Wedge is the obvious target for conversion, there will be conversions from other systems too and I don't see why they should be treated as second class citizens by comparison. The playing field should be kept level where possible.

I'm curious, why should $_GET be used explicitly for actions? There is one interesting circumstance, though I can't remember where it would ever be an issue, is that if both $_POST and $_GET were to provide an action, $_REQUEST would contain the $_POST version not the $_GET one.