After I found the cause of a specific extension interaction bug, brion pointed out that this was a common error in extensions, that is functions attached to hooks that do not return a value. Such functions previously stopped all other functions attached to the hook from running - hence the interaction issues.
Per r23133, any function that is called by a hook and does not return a value will through a full stop error. This should encourage new extension writers to use cleaner functions that avoid this, however, I noticed a lot extension functions didn't return anything. I grepped through and tried to fix them all, at least the ones on SVN, but someone else may want to doublecheck, especially for the ones one WikiMedia sites (mainly http://en.wikipedia.org/wiki/Special:Version).
Per r23133, any function that is called by a hook and does not return a value will through a full stop error.
Since not returning anything in PHP is equivalent (more or less) to returning null, is there any concern that the extension writer's intent was to do so? Or is this simply a massive case of forgotten "return true" statements?
I totally agree that the the forgetting of return true is a big problem - I've done it myself. I'm just not fully convinced that throwing a full stop is the best answer. (I'd like to hear what others think on the matter).
-- Jim R. Wilson (jimbojw)
On 6/20/07, Voice of All jschulz_4587@msn.com wrote:
After I found the cause of a specific extension interaction bug, brion pointed out that this was a common error in extensions, that is functions attached to hooks that do not return a value. Such functions previously stopped all other functions attached to the hook from running - hence the interaction issues.
Per r23133, any function that is called by a hook and does not return a value will through a full stop error. This should encourage new extension writers to use cleaner functions that avoid this, however, I noticed a lot extension functions didn't return anything. I grepped through and tried to fix them all, at least the ones on SVN, but someone else may want to doublecheck, especially for the ones one WikiMedia sites (mainly http://en.wikipedia.org/wiki/Special:Version). -- View this message in context: http://www.nabble.com/Hooked-function-return-values-tf3955943.html#a11224995 Sent from the Wikipedia Developers mailing list archive at Nabble.com.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 6/20/07, Jim Wilson wilson.jim.r@gmail.com wrote:
Since not returning anything in PHP is equivalent (more or less) to returning null, is there any concern that the extension writer's intent was to do so? Or is this simply a massive case of forgotten "return true" statements?
I believe it's "simply a massive case of forgotten 'return true' statements."
I totally agree that the the forgetting of return true is a big problem -
I've done it myself. I'm just not fully convinced that throwing a full stop is the best answer. (I'd like to hear what others think on the matter).
Personally, I find this to be a good solution. I've lately become quite used to fail-fast languages such as java, and I've found that such eager exception throwing *really* helps you to not make such simple mistakes that result in hours of debugging. It's much better to have something explode on you than have programs that pretend to work, but with subtle and hard-to-find bugs.
I frankly can't think of a situation where someone would *need* to return null--at least, I've not seen a case of it yet; I'm sure in cases where you want to return null, you can get by fine with returning true. Could you perchance think of an example where this would be necessary?
Could you perchance think of an example where this would be necessary?
There are times when killing the stack is intended. It doesn't have to be "null" per se - but something that resolves to false in a PHP boolean context.
One example (off the top of my head) would be "userCan". Say you're writing an extension that's supposed to limit access to something. It may be your extension's intent to set the $result to false and then actually return false so no other more lenient extension has a chance to display the protected resource.
There are other cases I've run into, but I'd have to look in the codebase for them. Pretty much whereever you see something like this:
if ( !wfRunHooks( 'SomethingOrOther', array( &$this, &anotherThing ) ) { // whatever }
Tends to indicate a case where an extension might rightly choose to return false on purpose.
-- Jim
On 6/20/07, Daniel Cannon cannon.danielc@gmail.com wrote:
On 6/20/07, Jim Wilson wilson.jim.r@gmail.com wrote:
Since not returning anything in PHP is equivalent (more or less) to returning null, is there any concern that the extension writer's intent was to do so? Or is this simply a massive case of forgotten "return true" statements?
I believe it's "simply a massive case of forgotten 'return true' statements."
I totally agree that the the forgetting of return true is a big problem -
I've done it myself. I'm just not fully convinced that throwing a full stop is the best answer. (I'd like to hear what others think on the matter).
Personally, I find this to be a good solution. I've lately become quite used to fail-fast languages such as java, and I've found that such eager exception throwing *really* helps you to not make such simple mistakes that result in hours of debugging. It's much better to have something explode on you than have programs that pretend to work, but with subtle and hard-to-find bugs.
I frankly can't think of a situation where someone would *need* to return null--at least, I've not seen a case of it yet; I'm sure in cases where you want to return null, you can get by fine with returning true. Could you perchance think of an example where this would be necessary?
-- Daniel Cannon (AmiDaniel)
http://amidaniel.com cannon.danielc@gmail.com _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Jim Wilson wrote:
Could you perchance think of an example where this would be necessary?
There are times when killing the stack is intended. It doesn't have to be "null" per se - but something that resolves to false in a PHP boolean context.
Then, why not return false instead of null? :-)
On 21/06/07, Jim Wilson wilson.jim.r@gmail.com wrote:
One example (off the top of my head) would be "userCan". Say you're writing an extension that's supposed to limit access to something. It may be your extension's intent to set the $result to false and then actually return false so no other more lenient extension has a chance to display the protected resource.
In which case, one would return false.
Rob Church
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Jim Wilson wrote:
Per r23133, any function that is called by a hook and does not return a value will through a full stop error.
Since not returning anything in PHP is equivalent (more or less) to returning null, is there any concern that the extension writer's intent was to do so? Or is this simply a massive case of forgotten "return true" statements?
A hook function (and I mean specifically those run via wfRunHooks()) can return three possible values:
true - continue running hooks, and return 'true' from wfRunHooks() false - cancel all remaining hooks, and return 'false' from wfRunHooks() a string - display the string as an error message, and return 'false' from wfRunHooks()
NULL is not a valid return value, but it's not a string and it evaluates to false when checked in boolean context. Since true is what is intended 99% of the time, the NULL default return breaks things.
There's three ways to handle that: 1) Ignore it -- what we did before ;) 2) Toss a big error message so it's obvious it needs to be fixed -- what I just added to try it out 3) Change it to treat as if you returned true instead of false -- what Tim suggests as the most convenient behavior
My one concern about 3) is that it may encourage continuing breakage in extensions that try to run across multiple versions, since they'd have to remember to return true for older versions.
- -- brion vibber (brion @ wikimedia.org)
Thanks for chiming in everyone. Hopefully this new behavior will save some aggravating extension-conflict headaches, and it seems the consensus is that not-returning a value should always be considered a bug in the extension.
Determining whether it was intended to return true or false or a String would have to be based on what the extension was trying to achieved, but in most cases it was probably just a forgotten "return true".
Brion wrote:
A hook function (and I mean specifically those run via wfRunHooks()) can return three possible values:
Since NULL is not an acceptable value, throwing a full-stop seems appropriate (to me). This certainly would have saved me some debugging time in the past :)
Brion wrote:
My one concern about 3) is that it may encourage continuing breakage in extensions that try to run across multiple versions, since they'd have to remember to return true for older versions.
I agree, and I'd add that coaxing NULL values into true may also cause subtle breakages where one hooking method "expects" another hooking method (in another extension) to have killed the wfRunHooks() stack due to the original behavior. Granted this is an edge case, and most likely indicates a bug in one or both extensions.
As a writer of multi-version compatible extensions, I appreciate steps taken to facilitate BC.
-- Jim
On 6/21/07, Brion Vibber brion@wikimedia.org wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Jim Wilson wrote:
Per r23133, any function that is called by a hook and does not return a value will through a full stop error.
Since not returning anything in PHP is equivalent (more or less) to returning null, is there any concern that the extension writer's intent
was
to do so? Or is this simply a massive case of forgotten "return true" statements?
A hook function (and I mean specifically those run via wfRunHooks()) can return three possible values:
true - continue running hooks, and return 'true' from wfRunHooks() false - cancel all remaining hooks, and return 'false' from wfRunHooks() a string - display the string as an error message, and return 'false' from wfRunHooks()
NULL is not a valid return value, but it's not a string and it evaluates to false when checked in boolean context. Since true is what is intended 99% of the time, the NULL default return breaks things.
There's three ways to handle that:
- Ignore it -- what we did before ;)
- Toss a big error message so it's obvious it needs to be fixed -- what
I just added to try it out 3) Change it to treat as if you returned true instead of false -- what Tim suggests as the most convenient behavior
My one concern about 3) is that it may encourage continuing breakage in extensions that try to run across multiple versions, since they'd have to remember to return true for older versions.
- -- brion vibber (brion @ wikimedia.org)
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2.2 (Darwin) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGeoFlwRnhpk1wk44RAribAJ411AwRj2LqgfUct253pKBzhF5tGQCfT6xF BIF13UvzRxxvvoMz8kS0fb0= =wU7j -----END PGP SIGNATURE-----
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Voice of All wrote:
After I found the cause of a specific extension interaction bug, brion pointed out that this was a common error in extensions, that is functions attached to hooks that do not return a value. Such functions previously stopped all other functions attached to the hook from running - hence the interaction issues.
Per r23133, any function that is called by a hook and does not return a value will through a full stop error. This should encourage new extension writers to use cleaner functions that avoid this, however, I noticed a lot extension functions didn't return anything. I grepped through and tried to fix them all, at least the ones on SVN, but someone else may want to doublecheck, especially for the ones one WikiMedia sites (mainly http://en.wikipedia.org/wiki/Special:Version).
I would prefer to have a null return value treated the same as a true return value. A true return value is almost always what is needed, it seems to me to make sense to allow this shortcut.
-- Tim Starling
Voice of All schrieb:
Per r23133, any function that is called by a hook and does not return a value will through a full stop error. This should encourage new extension writers to use cleaner functions that avoid this, however, I noticed a lot extension functions didn't return anything. I grepped through and tried to fix them all, at least the ones on SVN, but someone else may want to doublecheck, especially for the ones one WikiMedia sites (mainly http://en.wikipedia.org/wiki/Special:Version).
Getting an error with Quiz:
Invalid NULL return from broken hook resetQuizID::resetQuizID
Raymond.
Fixed. Didn't seem to have that extension so I had to SVN check it out.
Raimond Spekking-2 wrote:
Voice of All schrieb:
Per r23133, any function that is called by a hook and does not return a value will through a full stop error. This should encourage new extension writers to use cleaner functions that avoid this, however, I noticed a lot extension functions didn't return anything. I grepped through and tried to fix them all, at least the ones on SVN, but someone else may want to doublecheck, especially for the ones one WikiMedia sites (mainly http://en.wikipedia.org/wiki/Special:Version).
Getting an error with Quiz:
Invalid NULL return from broken hook resetQuizID::resetQuizID
Raymond.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Voice of All schrieb:
Per r23133, any function that is called by a hook and does not return a value will through a full stop error. This should encourage new extension
Another one:
Invalid NULL return from broken hook efUpdateCheckUserData
Backtrace:
#0 F:\xampp\htdocs\wikitest\includes\RecentChange.php(210): wfRunHooks() #1 F:\xampp\htdocs\wikitest\includes\RecentChange.php(426): RecentChange->save('RecentChange_sa...', Array) #2 F:\xampp\htdocs\wikitest\includes\LogPage.php(84): RecentChange->notifyLog() #3 F:\xampp\htdocs\wikitest\includes\LogPage.php(239): LogPage->saveContent('20070621100458', Object(Title), Object(User), '??nderte den Sc...', '', 'protect', 'modify', Object(Title), '[1] [edit=sysop...', '') #4 F:\xampp\htdocs\wikitest\includes\Article.php(1765): LogPage->addEntry() #5 F:\xampp\htdocs\wikitest\includes\ProtectionForm.php(197): Article->updateRestrictions('modify', Object(Title), '[1] [edit=sysop...') #6 F:\xampp\htdocs\wikitest\includes\ProtectionForm.php(91): ProtectionForm->save(Array, '[1]', true, 'infinity') #7 F:\xampp\htdocs\wikitest\includes\Article.php(1643): ProtectionForm->execute() #8 F:\xampp\htdocs\wikitest\includes\Wiki.php(392): Article->protect() #9 F:\xampp\htdocs\wikitest\includes\Wiki.php(48): MediaWiki->performAction() #10 F:\xampp\htdocs\wikitest\index.php(89): MediaWiki->initialize(Object(StubObject), Object(Article), Object(Title), Object(User), Object(WebRequest)) #11 {main}
Raymond.
Raimond Spekking schrieb:
Another one:
Invalid NULL return from broken hook efUpdateCheckUserData
Sorry, forget this one, my error, this installation was partly svn upd only :-(
Raymond.
wikitech-l@lists.wikimedia.org