brion@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?
Roan Kattouw (Catrope)
Roan Kattouw wrote:
brion@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)
Brion Vibber schreef:
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.
I didn't take it that way.
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.
Those local variables are scattered all over the function. How about having a hook for each of them? (A hook to interpret the NS, one to interpret the interwiki prefix, one to generate the DB key, etc.) That way we could still keep the hook you reverted and rename it to TitleSecureAndSplitDbKey.
Roan Kattouw (Catrope)
I was going to comment on the documentation for this, or rather lack thereof: "At the end of Title::secureAndSplit()". To begin with this is kind of pointless (if they knew what secureAndSplit() did, they'd have read the function and seen the hook already). But looked at another way, this is maybe the same sort of issue as Brion complained about. Hooks should have *logical* functions, they should not just be stuck somewhere in the code. Maybe tomorrow someone is going to rewrite part of Title.php and secureAndSplit() won't exist. When adding a hook, the questions to ask are
1) What functionality do I want the hook to provide to extensions? 2) Where does the hook need to be put to provide that functionality, in current code?
*in that order*. Otherwise you end up with hooks that aren't as useful to extension writers (they don't provide coherent or obvious functionality, and must possibly be looked up manually in the source code), and are prone to become weird when the code structure changes. Look at the two lonely userCan hooks embedded in the rewritten getUserPermissionsErrors(), which do almost the same thing but were formerly in different functions, so they have different interfaces.
Although, the documentation aspect is still significant. Compare it to the description of the hook right below: "called when showing a revision in Special:Undelete". That tells you exactly what it does, based on the hook's functionality, with no need to look at or know the first thing about the source code (well, except that you need to be familiar with the public interfaces of the objects you're given, obviously).
i applied a second patch for Ticket 13166.
i moved the discussion there. Maybe it's a bit overkill to post it here again.
https://bugzilla.wikimedia.org/show_bug.cgi?id=13166
I would call the Hook (i think one hook is absolutely appropriate) 'AlternativeTitleDBKey'
i defend all this on the bugtracker, so i am happy about your comments on what i wrote there.
Thanks in advance for all the input you gave me. i am happy for suggestions.
see you all josh
wikitech-l@lists.wikimedia.org