Wedge

Good :)

Also please confirm that we::$browser now works as intended? It does for me. php 5.2 with apache 2.2.11.

I haven't got PHP 5.2... haven't had it in almost a year... but back a year or so ago I was trying to make a weusr object and it wasn't working :/

If it's working for you, it'll almost certainly work for everyone else. But I'm going to be doing some real testing on real hosting before long (not a VPS I have total control over)

I see. Perhaps you forgot to add a static keyword to the variable? Which would be surprising...
Anyway, I can assure you that it works in all situations I tried.Lulz.

Okay, I'll be posting my comments on your latest commits here...

- ManageBans.english.php: this has exclusively $txt['ban_*'] entries, some of which are still present in other files. Do you think these other files have to be cleaned up, or they really need these entries separately from a custom loadLanguage('ManageBans') call...? It'd be nice to look into this.

- Actually a commit of mine...: I was wondering... Is ::getInstance() really THAT useful? I did a few class_exists('we') calls which I later realized return true not after calling we::getInstance(), but instead simply after loading the class file. Also, Wedge never actually requires getting an instance to treat it (for instance to get a list of variables or whatever, like I suggested in another post in this topic), so the only 'point' is to prevent getting two instances... However, it can also be done by providing a dummy __construct, a dummy __clone, and basically: what's the need for that..? Admins and users have no need for these, and plugin authors are unlikely to try and get an instance either (they'll 'do it' the way we do it, because they'll look at our code). So, I'd suggest simply having init() functions when needed (e.g. we object), and not bothering with calling new self() anywhere. Obviously, tested on PHP 5.2. One of the advantages is that you no longer need the strange call_array_func function call in Class-DB to extend the DB object. Because it doesn't do anything anyway... As you're more versed into objects than I am, please advise.

- Haven't tested your badge display order changes yet, but I was wondering... Couldn't my earlier system be replaced with yours? Or does it need both the display order thing and show_when stuff? (I'm not sure it can't be replaced with a negative value or whatever..?) I'm probably wrong, though. :)

- I liked, as a regular user, being able to view my own IP, so that I can determine if I'm on a dynamic or static IP. Though, it's also true that only power users would do that, and they'd usually have other ways to determine that... Or they'd own their own forum like I do, and check their IPs on them. ;)

- While I certainly don't mind you using British English in your comments (you are, after all, Her Majesty's subject), I'd like to point out that you're also using it in the language files (e.g. 'organise'), which is different -- AFAIK the language files are in US English, and British versions are to be provided in addition to them, not as replacements. I don't care what phpBB does, I just don't want to mix up British and US English, I already do it too much personally, but I have an excuse for that :lol:

More to (possibly) come later...

Singleton's __clone can also be private, just nuke the entire thing :P.

Well, good that you're aware of it. I like things clean. Enough work for translators already with the new strings... :)
Maybe we should do a script that takes all $txt[] strings and tests for them in the entire file set. If not found, add to a 'suspicious list' where we'll manually check that these strings aren't at least used in some more complex manners like string building ($txt['we_' . $string]), etc...It's very odd. If I just call we::init_user() and we::init_browser(), it works. If I load wedb without initializing it, it doesn't work. Does that mean we need to call a random method on the object in order for PHP to initialize it..?!Well, as it's a static object (at least static public methods...), with static variables in it, technically I guess PHP doesn't need to instantiate the object in the first place..? I don't know. I'm trying to learn.Yup, it's my only case of building a 'true' object, and making a 'clean' use of method visibility to simplify life for plugin/theme authors. It's actually what convinced me that objects can sometimes be more beneficial than procedural programming (I was fiercely opposed to objectifying anything before that... :P), but in the case of the system object, it's only static, it's not a 'real' object per se, although if you start moving allowedTo() over there, I guess it'll have to be... (Oh, how about a generic we::can(...) function? Lol.)I have no qualms with $user['permissions'] being private, actually I have no qualms with perms being moved to their own array outside $user, etc...I thought the comment next to that line sounded like you... What was the original implementation? I mean, if it's not using static late binding (because of compatibility), how did it do it? Multiple lines I guess? Can't bother to check it out...That's not what I meant... I had interest in seeing my IP's evolution over multiple messages, rather than my IP at the current moment. For instance, these days, my IP has been very static, it hasn't changed in weeks, perhaps months actually, even though officially I'm supposed to be on a dynamic IP... Things like that, I like being able to check. And I shall check that one. :PI feel for you. But just as my fellow Québecois cousins managed to gently bastardize French their own way, the 'main' language in here (as in 'used by the most people') is metropolitan French, i.e. they're the ones who have to take it on themselves to 'speak French' rather than Québecois when they want to address an international French audience. Same for the British... Because Americans can't stop thinking about sex, today the British have to speak American when they want to adopt a 'neutral tone'. Reminds me of that skit in The Meaning of Life... :lol:

