New revs - Public comments

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #180, on October 4th, 2011, 11:41 PM »
Re: deletion-- oops. I think I actually did it the correct way in the hack I posted back in 2008 or so at sm.org.

Really shouldn't be coding when I'm tired....
Re suggest, I was actually thinking of a separate page to confirm rather than a confirm box. Will have to fix suggest though. Unless you do it before me. Will resume my answer tomorrow.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #181, on October 4th, 2011, 11:47 PM »
I cannot reproduce the actual error with restoration myself, however I don't have pretty URLs on.[1]

It seems irrelevant, but the only thing I can tie into it is that when successfully proceeding with the restoration[2] is that at the end it falls to a redirectexit(). No skeleton is set up by the action itself, as no block is loaded - and that would give you the same error, presumably.

Something about pretty URLs causes redirectexit() - no parameters - to fail. In the spirit of interest, while already logged in, head over to action=login and see what happens. If that fails the same way (which I expect it to), there's your culprit. Fixing it though... that's another story.

As far as the restoration going into a phantom board, it certainly should check - the code looks like it's trying to do just that (and with an INNER JOIN to the previous board, it *should* fail at that point). I'd even argue that it should have validated the ownership of the post in the first place - because under the same setup, a post deleted from a board you can't see will end up in the same place, and you can restore it back to that board without having access to it.

Also note that the post's icon is changed too, there's no reason why it actually has to change the icon, displaying inside the recycle board simply needs to fudge the icon in the message and display loaders without having to compromise data.


I can certainly fix up suggest in the meantime since that would imply it's broken elsewhere too.
 1. Frankly, every time I do enable it, something usually breaks, so I never touch it, but maybe I am doing something wrong!
 2. In other news, it seems strange that RestoreTopic is in RemoveTopic.php, but then again we did the same in SimpleDesk...
Re: New revs - Public comments
« Reply #182, on October 5th, 2011, 12:26 AM »
OK, so I see the suggest JS is fixed. I've also fixed a side issue that I noticed from editing boards, in the clean_cache function.

Going back to the repairboards code, I can see that Subs-Menu is supposed to deal with pages that use 'select' to cross area boundaries as far as the menu's concerned. Line 205 has it resetting the current area, so as far as I'm concerned, it's supposed to be dealing with that automatically.

The only reason the top level stuff is handled in Admin.php is because it expressly doesn't appear in the menu, and really, neither of those two items should. repairboards is just special, because it's the only item that now does that in the Admin page.
Posted: October 5th, 2011, 12:17 AM

Meh, just rejigged the code in Subs-Menu to not only update the current_area but also to force it to be detected as such later.
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 #183, on October 6th, 2011, 02:58 PM »
Suggestion with contiguous pages:

$contiguous_pages = isset($modSettings['contiguous_pages']) ? $mod[..] : 5;

