Within mediawiki there is a split between returning false/null and throwing exceptions. There is also the Status class used by the wikitext parser(the Status class is somewhat closely tied to the parser reducing reusability though). essentially there are 3 kinds of error handling used within mediaiki.
We talked amongst the Flow team and agreed that we prefer false/null over exceptions, mostly due to issues where exceptions can short-circuit the expected execution path just about everywhere in a non-obvious manner(a big enough issue that java uses checked exceptions, another world of pain). During code review we spend a reasonable amount of time just ensuring that functions that can return false/null are actually checked.
Moving forward, the Flow team is considering using a php implementation that follows the ideas of the haskell Maybe monad( https://github.com/schmittjoh/php-option ). This is, in concept, rather similar to the Status class the wikitext parser returns. We would like to use this library as a way to more explicitly handle error situations and reduce the occurrences of forgetting to check false/null. This particular pattern is very common in Functional languages.
I do believe this method of error handling is friendlier to programmers memory, easier to code review, and more explicit about what happens in the error condition. Are there any concerns with the Flow project moving forward and utilizing this as our primary error handling mechanism rather than returning false/null?
Erik B.
On Mon, Oct 7, 2013 at 2:20 PM, Erik Bernhardson <ebernhardson@wikimedia.org
wrote:
Are there any concerns with the Flow project moving forward and utilizing this as our primary error handling mechanism rather than returning false/null?
What's wrong with just using the Status class itself? And depending on the answer to that question, would it be possible to improve the Status class in core first, and then use the improved version. That way everybody can use this new preferred error handling structure.
Also, just to be clear, while I do like this method of handling better (being a big fan of the Status class myself), it is not a complete replacement for exceptions. As mentioned, exceptions can be annoying because they just randomly traverse up the call stack until they're hopefully caught. However, sometimes (only sometimes) this is intentional. For example, when a SpecialPage throws an ErrorPageError, which just bypasses all logic and shows a nice error page. Usually errors that are fatal to the request, i.e., once they occur the request can no longer continue, work well with exceptions.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science
Hey,
Returning error codes and null force then handler to deal the error case. Often this is for no good reason, and often the handler cannot decide how to deal with such an error, and is forced to simply pass it on. In such cases, it is just silly to have in-band error handling. This is what exceptions are for.
The linked library has documentation that describes some of the problems of using such return values. Based on those docs, it seems like a good way to get around these problems for the null/false case. So if the choice is between returning null/false or using this library, I'd at least give the library a try.
Some further remarks:
* Exceptions should not be used for control flow of non-error cases * Returning error codes is problematic and is not solved by this library, Exceptions should typically be used
Remarks on the usage of this library:
* Is a WMF team really going to use a third-party library distributed via Composer? :) * It has over half a million downloads on Packagist, so is probably pretty solid * It has tests and runs them on TravisCI \o/
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --
On Mon, Oct 7, 2013 at 11:46 AM, Jeroen De Dauw jeroendedauw@gmail.comwrote:
- Is a WMF team really going to use a third-party library distributed via
Composer? :)
We use lots of libraries that happen to use composer. We just don't use composer to deploy them.
-Chad
Hey,
We use lots of libraries that happen to use composer. We just don't
use composer to deploy them.
Oh? Lots? Is there a list somewhere? Are most of those libraries forked? Are a good portion of them semi-assimilated into core? I hope the answer to the later two is "no".
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --
On Mon, Oct 7, 2013 at 12:12 PM, Jeroen De Dauw jeroendedauw@gmail.comwrote:
Hey,
We use lots of libraries that happen to use composer. We just don't
use composer to deploy them.
Oh? Lots? Is there a list somewhere? Are most of those libraries forked? Are a good portion of them semi-assimilated into core? I hope the answer to the later two is "no".
In order:
No. Yes, but because we don't deploy from things that aren't in gerrit. No.
-Chad
On Mon, Oct 7, 2013 at 3:12 PM, Jeroen De Dauw jeroendedauw@gmail.com wrote:
Hey,
We use lots of libraries that happen to use composer. We just don't
use composer to deploy them.
Oh? Lots? Is there a list somewhere? Are most of those libraries forked? Are a good portion of them semi-assimilated into core? I hope the answer to the later two is "no".
I believe the procedure is to set up a clone of them on gerrit, include them as a submodule, and then do *something* to make the classes autoload. Updating from upstream should be a mater of pulling the upstream update locally, pushing to gerrit, updating the submodule pointer, and making sure the autoloading still makes sense. In some respects it is a very convenient way to do things. In others, not so much.
There isn't a list, they are scattered among the mediawiki extensions in gerrit.
I'm not defending it, but I can see why we do it.
Nik
On Mon, Oct 7, 2013 at 2:20 PM, Erik Bernhardson ebernhardson@wikimedia.org wrote:
Are there any concerns with the Flow project moving forward and utilizing this as our primary error handling mechanism rather than returning false/null?
In other words, the best solution to the problem "we have three different ways to handle error reporting" is probably not "let's add another way to handle error reporting!"
What are the actual issues with Status, and how is this proposal different?
Hey,
What are the actual issues with Status, and how is this proposal
different?
The Status object is many ways is a fancy error code. It causes the same issues as returning error codes. It differs with the proposal in that the Status object deals with error cases, while the proposal does not.
Cheers
-- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. ~=[,,_,,]:3 --
On Mon, Oct 7, 2013 at 3:17 PM, Jeroen De Dauw jeroendedauw@gmail.comwrote:
The Status object is many ways is a fancy error code. It causes the same issues as returning error codes. It differs with the proposal in that the Status object deals with error cases, while the proposal does not.
The real question is why can't the Status object just be supplemented to support this functionality. Like Brad said, it's really not a good idea to introduce yet another method of error handling.
Also, the Status class does achieve more than just being a fancy error code. It allows clear distinction between return values and error values, which is something not always guaranteed by the "return null/false" strategy.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science
Out of curiosity, what's an actual example of code where the execution flow of exceptions is significantly more surprising than the execution flow of a billion manual checks to avoid "Fatal error: Call to a member function foo() on a non-object"?
I've heard the vague claim that exceptions are confusing for years, but for the life of me I've never seen exception-handling code that looked more complex or confusing than code riddled with checks for magic return values.
-- brion
On Mon, Oct 7, 2013 at 11:20 AM, Erik Bernhardson < ebernhardson@wikimedia.org> wrote:
Within mediawiki there is a split between returning false/null and throwing exceptions. There is also the Status class used by the wikitext parser(the Status class is somewhat closely tied to the parser reducing reusability though). essentially there are 3 kinds of error handling used within mediaiki.
We talked amongst the Flow team and agreed that we prefer false/null over exceptions, mostly due to issues where exceptions can short-circuit the expected execution path just about everywhere in a non-obvious manner(a big enough issue that java uses checked exceptions, another world of pain). During code review we spend a reasonable amount of time just ensuring that functions that can return false/null are actually checked.
Moving forward, the Flow team is considering using a php implementation that follows the ideas of the haskell Maybe monad( https://github.com/schmittjoh/php-option ). This is, in concept, rather similar to the Status class the wikitext parser returns. We would like to use this library as a way to more explicitly handle error situations and reduce the occurrences of forgetting to check false/null. This particular pattern is very common in Functional languages.
I do believe this method of error handling is friendlier to programmers memory, easier to code review, and more explicit about what happens in the error condition. Are there any concerns with the Flow project moving forward and utilizing this as our primary error handling mechanism rather than returning false/null?
Erik B. _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Out of curiosity, what's an actual example of code where the execution flow of exceptions is significantly more surprising than the execution flow of a billion manual checks to avoid "Fatal error: Call to a member function foo() on a non-object"?
I've heard the vague claim that exceptions are confusing for years, but for the life of me I've never seen exception-handling code that looked more complex or confusing than code riddled with checks for magic return values.
My experience has been similar. I personally prefer exceptions as they at least let me know in a highly visible way when I forget to do something about them, unlike magic return values which may cause problems which do not surface right away if a check is forgotten.
Thank you, Derric Atzrott
On 07.10.2013, 23:45 Brion wrote:
Out of curiosity, what's an actual example of code where the execution flow of exceptions is significantly more surprising than the execution flow of a billion manual checks to avoid "Fatal error: Call to a member function foo() on a non-object"?
I've heard the vague claim that exceptions are confusing for years, but for the life of me I've never seen exception-handling code that looked more complex or confusing than code riddled with checks for magic return values.
+1
On Mon, Oct 7, 2013 at 3:45 PM, Brion Vibber bvibber@wikimedia.org wrote:
I've heard the vague claim that exceptions are confusing for years, but for the life of me I've never seen exception-handling code that looked more complex or confusing than code riddled with checks for magic return values.
When I'm writing Haskell nothing is more intuitive than the error monad because that is how the compiler works. When I'm writing Java nothing is more intuitive than exceptions because that is how the standard library works. When I'm writing Scala nothing is more intuitive than exceptions for unrecoverable errors and Option/Either for recoverable ones because that is how the standard library works. When I'm writing C I deal with magic return values, modified arguments, and errno because that is what libc burdens me with. When I'm writing PHP I deal with magic return values, modifiable arguments, and exceptions because that is what is in the standard library. Oh, yeah, and I deal with Status too, because we use it sometimes.
I don't see the point in adding another error handling mechanism beyond the ones you are stuck with in the standard library. It is just too much work to wrap the standard library over and over and over again.
Unless you are writing Javascript, then promises are too compelling.
Nik
So I took a quick look at https://github.com/schmittjoh/php-option -- it appears to be a wrapper object around queries that can throw an exception for you if the wrapped value is null (exactly like just using exceptions in the first place), or can check for nullness and replace it with an alternate value (equivalent to an if/then or try/catch block, just in a function call instead of a block construct).
At best, it's syntactic sugar and adds no expressiveness to the language. At worst, it complicates the codebase by adding another coding pattern that would be different from the rest of the codebase. I don't think this would be a particularly useful pattern to follow.
-- brion
On Mon, Oct 7, 2013 at 12:45 PM, Brion Vibber bvibber@wikimedia.org wrote:
Out of curiosity, what's an actual example of code where the execution flow of exceptions is significantly more surprising than the execution flow of a billion manual checks to avoid "Fatal error: Call to a member function foo() on a non-object"?
I've heard the vague claim that exceptions are confusing for years, but for the life of me I've never seen exception-handling code that looked more complex or confusing than code riddled with checks for magic return values.
-- brion
On Mon, Oct 7, 2013 at 11:20 AM, Erik Bernhardson < ebernhardson@wikimedia.org> wrote:
Within mediawiki there is a split between returning false/null and throwing exceptions. There is also the Status class used by the wikitext parser(the Status class is somewhat closely tied to the parser reducing reusability though). essentially there are 3 kinds of error handling used within mediaiki.
We talked amongst the Flow team and agreed that we prefer false/null over exceptions, mostly due to issues where exceptions can short-circuit the expected execution path just about everywhere in a non-obvious manner(a big enough issue that java uses checked exceptions, another world of pain). During code review we spend a reasonable amount of time just ensuring that functions that can return false/null are actually checked.
Moving forward, the Flow team is considering using a php implementation that follows the ideas of the haskell Maybe monad( https://github.com/schmittjoh/php-option ). This is, in concept, rather similar to the Status class the wikitext parser returns. We would like to use this library as a way to more explicitly handle error situations and reduce the occurrences of forgetting to check false/null. This particular pattern is very common in Functional languages.
I do believe this method of error handling is friendlier to programmers memory, easier to code review, and more explicit about what happens in the error condition. Are there any concerns with the Flow project moving forward and utilizing this as our primary error handling mechanism rather than returning false/null?
Erik B. _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
I started replying, but there are just way too many different opinions to reply to you all. IF the accepted method of handling errors in mediawiki is to throw an exception and cancel page render we will simply do that instead.
On Mon, Oct 7, 2013 at 2:09 PM, Brion Vibber bvibber@wikimedia.org wrote:
So I took a quick look at https://github.com/schmittjoh/php-option -- it appears to be a wrapper object around queries that can throw an exception for you if the wrapped value is null (exactly like just using exceptions in the first place), or can check for nullness and replace it with an alternate value (equivalent to an if/then or try/catch block, just in a function call instead of a block construct).
At best, it's syntactic sugar and adds no expressiveness to the language. At worst, it complicates the codebase by adding another coding pattern that would be different from the rest of the codebase. I don't think this would be a particularly useful pattern to follow.
-- brion
On Mon, Oct 7, 2013 at 12:45 PM, Brion Vibber bvibber@wikimedia.org wrote:
Out of curiosity, what's an actual example of code where the execution flow of exceptions is significantly more surprising than the execution
flow
of a billion manual checks to avoid "Fatal error: Call to a member
function
foo() on a non-object"?
I've heard the vague claim that exceptions are confusing for years, but for the life of me I've never seen exception-handling code that looked
more
complex or confusing than code riddled with checks for magic return
values.
-- brion
On Mon, Oct 7, 2013 at 11:20 AM, Erik Bernhardson < ebernhardson@wikimedia.org> wrote:
Within mediawiki there is a split between returning false/null and throwing exceptions. There is also the Status class used by the wikitext parser(the Status class is somewhat closely tied to the parser reducing reusability though). essentially there are 3 kinds of error handling used within mediaiki.
We talked amongst the Flow team and agreed that we prefer false/null
over
exceptions, mostly due to issues where exceptions can short-circuit the expected execution path just about everywhere in a non-obvious manner(a big enough issue that java uses checked exceptions, another world of pain). During code review we spend a reasonable amount of time just ensuring that functions that can return false/null are actually checked.
Moving forward, the Flow team is considering using a php implementation that follows the ideas of the haskell Maybe monad( https://github.com/schmittjoh/php-option ). This is, in concept,
rather
similar to the Status class the wikitext parser returns. We would like
to
use this library as a way to more explicitly handle error situations and reduce the occurrences of forgetting to check false/null. This
particular
pattern is very common in Functional languages.
I do believe this method of error handling is friendlier to programmers memory, easier to code review, and more explicit about what happens in
the
error condition. Are there any concerns with the Flow project moving forward and utilizing this as our primary error handling mechanism
rather
than returning false/null?
Erik B. _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Mon, Oct 7, 2013 at 2:45 PM, Erik Bernhardson <ebernhardson@wikimedia.org
wrote:
I started replying, but there are just way too many different opinions to reply to you all. IF the accepted method of handling errors in mediawiki is to throw an exception and cancel page render we will simply do that instead.
I think every response so far can be distilled into this:
"We already have three major error-reporting patterns: return null/false, throw exceptions, and return a Status object. Can you justify the idea of adding a fourth with strong benefits compared to the existing three?"
-- brion
On Mon, Oct 7, 2013 at 2:56 PM, Brion Vibber bvibber@wikimedia.org wrote:
"We already have three major error-reporting patterns: return null/false, throw exceptions, and return a Status object. Can you justify the idea of adding a fourth with strong benefits compared to the existing three?"
-- brion
Exceptions:
After re-reading my initial post i can see where some confusion might come in, I do not advocate banning exceptions, rather I advocate using exceptions only for exceptional circumstances. What constitutes an exceptional circumstance can vary from developer to developer, but if we agree that exceptions are only for exceptional circumstances then we agree exceptions are only a piece of the solution.
In addition to the above there is the question of using exceptions for control flow. At a past job our lead(who came from enterprise java) used to say "If the language wanted to give you a statically typed stack breaking return statement they would give you one. Don't hack it on top of Exceptions." I still agree with this, but as with what is an exceptional circumstance intelligent developers are allowed to disagree here.
return null/false:
If you buy into the notion that exceptions should be used for exceptional circumstances then you need something to return in those not so exceptional circumstances. The go to value's within php are either null or false depending on context. Within Flow we have both exceptions and null/false return values.
As lead developer on the Flow project I feel it is my responsibility to recognize and attempt to find solutions to recurring problems within the development team. We are semi-regularly finding newly submitted code that fails to handle the null/false case originating from both core and our own code. This is not a new problem unique to Flow, one of our developers mentioned that the reason he checks for them so much in reviews is that he had the same issue in his very first WMF patch. I think developers time could be better used than looking up all the function calls used in a patch to see how they handle errors.
I'm not trying to and don't expect anything to change in core, but I wanted to bring in battle tested code used by thousands of other developers to solve development problems within the extension we are writing.
Status class:
A review of the code within this class tells me that its primary use case(its single responsibility if you will) is to assist the caller in generating user facing error messages about an operation that can fail. If null/false is an acceptable return value then there is no reason to use this class.
If this were a smaller codebase and php-option was an accepted solution I would investigate(e.g. review all the places its used and see if its even solving the same problems) replacing the Status class usage with an extended version of php-option's None class that assists the caller in generating user facing error messages. But this is not a small codebase. This is a 10 year old codebase with over a million lines of code and many baked in assumptions. It would be, at best, ridiculous to go around breaking code that currently works.
So, then what?
As stated above we have a problem within Flow development. Developers are not consistently checking every possible source of null/false. We could just call it a developer error and tell us all to smarten up. But I think we have the opportunity to do better. A better error handling solution should be obvious when doing code review. A reviewer should be able to know if the error conditions are properly handled by looking at the new code, not by looking up all the function calls to see what they can possibly return.
I am not particularly tied to php-option. I do have a problem that I need to solve though. I have hopefully addressed why the existing error handling solutions within mediawiki are not solving this problem. I am assuming that if some null/false errors are regularly being caught in code review then others are sneaking through into master. I don't know how to solve these problems using the solutions currently in mediawiki so I looked to what is being done in the wider developer audience.
Moving Forward
I honestly don't know what to do moving forward. So far the plan is keep using null/false, and hope that we don't ruin too many wikipedian's day when some function has a non-exceptional error condition and blows up the extension.
Erik B.
On 08/10/13 14:40, Erik Bernhardson wrote:
A reviewer should be able to know if the error conditions are properly handled by looking at the new code, not by looking up all the function calls to see what they can possibly return.
This is why the recommended pattern for Status objects is to return a Status object unconditionally, i.e. both on success and on failure -- so that the developer of the calling code is reminded that the function can fail, and so that a lack of error-handling will be explicit:
$status = $this->foo(); $result = $status->value; // reviewer alert $result->bar();
Instead of implicit:
$result = $this->foo(); $result->bar();
php-option seems to be following the same pattern, except with a throwing accessor for $status->value.
I gather the main thing you don't like about Status is that it contains message formatting. It seems to me that if you added a throwing accessor to the Status class, and didn't use its message formatting functions, it would more or less fit your needs.
-- Tim Starling
On Tue, Oct 8, 2013 at 12:15 AM, Tim Starling tstarling@wikimedia.org wrote:
On 08/10/13 14:40, Erik Bernhardson wrote:
A reviewer should be able to know if the error conditions are properly handled by looking at the new code, not by looking up all the function calls to see what they can possibly return.
This is why the recommended pattern for Status objects is to return a Status object unconditionally
Can we add an example of that usage to the status object with a note not to follow the "return this in case of error" pattern that you might see elsewhere in the code? It might even be worth a bit of refactoring to get rid of the old pattern or people will still keep finding it and copying it.
Can we add an example of that usage to the status object with a note not to follow the "return this in case of error" pattern that you might see elsewhere in the code? It might even be worth a bit of refactoring to get rid of the old pattern or people will still keep finding it and copying it.
{{done}} [0]
[0]: https://gerrit.wikimedia.org/r/#q,Ia98543caaa829cad443abf0f0f5038b3de943ef8,...
On 10/07/2013 11:40 PM, Erik Bernhardson wrote:
Status class:
A review of the code within this class tells me that its primary use case(its single responsibility if you will) is to assist the caller in generating user facing error messages about an operation that can fail. If null/false is an acceptable return value then there is no reason to use this class.
There are really two parts of the status class:
* Setting error codes of varying severity (e.g. newGood for success, newFatal for fatal error, error for a regular error), then letting the caller check for them (e.g. hasMessage(), isOK and isGood, if it's *not* okay follow-up with getErrorsArray()). Note that hasMessage uses internal error codes *not* user-facing errors, so it's a way to check programmatically if a particular type of error occurred.
* Message formatting with wfMessage and friends; indeed, this whould probably not be in the class if it were designed today, under single responsibility principle.
I would investigate if you can use just the first.
Matt Flaschen
Le 08/10/13 07:23, Matthew Flaschen a écrit :
There are really two parts of the status class:
- Setting error codes of varying severity (e.g. newGood for success,
newFatal for fatal error, error for a regular error), then letting the caller check for them (e.g. hasMessage(), isOK and isGood, if it's *not* okay follow-up with getErrorsArray()). Note that hasMessage uses internal error codes *not* user-facing errors, so it's a way to check programmatically if a particular type of error occurred.
- Message formatting with wfMessage and friends; indeed, this whould
probably not be in the class if it were designed today, under single responsibility principle.
I would investigate if you can use just the first.
Would it make sense to RFC the decoupling of the Status class and get it split in two parts: Status and StatusFormatter (or whatever).
On 08/10/13 06:45, Brion Vibber wrote:
Out of curiosity, what's an actual example of code where the execution flow of exceptions is significantly more surprising than the execution flow of a billion manual checks to avoid "Fatal error: Call to a member function foo() on a non-object"?
I've heard the vague claim that exceptions are confusing for years, but for the life of me I've never seen exception-handling code that looked more complex or confusing than code riddled with checks for magic return values.
I don't think exceptions are confusing. The sole difference between exceptions and error returns is their default handling -- exceptions unwind the stack and show an error to the user, whereas error returns do nothing.
Sometimes it is a choice between exception and a fatal error, but more often, it is a choice between an exception and some subtle context-dependent malfunction. In many cases, ignoring the error is not exactly correct, but it is less damaging than allowing the exception to terminate the whole request so that it is shown to the user. Whether you prefer a subtle failure or an epic failure is mostly a matter of personal preference.
The designers of PHP have their feet firmly in the "subtle failure" camp, and this presumably influences the developers who work with the language. Personally, I am inclined towards tolerance -- I think developers are most happy and productive when they are allowed to choose which approach they will take to this sort of problem.
Of course, that doesn't stop us from offering advice.
-- Tim Starling
Erik Bernhardson ebernhardson@wikimedia.org wrote:
Moving forward, the Flow team is considering using a php implementation that follows the ideas of the haskell Maybe monad( https://github.com/schmittjoh/php-option ). This is, in concept, rather similar to the Status class the wikitext parser returns. We would like to use this library as a way to more explicitly handle error situations and reduce the occurrences of forgetting to check false/null. This particular pattern is very common in Functional languages.
I don't think exceptions are evil, they are more structured gotos and goto can be used properly to handle errors.
Status class has similar problem as unchecked exceptions - you never know which exception might come, and similarly, you never know what kind of Status you might inherit from the code called.
However, exceptions can form a hierarchy, which allows the caller to react selectively to the class of problems we have.
I recently was struggling with this piece of MediaWiki code, which interprets values of internalAttemptSave:
(from EditPage.php)
1195 // FIXME: once the interface for internalAttemptSave() is made nicer, this should use the message in $status 1196 if ( $status->value == self::AS_SUCCESS_UPDATE || $status->value == self::AS_SUCCESS_NEW_ARTICLE ) { 1197 $this->didSave = true; 1198 if ( !$resultDetails['nullEdit'] ) { 1199 $this->setPostEditCookie(); 1200 } 1201 } 1202 1203 switch ( $status->value ) { 1204 case self::AS_HOOK_ERROR_EXPECTED: 1205 case self::AS_CONTENT_TOO_BIG: 1206 case self::AS_ARTICLE_WAS_DELETED: 1207 case self::AS_CONFLICT_DETECTED: 1208 case self::AS_SUMMARY_NEEDED: 1209 case self::AS_TEXTBOX_EMPTY: 1210 case self::AS_MAX_ARTICLE_SIZE_EXCEEDED: 1211 case self::AS_END: 1212 return true; 1213 1214 case self::AS_HOOK_ERROR: 1215 return false; 1216 1217 case self::AS_PARSE_ERROR: 1218 $wgOut->addWikiText( '<div class="error">' . $status->getWikiText() . '</div>' ); 1219 return true; 1220 1221 case self::AS_SUCCESS_NEW_ARTICLE: 1222 $query = $resultDetails['redirect'] ? 'redirect=no' : ''; 1223 $anchor = isset( $resultDetails['sectionanchor'] ) ? $resultDetails['sectionanchor'] : ''; 1224 $wgOut->redirect( $this->mTitle->getFullURL( $query ) . $anchor ); 1225 return false; 1226
This code is somehow replicated in ApiEditPage.php, but in another way.
I wanted re-use this logic in the Collection extension (which sometimes creates some page in bulk on behalf of the user) and I really wished error reporting was done with exceptions. At top level, they could be handled in EditPage.php way, API would return exception objects instead and other extensions could selectively handle some values and ignore others - for example, I would be happy to get the standard EditPage.php behaviour for most of the errors I am not particularly interested in.
Regarding the use of php-option:
php-option seems to be as a handy way to provide substitute objects in case of errors; I think this case comes not very often and is arguably better handled by the use of factory methods, i.e. a method is reponsible to deliver foreign instance of some class; factory methods can be defined once for particular orElse/getorElse situation; it is also easier to make sure that particular instances will deliver the same (or similar) interface. And factory methods can be overriden if some particular implementation needs different creation of fallback objects.
Instead of having:
return $this->findSomeEntity()->getOrElse(new Entity());
One could have:
interface StuffNeededFromEntity { /* what Entity() really provides */ }
class Entity implements StuffNeededFromEntity { }
class FallbackEntity implements StuffNeededFromEntity { }
/*...*/
/* In the code trying to findSomeEntity */
/* @return StuffNeededFromEntity */ protected function entityFactory() { $entity = $this->findSomeEntity(); if (null === $entity) { return new FallbackEntity(); } }
/* replace return $this->findSomeEntity()->getOrElse(new Entity()); */ return $this->entityFactory();
[The use of interface/implements above is for clarity only, one does not actually need to use those features]
I agree with php-option author that if ($x===null) boilerplate should be avoided, but in my opinion the solution is to do the logic once and abstract it properly for re-use and not hide it in some syntatic sugar instead. Imagine what happenes if we need some constructor parameters for new Entity() -> instead of updating entityFactory once one needs to go through all those "orElse" cases again.
//Saper
wikitech-l@lists.wikimedia.org