Anyway! Can you check it out?You're welcome. I like to think my annoying diff reading sessions can sometimes help me catch a bug we wouldn't have found until much later otherwise. ;)Shorter function = more space for 'public' keywords, the very ones I complained about... :PDo you mean that this particular issue is caused by my topic privacy implementation...? Ooops. (Well, it's not even 'officially implemented' yet... And I doubt it'll ever be, because of all the performance issues we discussed?!)

I don't know how you're calling wedb...That's the thing... if it is a object with static variables, the variables don't exist until they are set as such.They're a tool, nothing more. What they really do is allow common groups of procedural functions, especially if they have data that is common to those functions, to be grouped together in one place. Especially if that data that the functions want to share doesn't need to go outside the group - e.g. DB connection resource.Therein lies the subtlety. If you're the admin, you have the ability to issue bans, therefore you can see IP addresses... it's really only an issue for those who can't issue bans, and most of the time those people aren't the webmaster ;)Sure I can, when I handle my next commit. This one's a bastard, though. If I were to say that I made a synopsis of what it needs to do and that was a list of 46 items in a row to be executed. I'm barely 6 lines into the list and already that's another 100 lines of code on top of the 18K class I already wrote... I could be here a while.The original implementation was all about $smcFunc. Back in $smcFunc days, $smcFunc was an array containing the names of functions to call in certain circumstances. db_extend would load the file referenced, then extend the contents of $smcFunc to refer to the newly loaded functions. I pretty much just turned that into some crazy class thing, rather than doing what I should have been doing, which was trimming what wasn't necessary and making a more sane method of calling those extra functions.I'm not entirely convinced it's 100% right in SMF either. The problem is that post approval is checked very, very early - and before permissions are loaded. This has actually been an issue for months, I first documented it here back in November (private topic, id 7677 in the team board), but back then there were no visible signs, because allowedTo() just returned false for it.

But now the bug is more visible. Checking whether we is defined won't fix it, because it will be defined, it just won't have had permissions loaded (which should *really* be a method of the class anyway)

There is a sort of cyclic dependency issue - we can't load permissions until we know what board we're trying to access, and we can't load the board without knowing something about permissions (since loading the board is when we figure out topic privacy and that's dependent on permissions). We could juggle the ordering around a bit, I guess, but really what we should be doing is overhauling it entirely and that would have other consequences that need to be attended to.

I've already outlined how I think it should be overhauled, both structurally and semantically but when I'm not stuffed up with cold and overflowing with zip file craziness, I'll explain how I think it should work, what the consequences are for the admin panel / permissions UI and the rest of the codebase are etc.

I hate colds

I meant wesql.
And it works really... Just rename __construct() to getInstance(), and remove the other getInstance() function... Voilà, it works perfectly that way too.
As long as you only access an object with static methods or variables, you don't need to instantiate it. That's why I'm tempted to remove getInstance() calls and replace them either with nothing, or with init() functions when needed. It makes things clearer IMHO.Really, you should try... I was the first surprised. Just include Class-System.php in a blank PHP file, and then immediately do:

echo we::$is_admin === null; // returns 1
echo we::$non_existent_variable === null; // returns error

This means an object's static members can be accessed as soon as the object is declared.

Also, I'm becoming serious about doing we::can('moderate_forum') instead of we::allowedTo(), it does seem very readable and blends in nicely with the we::is() function.Yeah, that's what I was saying: it's pretty much a moot point because I've been doing something only geeks would do, and most forum geeks already have a forum of their own...Well, as long as it doesn't reach the size you had before..?
I'd be tempted to say, why not just use the default zip loading classes and be done with it..? What do you need that they don't support?Can't we just do a backtrace on allowedTo() and catch the place where it's done wrongly, and then add a flag to disable permission testing if they're not loaded in the first place...? Then maybe call that function again later when permissions are loaded, to 'confirm' the actual user rights. Seems feasable to me, and most importantly, it saves time. I don't like seeing you waste time reinventing the wheel just to avoid a minor hack... SMF/Wedge are full of hacks, that never killed anyone.

