On 9/9/07, werdna@svn.wikimedia.org werdna@svn.wikimedia.org wrote:
Revision: 25680 Author: werdna Date: 2007-09-09 08:11:58 +0000 (Sun, 09 Sep 2007)
Log Message:
- Allow userCan to take null $user - and replace it with $wgUser if passed.
. . .
First of all, why? We should be cutting down the use of $wgUser as much as possible, not adding it in extra places. The more methods that are free of any given global, the better. I'm not the only one who dreams fondly of elimination of things like $wgUser altogether. If someone forgets a function argument, they can get a fatal error and take two minutes to fix it.
Second of all, it doesn't even work, since you made the assignment after the global declaration. "Fatal error: Call to a member function isAllowed() on a non-object in /var/www/trunk/phase3/includes/Title.php on line 1187"
And while I was looking at why $wgUser was declared at all, I came across this, which I'm pretty sure is wrong:
$blockTimestamp = $wgLang->timeanddate( wfTimestamp( TS_MW, $wgUser->mBlock->mTimestamp ), true );
Every other reference in the block of code is to $user->mBlock, not $wgUser->mBlock.
On 9/10/07, Simetrical Simetrical+wikilist@gmail.com wrote:
[snip]
All suggested changes made in r25712.
Simetrical wrote:
On 9/9/07, werdna@svn.wikimedia.org werdna@svn.wikimedia.org wrote:
Revision: 25680 Author: werdna Date: 2007-09-09 08:11:58 +0000 (Sun, 09 Sep 2007)
Log Message:
- Allow userCan to take null $user - and replace it with $wgUser if passed.
. . .
First of all, why? We should be cutting down the use of $wgUser as much as possible, not adding it in extra places. The more methods that are free of any given global, the better. I'm not the only one who dreams fondly of elimination of things like $wgUser altogether. If someone forgets a function argument, they can get a fatal error and take two minutes to fix it.
I think $wgUser is the last global we should get rid of, if we get rid of it at all. I later regretted inventing hideous syntax such as:
$popt = ParserOptions::newFromUser( $wgUser );
...now you can do the same thing with just
$popt = new ParserOptions;
I did it with a user parameter with a default of null.
The problem is that the existence of a single global user is implied in many places. You don't gain any flexibility by having the "global $wgUser" in the caller instead of the callee. And in 99% of code there is no application for multiple user objects.
You could gain some flexibility by bundling variables like $wgUser, $wgRequest and the configuration variables into a single context object, and then using that context object as a factory for subsidiary objects. So you would have:
$popt = $mw->newParserOptions();
or in the userCan case:
$title = $mw->newTitle($titleText); $title->getUserPermissionsErrors( 'view' );
It still assumes a single fixed user object as part of an execution context, but it allows subclassing of core objects and comprehensive namespace separation to be implemented transparently to the caller. It also allows straightforward context generation and cloning, which gives you some amount of flexibility if you need to go outside the usual use case.
-- Tim Starling
On 9/9/07, Tim Starling tstarling@wikimedia.org wrote:
I think $wgUser is the last global we should get rid of, if we get rid of it at all.
I think it's closer to the first, really . . .
The problem is that the existence of a single global user is implied in many places.
Not really. Or rather, any given context might imply one single user, but that user need not be the same as in every other context up and down the stack and in all past and future iterations within that request.
You don't gain any flexibility by having the "global $wgUser" in the caller instead of the callee.
Sure you do, because other callers can use other Users.
And in 99% of code there is no application for multiple user objects.
What's a method where there could be no possible benefit from allowing it to deal with users other than the one generating the web request (if applicable)?
You could gain some flexibility by bundling variables like $wgUser, $wgRequest and the configuration variables into a single context object, and then using that context object as a factory for subsidiary objects. So you would have:
$popt = $mw->newParserOptions();
or in the userCan case:
$title = $mw->newTitle($titleText); $title->getUserPermissionsErrors( 'view' );
It still assumes a single fixed user object as part of an execution context, but it allows subclassing of core objects and comprehensive namespace separation to be implemented transparently to the caller. It also allows straightforward context generation and cloning, which gives you some amount of flexibility if you need to go outside the usual use case.
Well, at the very least a "bundle" object would be needed if we were to kill all the globals. You aren't going to pass the request object, user object, article object, etc. up the stack separately. Adding factory methods to it as you propose is an interesting idea, but it doesn't seem useful as such: it seems to be identical in function except for the nonstandard syntax, basically. Subclassing should work somewhat for extensions, but there are sharp limits to that when you have multiple extensions working together. If two extensions each replace context objects at various points in the code with their own derived objects that they need to function, it's potluck which context object will be handy when it next comes back to the extension.
Simetrical wrote:
On 9/9/07, Tim Starling tstarling@wikimedia.org wrote:
[...]
You don't gain any flexibility by having the "global $wgUser" in the caller instead of the callee.
Sure you do, because other callers can use other Users.
And in 99% of code there is no application for multiple user objects.
What's a method where there could be no possible benefit from allowing it to deal with users other than the one generating the web request (if applicable)?
Methods don't exist in a vacuum, they exist to support certain public user interface operations, such as:
* Page view * Edit * Upload * RC * Watchlist * Login
Out of those six operations, only edit requires dealing with multiple user objects, and only then in a small, self-contained file (UserMailer). Login briefly has two user objects in existence, but for the most part, it works by changing the global context.
If I may turn the question around, what do you envisage to be the applications for, say, the $user parameter to ParserOptions::__construct()?
-- Tim Starling
On 9/11/07, Tim Starling tstarling@wikimedia.org wrote:
Methods don't exist in a vacuum, they exist to support certain public user interface operations, such as:
- Page view
- Edit
- Upload
- RC
- Watchlist
- Login
And:
* Anything else anyone thinks of at some point in the future
Flexibility isn't important if you consider a fixed set of operations, obviously.
If I may turn the question around, what do you envisage to be the applications for, say, the $user parameter to ParserOptions::__construct()?
Well, first I'll give you a nice concrete example where I was bitten by this trying to write an extension for another piece of software. I wanted to write a plugin for vBulletin that would read through each post made, and if it contained a word on a blacklist, report it to the moderators. One of the problems I encountered was that the existing code for reporting posts relied on globals, and assumed that the only person who could possibly be reporting posts would be the global user. But here, I didn't want the person to show up as reporting their own post, I had a pseudo-account set up for the purpose. If I had a function like report( $reporter, $postid, $reason ), it would have been entirely simple. As it is, I never got the plugin to work to this day.
For ParserOptions, of course, you might want to render the page according to some different set of options than the global user has. Not terribly likely, but then that's not the best example.
On 9/11/07, Thomas Dalton thomas.dalton@gmail.com wrote:
I don't see what is to be gained by bundling them together, either, it just replaces lots of small globals with one big one - surely that's just a waste of resources?
If they're no longer globals, they need to be bundled together into a single variable for the most part. Otherwise you'll have to constantly update various methods' call lines when you realize you now want to refer to some property of the user in some method or other that wasn't previously passed a user object. Globals do simplify call lines, because they're implicitly passed to everything, as it were.
If they're no longer globals, they need to be bundled together into a single variable for the most part. Otherwise you'll have to constantly update various methods' call lines when you realize you now want to refer to some property of the user in some method or other that wasn't previously passed a user object. Globals do simplify call lines, because they're implicitly passed to everything, as it were.
And that single variable would be global, surely, otherwise the call lines would still have to be updated. One large global is surely much less efficient that just calling a few small globals when you need them.
On 9/12/07, Thomas Dalton thomas.dalton@gmail.com wrote:
And that single variable would be global, surely, otherwise the call lines would still have to be updated.
It would be local, otherwise there's no point. The call lines would still have to be updated, but only once.
It would be local, otherwise there's no point. The call lines would still have to be updated, but only once.
If every call line is updated to pass the bundle, then it's effectively a home-made global - what's the point of that?
I think $wgUser is the last global we should get rid of, if we get rid of it at all. I later regretted inventing hideous syntax such as:
$popt = ParserOptions::newFromUser( $wgUser );
...now you can do the same thing with just
$popt = new ParserOptions;
I did it with a user parameter with a default of null.
Why not write a User::getParserOptions() method? Static methods are generally "hideous", as you put it. If the options are a property of the user (which, conceptually they are), why not make getting them a method of the user?
I think the current user needs to be a global and it should be used by the caller. Just because 99.9% of the time the method is going to be called with $wgUser doesn't mean we shouldn't support the 1 time in a 1000 when it isn't. Remember, these methods are used by extensions, not just the core code, so we really have no idea what people are going to want to do with them. I don't see what is to be gained by bundling them together, either, it just replaces lots of small globals with one big one - surely that's just a waste of resources?
wikitech-l@lists.wikimedia.org