(Can't remember the name of the variable off the top of my head.)
This would allow Wedge to simply re-use the imported SMF's value; although probably not very important...
Similarly, every time we change the name of an SMF variable (from $modSettings and $settings, I mean), we should ensure it's also reflected in the import tool.

Thorsten, do you keep track of these...?
Quote from Nao on October 4th, 2011, 11:11 PM
Re: restore posts, I couldn't find the origin of the skeleton error. Argh. If you could look into it as well... I have no time to actually go and try to restore posts for now.
Headache... Tried to delete another post, and it doesn't show up in the recycle bin. I probably disabled it... But then why did my earlier topic get put into it...? I suspect there's something awry here.
Again, not really worth much of my attention, as I'd really like to get rid of the bin before we go alpha...
Quote
Loosely related: when deleting a post, Wedge should update its 'children' posts to use the deleted post's parent ID. Should be easy enough, but we then have to update their own children, too... I'm not sure how to do this with minimal impact on the DB performance.
I committed my query for this. Heck, it pissed me off so much, I didn't take time to actually test it... (I mean, it doesn't crash, but I haven't tested with phpMyAdmin around.)

Been procrastinating a bit regarding Wedge lately... Got hooked on Misfits (UK show). Misfits is to Heroes what Skins was to 90210 & co. (Well, except I liked the first few episodes of Heroes better than the first season of Misfits. But then again, they should have made Heroes focus on Hiro and last only one season... Instant cult show.)
Posted: October 6th, 2011, 02:57 PM
Quote from Arantor on October 4th, 2011, 11:47 PM
Something about pretty URLs causes redirectexit() - no parameters - to fail. In the spirit of interest, while already logged in, head over to action=login and see what happens. If that fails the same way (which I expect it to), there's your culprit.
Nope..... :(
No error.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #184, on October 6th, 2011, 03:05 PM »
Quote
Suggestion with contiguous pages:

$contiguous_pages = isset($modSettings['contiguous_pages']) ? $mod[..] : 5;
Works for me.
Quote
Similarly, every time we change the name of an SMF variable (from $modSettings and $settings, I mean), we should ensure it's also reflected in the import tool.
On the one hand I see the wisdom of this, and on the other, I find I'm not actually that bothered by setting it. Stuff that's actually important functionally, yes, but stuff that's aesthetic, like this - I'm just not that bothered, especially since I doubt most of the time people even change half these settings. In this particular case I deliberately didn't even make it a theme setting - because I do not recall ever seeing it modified anywhere. Not everything needs to be configurable from the ACP.
Quote
Headache... Tried to delete another post, and it doesn't show up in the recycle bin. I probably disabled it... But then why did my earlier topic get put into it...? I suspect there's something awry here.
Again, not really worth much of my attention, as I'd really like to get rid of the bin before we go alpha...
Then let's figure out how it should work instead of trying to fix issues that will be obsoleted out.
Quote
Been procrastinating a bit regarding Wedge lately...
You are allowed to have a break, you know :)
Quote
No error.
It was down to how the select handling worked, I've since made a change to Subs-Menu that deals with these cases, though repairboards is the only case of it.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #185, on October 6th, 2011, 05:39 PM »
Quote from Arantor on October 6th, 2011, 03:05 PM
Works for me.
Done...
Also converted the floor((int) x/2) to just x>>1, which does exactly the same thing... Only faster :D
It's funny that SMF's version of the conversion is even more complicated, something like ((int) x - ((int) x % 2)) / 2... Lol.
Quote
On the one hand I see the wisdom of this, and on the other, I find I'm not actually that bothered by setting it.
I think we renamed a few 'important' things in the past... Can't remember which, though. Well, I guess our users will tell us in time ;)
Quote
Then let's figure out how it should work instead of trying to fix issues that will be obsoleted out.
We should probably get started on removing the bin right now...
Quote
You are allowed to have a break, you know :)
I'm not sure about that... My schedule... It's... Oh well, I guess we can screw my schedule...
Quote
It was down to how the select handling worked, I've since made a change to Subs-Menu that deals with these cases, though repairboards is the only case of it.
Are we talking about the same thing...? The empty skeleton error?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #186, on October 6th, 2011, 05:50 PM »
Quote
We should probably get started on removing the bin right now...
That's cool, provided that we come up with a functional replacement.
Quote
Are we talking about the same thing...? The empty skeleton error?
I didn't get into the error in recycling topics, I didn't see a lot of point, but I did look into the other error that was mentioned at the same time in repairboards.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #187, on October 6th, 2011, 06:53 PM »
Quote from Arantor on October 6th, 2011, 05:50 PM
Quote
We should probably get started on removing the bin right now...
That's cool, provided that we come up with a functional replacement.
The ''approved' bit...? Setting it to '2' for future posting (or whatever), and '3' for 'deleted'...? Isn't it enough?
We could simply rename it to 'status', 'state' or 'online'... (Well, that would be something we need to correctly import from SMF... ;))
Posted: October 6th, 2011, 06:52 PM
Quote from Arantor on October 6th, 2011, 05:50 PM
I didn't get into the error in recycling topics, I didn't see a lot of point, but I did look into the other error that was mentioned at the same time in repairboards.
But I suspect we'll be getting more of this 'empty skeleton' error where we expect it the least... Maybe I should give up on that 'feature'.

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #188, on October 6th, 2011, 07:46 PM »
I'm not so bothered by the physical status part, though that is ultimately the way to do it. What concerns me is how we show the user. How do we display it?