I'm sure there was a specific reason for NOT doing it that way, but I forgot what it is.So instead of having getInstance() calls, we're going to have init() calls. Means the same thing, yes? But I'm still sure there's a reason why it was done the way it was done rather than what you're doing. I'm also a little annoyed at the sake of change for the saving of a handful of bytes with no other benefit as far as I can see.At this stage I don't really care. The whole permissions system needs to be rewritten anyway.Well, 199KB for a badly written and unmaintained class. 18KB, given proper documentation, is quite pleasant, not to mention the fact that I will be able to ditch parts of the old package manager in return.

As for the default zip loading classes... I guess I'll just repeat this yet again.

1. They all seem to rely on having various extensions for PHP which may or may not be included. Usually they are not included.
2. I explicitly need to be able to retrieve files from the zip file without creating ANY temporary files. I absolutely need to be able to step through the zip and extract individual files without ever writing anything to the filesystem anywhere, because I need to extract a zip file by file, specifically so I can use FTP to upload it.

I have spent many hours on this now, and I'm getting tired of people questioning why I'm doing it or what I feel I need to do, especially given that I have explained this multiple times, as well as the fact that I've had enough people telling me I should just take the easy option and strip it out (which also removes any possibility of easy patching from the admin panel, completely and utterly outright).

So, I will say this one more time. I am doing what I am doing with zip files and FTP. If you leave me alone to do it, we get nice and convenient uploads of plugins, including mostly magic updating of packages (i.e. upload a new version, it figures out, disables the old one, cleans up its files, then adds the new one), and that opens the door for patching Wedge with periodic updates as and when they come out.

If I keep getting people suggesting that what I'm doing is not worth the effort and that I should rely on methods that might not be available for users, I will end up just cutting all that functionality entirely because I know from experience that I can't rely on the ftp or zip extensions to be available.[1]

Also, if you're shitting yourself about a couple of hundred KB in the install package now, you're going to go fucking nuts in a few weeks when I update phpseclib and use an unflattened copy of the code, where I haven't stripped all its extra spaces and whatnot, because I won't spend two days manually patching phpseclib, I'll just include it wholesale.What would be the point? I know exactly where it's done wrongly. I've known for months where it's done wrongly. I even documented it and explained what has to be done to fix it being called wrongly. You might as well just replace the one expression with false for all the good it will do - yes, it will stop throwing an error but it doesn't fix the problem that's been there. The broken logic will still be broken, it just won't show you an error about it, like it hasn't been doing since forever.

I will say this a fourth time. THIS IS NOT A NEW BUG SUDDENLY BECAUSE OF YOUR CHANGES TO WE::*. This bug is months old. The only reason you're even noticing it is because now the call is being made before permissions are loaded and it won't be trapped by the old test in allowedTo(). I noticed the error in logic in November, but it's months older than that. The logic is still fundamentally wrong, the fact it throws an error now is merely the result of changes that made an already existing bug more obvious.Are you not reading the words I'm writing? Am I talking to myself here about how all this stuff works? Did you read the topic I have mentioned (twice now in this topic alone) that spells out the problem, why it's a problem and how it can be fixed?

It's not going to save time to have to re-figure out all the permissions for boards and topics. The only way to fix this is to rewrite, once and for all, how permissions works.How is it reinventing the wheel to avoid going through the entire permissions stuff twice because it's fucking broken?!

Oh jesus I can tell how furious I am right now about how much effort I've spent and how much of a waste of time it feels like. It is what it is. Good night.

1.

And if anyone so much as suggests that I don't need to worry about FTP and that I can rely on chmod, I will visit them in the small hours, cut their heart out with a spoon, then stick their head on a spike, above a sign saying "This is what happens to people who think making software insecure is a good idea." I won't of course, but there is absolutely NO WAY IN HELL I will encourage this sort of behaviour. It's FTP/SFTP or not at all.

Oh, please... -_-

I'm tired, I'm pissed off and I keep hearing the same things over and over: why don't you use <x>, why don't you do it <this way>? If I thought using <x> was a good idea or doing it <this way> was a good idea, I'd have done it.

