Roan Kattouw wrote:
brion(a)svn.wikimedia.org schreef:
Revision: 31519
Author: brion
Date: 2008-03-03 19:54:22 +0000 (Mon, 03 Mar 2008)
Log Message:
-----------
Revert r31505, r31510 (secureAndSplit hook).
This hook doesn't seem well thought out; a hook isn't given clean access to the
various possible fields it's likely to want to manipulate, and generally feels a bit
rushed.
Care to improve this rather than just taking it back out?
Let me start off with a note -- my responsibility is to keep the code
clean and functional and moving forward.
If something is added that I don't believe is as good as it should be,
backing it out until it's been improved is a perfectly normal and
virtuous bit of code maintenance, whether I'm the one who improves it or
not.
We give out commit access liberally not because everyone has a right to
have their whims included in the trunk code, but because it streamlines
the process -- it's usually a lot easier to let 90% good commits go in
and revert or fix the 10% that need more work than to review 100% of
patches beforehand.
When that happens, it's not an attack on the committer, nor on the
submitter of the patch. No malice is intended; no punishment is meted out.
Hell, even I get reverted sometimes. ;)
Now, to some notes specifically about that function --
The basic problem with the hook as written was that it passed in only
one of the many relevant bits of a title -- the dbkey portion. There's
also a namespace, an interwiki prefix, and a fragment; they would only
be manipulable to the hook by poking into internal members of the title
object, which isn't a clean interface and could lead to compatibility
problems in the future.
More generally, Title::secureAndSplit() is itself some pretty ugly code.
The way it manipulates its various bits isn't very clean, and doesn't
lend itself as well as it might to passing it all off to a subhook --
some variables are kept in the function, others directly manipulate
private object properties.
It might be desirable to do some refactoring within the function, then
pass off all relevant internal variables to the hook.
-- brion vibber (brion @
wikimedia.org)