Wedge
Public area => The Pub => Topic started by: Nao on December 23rd, 2012, 11:26 AM
-
I'd also like to create a cx (context) object. Not with the usual 'we' prefix, I know, but the idea is to keep it very, very short, and 'we' is already taken for the system class, although we MIGHT be able to use 'we' instead and just have we::$user behave like if it is was $context['user'], or whatever.
The main problems are:
1/ Performance. $context is used in TONS of areas, including time-critical code, so it's going to be hard to tell people to use 'cx::$var' instead of '$context['var']' in these areas. So we'd need to keep having a global point to the array.
2/ Because of (1) and general laziness from devs who might have $context so deeply carved into their DNA, we could/should/might use $context =& we::$cx, or cx::$cx, or something, but that means we can't use cx::$var, but instead we::$cx['var'], which only saves one byte compared to $context['var']. We could go as far as we:$c['var'], but even then, it's a bit ugly and all anyway...
3/ Some people might argue that we could simply rename $context to $cx, and be done with it, accept globals and that's it. :P
I'm looking into other solutions... So far I've found a strange one, which could work but only for variables that never change...
$context = get_object_vars(cx::getInstance());
This will effectively transform cx::$var into $context['var']. Seriously. But I'm guessing that, even without benchmarking it, this function call is not 'free' and thus can only be done on purpose at one point or another...
Ahhhhhhh... If only accessing a singleton variable was just as fast as accessing a global var! Why does it have to be about 60% slower..?!
-
Because of the fact you're addressing an object statically rather than actually addressing a variable. It is much the same overhead as calling a function - classes live in the same sort of place as functions and addressing is handled in much the same way.
-
Nope, remember... we::$var is 60% slower than $var, while we::get('var') with function get ($v) { return self::$$v; } or something like that would be something like 1000% slower... So it's definitely better to use method static variables than functions to retrieve a variable that's not within our scope.
Other than that, I'd like to know if any of these ideas made sense to you... ;)
-
The reason is that it has to do lookups to find the class and the way it does that is much the same as the way it looks up defined functions...
I have no problem with telling people not to be lazy. So if that means they have to learn not to use $context, that's fine with me.
I'm not opposed to such a class but it might be better served with other kinds of refactoring and making sure that what goes into $context actually needs to go into $context. Of course, when in loops it can always be pulled into a local variable, acted upon and if needed, later pushed back to $context.
-
Ahhhh.
Now I remember why we::$user or similar wouldn't work for me when I first tried it... it's PHP 5.3 only. We just effectively made Wedge PHP 5.3+ only.
When I previously tried it I was still using PHP 5.2 which is why it wouldn't work.
-
I probably forgot to declare the variables as static.
Can you verify it works if you do that? I'm pretty sure it does. 5.3 only allows NOT specifying the static keyword and still being able to access it.
-
Given that it appears to be run through the LSB code which is 5.3+ only and it's even specifically referred to in the Zend PHP 5.3 certification as such...
-
I dont get it?
Im just saying, in we::$user, we need to declare public static $user instead of just public $user. Is that the case? I'm not in front of my computer for now...
-
And I'm saying that as far as I'm aware, that won't work before PHP 5.3 for calling it outside the class.
I'm taking the Zend certification in two weeks, so I'm reading up, and in the study guide for PHP 5.3 it actually mentions this and says that it should only work in 5.3+, suggesting that it is reliant on the implementation of Late Static Binding (which would also explain the performance hit)
Mind you, I've found multiple other issues in the study guide so it wouldn't surprise me if it were actually wrong anyway. All I know for certain is, I tried to do something much the same a couple of years ago and it baulked at it because it was PHP 5.2. Whether something is slightly different or not, I don't know. Whether I did something different or not, I don't know. But I was following much the same basic approach then and it didn't work.
And I'm concerned that this effectively makes PHP 5.3 a minimum for Wedge. I'm not entirely convinced that's a bad thing, though.
I have to admit my temper is *a lot* shorter in recent times. I don't like Christmas. I don't particularly like New Year. I've spent a lot of time trying to make sense of all the changes to we::* to figure out why stylesheets aren't rendering properly any more (at all) and given up hoping that it'll just be fixed some time so that I can actually navigate the admin menus again... combined with the fact that I basically wrote all those little things last night to feel like I'm actually doing something useful rather than spending weeks farting around with something that just feels like a total waste of time.
-
I'll have a look when I'm on my pc.
Everything works fine for me locally, otherwise I wouldn't have committed. I'm very surprised. Hopefully this won't represent the quality of my work in 2013 ;)
-
Well, I found another bug that I've caused by my removal of loadUserSettings, which I've fixed locally, but ever since we was introduced, stylesheets are compiled but there's nothing browser specific in them, meaning menus don't have a background (which makes then practically unusable), the error log counter etc. is invisible, the background to the page is not a nice gradient but instead just blue, etc.
As far as my last point goes, yes, I could just ignore the whole package thing and just remove the add plugin setup, instead of faffing around understanding the zip format and making a nice class instead of the crap I found online (I think you'd rather I committed something around 80KB instead of a 199KB library, for example) and I'm just pissed off with how much work has gone into it with no visible benefits yet.
-
Yet being the key word. You are doing great worK both of you. As for 5.3 hard to say its a bad thing really might annoye some.
-
That's the thing, I'm not really the delayed gratification type. I like to see visible signs of progress as I go. The thought of spending two weeks staring at a piece of crap code and without anything to show for it except a strong dislike of anyone else's code... yeah, it's as frustrating as it sounds.
After two weeks of attacking it, stripping it down to half size, I realised it could be done further but it would at that point be quicker just to start fresh, which I did.
-
Pete -- I just can't seem to be able to install PHP 5.2 on my PC. It keeps sending me a connection error in WampServer, even though I followed the pretty straightforward install process. Anyway, problem is -- the system object DOES have 'static' keywords in front of our variables like $user. So it really should be working in PHP 5.2... I've even seen comments to that effect on the web.
Maybe (but it's very unlikely) you need to add '&' in front of the getInstance method in the object, to return a reference, but I don't see why that would change anything.
Anyway, I found this:
http://www.nqbao.com/2009/11/the-best-way-to-access-PHP-global-variable
The guy runs PHP 5.2.9 and look at solution #7... It's the same as mine, pretty much, only very simplified.
So I'm guessing it does work in PHP 5.2, it just doesn't work for you in that particular fashion.
Re: stylesheets, the main problem is that my own local version is uncommitted because it makes so many little changes here and there... I don't know exactly if committing them immediately would fix your problems.
Re: zip class, I don't know any better... I think a 80KB library is overkill already. But that's just me... If it makes life easier for everyone...
-
I could have sworn that it wouldn't work. I don't know what I did differently that made it not work, and when I stumbled on the reference to it in the study guide, I figured that it was all related, and it probably is. I don't know. I just don't want to compromise Wedge working on PHP 5.2.
The problem with stylesheets isn't the content of the stylesheet itself as such, but the content of the WeCSS loader/handler barfing over we::something. I don't know anywhere nearly enough to even begin debugging.
Re: zip class. Well, the choices are simple.
1) We don't have add plugin or auto update at all.
2) I add a 199KB class that is beyond ugly.
3) I finish writing my own class that does the job. It'll be usable when finished. The thing is it's a complete Zip class, not just the minimum required for extracting files. If I thought that was acceptable, I'd do that, but I can see the benefits of a full zip class that I can redistribute separately as well.
Also: with what I'm working on, I'm going to be adding a copy of jQuery UI. Primarily for the sortable unit but it seems that for the admin panel we could just include and be done with it. I don't have the patience or mindset to sit and write my own sortable/draggable component right now, I'd rather just run with a proven and stable one for the time being but if anyone wants to write a newer, less bulky one, go nuts. Just don't break anything :P
-
Just as a quick footnote, I realised that in actuality a zip creation library is not really needed - we already have a limited one as it is. So I'm just going to shave that stuff off and concentrate on it being a read library as I originally intended. Much closer to 18KB than 80KB.
-
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 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 :/
Anyway, I can assure you that it works in all situations I tried.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)
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...
-
- 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()
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...
Classes aren't just a specific way of breaking functionality up, they're also there for protecting class parameters from outside the class. For example, the template list can only be modified from within the wetem class. In the case of user information and permissions, I'd argue that we::* should be keeping that as a private member if it isn't already, because there is no reason the permissions need to be exposed outside the we object except for allowedTo(), and arguably that should be a method of we::* anyway, as should loading permissions really.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:
-
Also, I looked at r1825. Thanks for fixing my typo :)
As to the other changes regarding the we object, the change of __clone, yes, technically it can be a void method though returning false is more 'correct'.
As to the other loading, it doesn't matter whether we::* is defined or not, because that's not really the problem. The problem is that the permissions haven't been loaded yet, and we::* doesn't deal with permissions anyway other than storing them. There should never be any reason to check permissions prior to permissions being loaded, other than the fact topic privacy is currently fatally broken.
I think I'm going to have to implement what I said about fixing permissions before, and restructuring it entirely. Ugh, not looking forward to that, but I guess it's got to be done. Let me tackle that job, because I know what I have in mind, and it has a lot more consequences than just the loading order.
-
Singleton's __clone can also be private, just nuke the entire thing :P.
-
Yup, I'm aware of that. They are going to be cleaned up in due course when I finish the rest of the UI. Mostly it's going to be pushed to the ManageBans file, the only stuff actually needed in the main admin language file is stuff that is needed for the main admin menu itself.
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...Well, it's sort of required, if you're not calling getInstance(), you need to do something else to instantiate it and its content. Just loading it isn't enough.
So if you don't get the instance, how exactly does it get any of its content? Just loading it is not enough.
For example, the template list can only be modified from within the wetem class.
In the case of user information and permissions, I'd argue that we::* should be keeping that as a private member if it isn't already, because there is no reason the permissions need to be exposed outside the we object except for allowedTo(), and arguably that should be a method of we::* anyway, as should loading permissions really.
I've been tempted more than once to rewrite that in some fashion to clean it up, it's mostly a thinly veiled port of how it was done before.
I give you http://www.whatsmyip.org/
That's mostly just a mental lapse. It's hard work for me to consciously use bastardised English.
Anyway! Can you check it out?Also, I looked at r1825. Thanks for fixing my typo :)
As to the other changes regarding the we object, the change of __clone, yes, technically it can be a void method though returning false is more 'correct'.
There should never be any reason to check permissions prior to permissions being loaded, other than the fact topic privacy is currently fatally broken.
-
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.
It's actually what convinced me that objects can sometimes be more beneficial than procedural programming
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.
Anyway! Can you check it out?
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...
Do 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?!)
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 don't know how you're calling wedb...
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.That's the thing... if it is a object with static variables, the variables don't exist until they are set as such.
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.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.
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?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)
-
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.
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.
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?
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.
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.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...?
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.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.
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.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.
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.
-
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.
-
And it works really... Just rename __construct() to getInstance(), and remove the other getInstance() function... Voilà, it works perfectly that way too.
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.)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.
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 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).
- 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: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.
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..?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::*.
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?
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.
-
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:
A plugin calling for another instance of an object?
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"
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'.
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.
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...)
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
But permission error = security error, right...? Doesn't that warrant an immediate hack fix?
But this has been present for months, it's present on this very site right now for all users who are not you. The difference is, it doesn't have any visible sign that it's happening here, there's nothing going to the error log. It's the same class of error that you could stuff a @ in front of, knowing that it's probably going to fail and you don't care if it does. And there are times when that's acceptable; there's plenty of cases in wextr where I've done that, with the expectation that it's going to fail, but I didn't want any kind of error being thrown by PHP when I will be cleaning it up myself.
In this particular case, though, putting a @ in it will stop the error being thrown but it won't actually fix the problem. It's not like the error must always be thrown and there's no way to fix that; I used it with extreme prejudice in wextr because I'm dealing with fopen/fread etc, where they're relying on things outside of Wedge's control to behave, but here, everything is under Wedge's control and there is no reason why we shouldn't fix it once and for all - properly.
The danger of doing a hack is much like putting a band-aid on a problem; it doesn't fix the problem. It just prevents it getting obviously worse. What we should be doing is fixing the root problem instead.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.