I wouldn't have spent the time and effort I have spent, chasing the most ass-backward solution I couldn't even have dreamed of in my darkest nightmares, if I thought for one moment there was a better way that had any reliability without compromising security. The fact that I've documented this many times in the last 18 months is just icing on the cake to me, only to now effectively be told that I'm wasting my time. It's like an architect spending a year of his life designing the most amazing palace and then just getting a shed from IKEA and knocking it together in the afternoon so he could go down the pub. That isn't how I work, you know me well enough by now to know that that isn't how I work.

Yes, I could use these other methods, but I don't like compromise. I won't trade security for anything, and I won't trade performance for laziness - calling something twice because the first time it couldn't be done properly just says we need to fix it so it would work the first time as far as possible, and that means doing what I've been saying for 2 1/2 years about overhauling permissions anyway.

This is why I'm so pissed off. It feels like no-one's listening to what I'm saying. None of this is new, none of this is suddenly out of nowhere. It's not like all of a sudden I'm springing things on you or anyone else. I feel like my posts are on global ignore because what I'm saying is not being taken into account even if I explain them multiple times at great length.

It hurts. It hurts that I feel like this. It hurts that I feel like I'm wasting my time, that I've spent hours and hours researching this stuff to be able to build the best that it can be. It hurts that you don't seem to trust my judgement enough, because if you did, you wouldn't be questioning the rationale behind my thoughts (doubly so since I've already explained most of them multiple times)

I've been tempted to throw the towel in, but we've come a long way and I don't want to waste that. Not for anything or anyone, no matter how frustrated I get. But there's no point in my documenting what I'm going to do, why I'm going to do it or how I think it should just be done - because no-one's reading it anyway. Instead I'm just going to do it and say what I've done. I'm not even going to bother writing *why* any of it was done, because there's clearly no point in doing that beyond the changelog, I'll just wait to be asked in future, because then I'll only write it all out 1 1/2 times (there are of course comments in the source for everything), and save my energy for trying to write code, because I clearly suck at anything else like explaining what I'm doing in a way that anyone reads or cares about.

I think I know why... There are two ways of making a singleton. One is static (the ones we usually use), one is dynamic (I think the editor somehow uses instances, would have to dig into it). Here's an example:
http://www.matthewelliston.com/php-singleton-database-class/
As you can see, this one has the getInstance() function too (it seems to be a common name for class init functions...), but it also uses $this inside the object itself. I haven't tried it, but perhaps it could also work if replacing all $this-> call with self::, I don't know, what I'm interested right now, is in the fact that I'm still learning about OOP and as a geek, I need to *understand* the inner workings of objects, rather than just "copy and paste code that I saw elsewhere and worked perfectly" (which, I'll admit, is something I've been doing a lot when it comes to objects... Trial and error is my thing, rather than reading docs :P)

So, I'm thinking that we could simplify these objects, maybe not even care about singles instances, because what's the worse that could happen...? A plugin calling for another instance of an object? Why? At best, it would simply not work. They can't "hurt" Wedge as far as I know by doing that. Generally speaking, my pushing wetem methods to public or protected statuses was only done to help plugin authors understand what methods are there for them to use, and what methods are part of the magic trick that they don't *need* to call (not that they can't look into it... They just don't need it.)

Actually, until a few months ago, I thought that ::getInstance() was some sort of magic function without the double underscore convention. It's only after making a google search on this that I realized it's not a feature, it's just a naming convention. And in my humble opinion (you don't see that one spelled out entirely a lot these days :P), it was created for objects that are actually instantiated (either once or several times), and just re-used by developers who made a copy and paste and rewrote the code to prevent further instances -- without really thinking about the fact that the function itself may not be needed. If you'll kindly look into our code, none of the Wedge files call ::getInstance() for anything else than object initialization. And some of these initializations are absolutely not needed. Again, just try removing all of the new self() calls, most objects will not only initialize fine, they will work the way they're expected to. And that's with PHP 5.2 enabled on my dev platform (I'll admit that it's best to use the lowest common platform for my testing, otherwise I'm bound to have issues in the future.)I can sense the tongue in cheek, but really it isn't the same thing. getInstance() means to me "get a pointer to that object", while init() means "initialize that object". The fact that getInstance() initializes the singleton isn't something we are to 'expect', it's a convention. If you're just a rookie programmer, you'll be puzzled by that call. I know I was when you started implementing it for wesql and others. I just didn't know what it was about. I let you do that magic because frankly it's quite harmless and I'll admit I didn't want to seem like a complete OOP rookie (which I was, I've never been comfortable with objects), but I know better now, at least when it comes to singleton concepts, and I think that we should cut rookies some slack and rename that to init() and only call it if the object actually needs some filling before being used ;)Mostly clarity, in my mind. And making me more comfortable with OOP, which in turns encourages me to use objects even more (given that I've already created quite a handful of critical objects for Wedge, such as wess, wetem and we, it's good time that I get comfortable with developing them further without relying on pre-existing code.)Do I have your go for moving allowedTo and isAllowedTo to we ASAP? (i.e. are you working on this right now yourself?)I thought you'd said that you'd managed to reduce it to something like 80KB but that it was still messy. So I figured, if it starts like Wess (an easily manageable 15KB file which progressively got new features added and ended up at 55KB, although not a mess this time ahah), I'm just trying to help and save you time.
I must say though, what you committed yesterday looks great, and I also forgot about some of the requirements you had for your object.I need to make a few things clearer, I guess.
- This month hasn't been easy on either of us.
- After your leave of absence (in terms of commits-ment) of several months, I kinda grew accustomed to being the 'guy in charge', so it's proven difficult for me to dive again into something I've never liked (proof-reading others' code), because eventually I'll always start nit-picking about this or that, not because it NEEDS to be said, but because I feel I'm wasting my time if I don't have at least a line or two to change. I don't know how you do it yourself, maybe you don't proof-read much of my code, I don't know, maybe you just don't feel the need to add your touch, in which case you're lucky, but in the end that's what I am: an annoying partner who's full of good intentions, but not exactly good at explaining them or justifying them other than saying it 'makes more sense'.

Your architect analogy reminded me of The Fountainhead... Or was it How I Met Your Mother :lol:No, actually I don't give a damn about filesize. ESPECIALLY when it comes to the admin area -- go crazy and waste as many bytes as you like over there, because it's a private area that has absolutely zero impact on overall server performance or bandwidth. You didn't see me complain about the addition of jQuery UI, did you? :P Nah, really what I thought is that maybe you shouldn't kill yourself over rewriting a class if there's already an acceptable solution out there. Then again, we're both all about challenge, aren't we..? We're long past the work-hour efficiency stage, otherwise we'd have released Wedge back in 2011, and our lives would be less complicated ;)Again, do as you like in the admin area. I'm just conservative on general forum page bytesize ;)
For instance, it's your job to determine that you'd rather use jQuery UI for the badge manager. Which is great. Then it's my job to determine that if plugin authors want to use that copy, they should access it through 'jquery-ui.js' in case we update the file.