I'd also prefer to separate deleted-by-author from deleted-by-moderator.

As far as empty-skeleton errors, that implies a logic error, and if there isn't a fatal error shown to the user, what would happen? I'd argue that a page with no content is less helpful than a page with an error message.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #189, on October 7th, 2011, 07:07 PM »
I think you could have guessed I was planning to split the Display template into several functions just like I did for the index template... But the good news is that you did it in a very clean way and I couldn't have done it better so it's all good :) (Maybe I would have removed the spellchecker though... I'm not exactly sure how useful it is in these days when most browsers have inline spellchecking.)

(Plus, I'm having such a hard time working on Wedge these days...)
Quote from Arantor on October 6th, 2011, 07:46 PM
I'm not so bothered by the physical status part, though that is ultimately the way to do it. What concerns me is how we show the user. How do we display it?
It's the easy part to me... :^^;:
Quote
I'd also prefer to separate deleted-by-author from deleted-by-moderator.
Sure. The 'approved' field can really have any value at this point... As long as the only valid value for showing a post is '1'...
I've started work on it, BTW. Although I'm currently trying to determine whether I should keep it as 'approved' or use the opportunity to rename it to 'status'.
Quote
As far as empty-skeleton errors, that implies a logic error, and if there isn't a fatal error shown to the user, what would happen? I'd argue that a page with no content is less helpful than a page with an error message.
Possibly. But maybe we should also log the contents of the empty skeleton for admins to look into...
Posted: October 7th, 2011, 04:28 PM
Quote from Arantor on October 7th, 2011, 11:14 AM
@ Nao: Something in my brain told me not to make the title_upper and _lower parts (which are used for the page titles/go up+down areas) into a template layer but I'm not sure *why* my brain told me that. If you wanna change it, please go right ahead.
I have absolutely no idea why this shouldn't be done... I'll be working on it. It's not like it can't be reverted...
Posted: October 7th, 2011, 04:32 PM

Oh, and one thing that could be improved... Testing for some values and, if they're not true, either don't add the corresponding block, and simply removeBlock() it... (Well, the problem with the second solution is that when we start using an object for this, it'll be a tad slower than just unsetting the block in the skeleton array...)

What do you think of this?

Another possible improvement: allow for params to be passed to template functions. Something like, loadBlock('mod_buttons', 'upper'), with 'upper' being passed to the template function so that it knows what to show.
Posted: October 7th, 2011, 04:37 PM

Having trouble with the postlist (i.e. topic posts) layer...
So, if I want to load the layer into default, surrounded by blocks, I have to do it this way...

Code: [Select]
loadBlock(
array(
'report_success',
'display_draft',
'title_upper',
'topic_poll',
'linked_calendar',
)
);
loadLayer('postlist', 'default', 'child');
loadBlock('display_posts', 'postlist');
loadBlock(
array(
'quick_access',
'mod_buttons',
'quick_reply'
),
'',
'add'
);

I'd say it's pretty ugly.
Other solutions:
- add the ability to specify layers directly within the block list, by specifying a sub-array, and possibly other blocks inside the array.
- just forget about it...
- add the layer after the main block code, and add to loadLayer() the ability to inject a layer before or after a *block*, just like loadBlock offers the ability to inject a block before or after a block or a layer. This is my favorite solution (I like the second one equally, though... :^^;:), but it may make the code even more confusing... (?)

Opinions?

Arantor

  • As powerful as possible, as complex as necessary.
  • Posts: 14,278
Re: New revs - Public comments
« Reply #190, on October 7th, 2011, 09:03 PM »
There is, of course, an ulterior motive for breaking the template up - injecting the calendar into its original place, from a plugin perspective.

I've been debating removing the spell checker. It's not unreliable but it is a bit fiddly to set up, and with it being on the client side (which will likely have the user's word customisations in, and so on) it seems less worth including.

I'd take the opportunity to rename it. Question: what happens with an unapproved post that then gets deleted?

Logging the empty skeleton is only useful if you can log the circumstances that got you there...

Ah, yes, that was my reason: I couldn't figure out a way to load the block/layer list in a single statement. As for loading things/not loading things in the block list, I couldn't figure out whether it would be cheaper to load everything and exit per template if we weren't doing anything, or load the list and remove items if we weren't going to use them, or only add them if we were going to (but in my head, adding an item is probably slightly more expensive than adding a single big list and exiting the function)

I don't think there is a good right way to do it...

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 #191, on October 8th, 2011, 09:43 AM »
Warning: Invalid argument supplied for foreach() in /wedge/Sources/Subs-Cache.php on line 452

Litespeed, PHP 5.3.6, and XCache
Posted: October 8th, 2011, 08:46 AM

I haven't the faintest on how that cache works, but from the error;'s behavior, it seems that when it creates a new file it spits that out since I only get it on pages I didn't yet visit.

JS is minified with Packer and compressed JS/CSS is disabled, also filename obfuscation is off. This is on a new install.

Also, the installer's CSS style seems broken when I ran it this evening.
A confident man keeps quiet.whereas a frightened man keeps talking, hiding his fear.

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #192, on October 8th, 2011, 10:14 AM »
Will have a look.

Installer -- I rarely run it. I suppose it's linked to the caching error.

TE

  • Posts: 286
Re: New revs - Public comments
« Reply #193, on October 9th, 2011, 08:37 AM »Last edited on October 9th, 2011, 09:11 AM by TE
just a small bug report:
Security.php line 419 starts with:
Quote
fatal_lang_error('your_ban'], 'user'...
the closing  bracket needs to be removed:
Quote
fatal_lang_error('your_ban', 'user'...
Edit: found another one:
Admin Area - side bar:
Code: [Select]
Warning: strpos() [function.strpos]: Empty delimiter in C:\Program Files\Apache Software Foundation\Apache2.2\htdocs\wedge\trunk\Sources\Errors.php on line 98

btw, how to enable or disable "Core Features"? I had disabled the gallery a while ago and now I cant find an option to enable the gallery again.. Maybe I'm blind but the "Core features" don't allow me to enable / disable a feature...
Thorsten "TE" Eurich - Former SMF Developer & Converters Guru

Nao

  • Dadman with a boy
  • Posts: 16,082
Re: New revs - Public comments
« Reply #194, on October 9th, 2011, 11:28 AM »
Quote from TE on October 9th, 2011, 08:37 AM
just a small bug report:
Security.php line 419 starts with:
Thanks, fixed for Security... As for Errors, I can add an extra test, but are you sure you updated your local copy's Settings.php file? I know I didn't, originally, so $pluginsdir was empty...

I'm currently going through Pete's commits. 2000 more lines to go... Known song, this week :lol:
Unfortunately I can't devote a lot of time to Wedge this week. RL stuff :(
Quote
btw, how to enable or disable "Core Features"? I had disabled the gallery a while ago and now I cant find an option to enable the gallery again.. Maybe I'm blind but the "Core features" don't allow me to enable / disable a feature...
I don't believe the feature is buggy, but OTOH, Pete is adamant on deleting that page IIRC, so it'll end up being somewhere else.

If you want to re-enable it manually, that's in $modSettings/wedge_settings ;)
Posted: October 9th, 2011, 10:46 AM

Oh... And did you see the question for you above? (reply #183)
Posted: October 9th, 2011, 10:52 AM

(Found another syntax error in Activate.php, will fix in the next commit.)
Posted: October 9th, 2011, 11:21 AM

Pete -- there are so FEW fatal_error() calls left, I think it'd make sense to actually rename them to fatal_error_hardcoded() or something (who cares), and rename fatal_lang_error() to fatal_error()...
Posted: October 9th, 2011, 11:23 AM

Also, there are some places where the fatal error gets a 'user' value to it when the original was either false or 'general' (or empty, = 'general'). They're in Aeva-Gallery.php and Reminder.php:

-               fatal_error(sprintf($txt['media_cf_empty'], $field['name']));
+               fatal_lang_error('media_cf_empty', 'user', array($field['name']));

Any reason, Pete?