Oh, by the way this reminds me... Why not follow the CDN setting for jQuery UI too? I mean, the same file is available through here as well...
ajax.googleapis.com/ajax/libs/jqueryui/1.8.24/jquery-ui.min.js (latest hosted version is 1.9.2)
And other CDNs like code.jquery.com, of course.
Or directly use that one only. I'm not 100% sure about it though, I'm not even 20% sure about it, because if we encourage plugin authors to use the CDN version, there's hardly a way to determine that they're not double-loading it -- IIRC Wedge deletes duplicate URLs, but we can't be sure they're not going to use a different CDN or version.
(Then again, the most likely conflict to happen is that people start making a plugin out of a JS script that also requires jQuery UI, and then they add another JS script that also requires it, and voilà, conflict...)

Oh, my head...!
What was I talking about already..?But permission error = security error, right...? Doesn't that warrant an immediate hack fix?I *did* understand that part, at the very least ;)As I said at the beginning of my post, ever since you went back in full gear, I've been having a very hard time catching up with your topics... And I actually stopped trying a couple of weeks after your numerous new topics. I just couldn't keep up, so I just focused on the new ones. It's hard. I guess I'm more of a "code first, ask questions later" type...

Actually, I think you should do that as well. Your code is always of the utmost quality, you have great ideas, you don't have to explain them and ask for opinions because we're always here telling you to do it. And whatever comes later -- well, at least you'll have done it, which is not something many people can brag about.