Looking through a lot of new code for #2230931: Supporting multiple item types per index, I came across a lot of instances (especially in IndexInterface
) where a boolean return value was used to indicate success or failure of an operation. In any language that supports exceptions, I think that's an anti-pattern which we should avoid. Especially since, as far as I can see, we don't even check the return value most of the times (which is, of course, exactly the problem with using the return value for error detection).
Of course, when switching to exceptions, we should definitely take care to always catch exceptions at some point further up the calling chain, they should only leave the module in a few instances (when searching, for example). Also, we of course need to make sure to always document which exceptions can be thrown by a method/function.
I would then also like to remove the hasValid*()
methods, which externalize error detection logic all over the module. (I.e., in all instances where you request, e.g., a datasource, you have to remember to do hasValidDatasource()
beforehand.)
Comment | File | Size | Author |
---|---|---|---|
#32 | 2242493-followup.patch | 1.56 KB | amateescu |
#22 | interdiff.txt | 4.46 KB | amateescu |
#22 | 2242493-22.patch | 11.04 KB | amateescu |
Comments
Comment #1
alarcombe CreditAttribution: alarcombe commentedIME using exceptions as part of normal program flow is the anti-pattern. :) FWIW, I always recommend against using exceptions, except in exceptional circumstances - namely that the method cannot be completely executed because of (typically) a resource issue (out of memory, file cannot be created etc).
There's a good article here on why exceptions can be considered worse than GOTOS:
http://www.joelonsoftware.com/items/2003/10/13.html
Food for thought?
Comment #2
freblasty CreditAttribution: freblasty commentedI'll have to agree with alarcombe, exceptions should be used in exceptional circumstances and not for normal program flow.
Comment #3
drunken monkeyFirst off, regarding "invisible in the source code": That's why you have to take extra care to always add the appropriate
@throws
declarations (transitively – i.e., if you call a method that can throw an exception, either catch it or also declare it).I admit, this is a lot easier in Java or other languages that force you to explicitly declare the exceptions that can be thrown. But still, I don't really see how a boolean return type is any more visible in the code – in both cases, you have to check the documentation of the method you are calling to learn about this.
But, and here comes the crucial part: if you are sloppy with return types, you often won't notice it. E.g., we currently completely ignore the return values of
IndexInterface::insertItems()
(and the other tracking methods in the index). If tracking doesn't work, the admin will never get notified, they might just, at some point, notice that the numbers are off.If we'd make the same mistake with an exception, as soon as the error state would occur the user (or, probably, a developer at some point, long before a stable release) would get a big, ugly exception screen – and complain about it in the issue queue, enabling us to easily fix this.
However, foremost I don't actually see the issue you are having here. Apart from the "invisible in the source code" part, I fully agree with you. Exceptions should only be used for exceptional states, never (or only after careful consideration) for normal control flow.
But that's exactly what we have here: an index should always have a valid tracker, a valid datasource (or, soon, valid datasource plugins for all its enabled datasources) and a valid server (unless it is disabled). Otherwise, there was some incompatible code change or the user has uninstalled a module we depend on – in both cases, this is an exceptional state in which we cannot work normally anymore, and which the user should be immediately notified about.
Comment #4
Nick_vhI have to agree with drunkenmonkey here and defend exceptions in those cases. It allows also to be much more verbose as to what exactly happened and what you can return to the user. You could in theory make a contract with negative numbers and return those from your core functions so you know what exactly went wrong but that wouldn't solve much in the long run.
Exceptions make total sense in the context of required parameters. We just have to make sure there is a good scope as to where we catch those exceptions and handle them properly (as drunkenmonkey mentioned earlier).
Comment #5
freblasty CreditAttribution: freblasty commentedSo exceptions will only be used for exceptional circumstances. However if we remove all
hasValid*()
methods than then users with a misconfigured or broken configuration index/server will be unable to correct their error using the UI as it will generate an exception when retrieving e.g.getTracker()
orgetDatasource()
. We should still allow developers to prevent an exception from occurring when possible.Comment #6
amateescu CreditAttribution: amateescu commentedShouldn't we catch the exceptions in this case and inform the user through a drupal_set_message() or something?
Comment #7
freblasty CreditAttribution: freblasty commentedWhy would we want to catch an exception if we can prevent on from occurring and show a message? Currently the index/server overview page already provide this functionality and will report "Missing or invalid plugin".
Comment #8
drunken monkeyYes, exactly. We just have to catch the exception at the right stage, and enabling the user to actually fix the broken configuration is of course one of the criteria here.
Because it's an exceptional state, and that's exactly what exceptions are for. And of course we still want to show a message. But we don't want to have a contract where we force every caller of a method to first call another method to ensure the call is legal. With exceptions, we can encapsulate that whole logic in a clean way, instead of necessitating callers to know about such internal logic.
Comment #9
freblasty CreditAttribution: freblasty commentedI'm for using exceptions when someone call
getDatasource()
and throw an exception when something is wrong. However we should still provide methods to check whether something is broken to keep code cleaner and not writing exception driven code.In a normal workflow for indexing items I would not check for a valid datasource and use exception handling as we expect the all plugins should be working correctly.
For an overview or edit page however instead of writing
try-catch
, a simplehasValid*()
is much cleaner.Comment #10
drunken monkeyOK, I agree, it does make sense in some cases. Especially in edit forms, when we want to ignore the error anyways.
(At least for the tracker and server – in my code from #2230931: Supporting multiple item types per index,
getDatasources()
just ignores invalid plugins, which will be the desired behavior anyways, and which will probably be used in overviews and edit forms, too. Only when a specific datasource is requested, which is invalid, an exception is thrown.)Is everyone in line with this, or does someone think we should always use exceptions?
Comment #11
amateescu CreditAttribution: amateescu commentedAnother possiblity is to use the concept of "broken/null" plugins when we encounter a missing plugin. Core (actually, Views) uses it a lot and there's also an issue to 'formalize' it: #1822048: Introduce a generic fallback plugin mechanism
Some other related core issues:
#1881630: [meta] Determine how to respond to invalid plugins (plugin dependencies)
#2178795: Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin
Edit: I'm +1 on #10 :)
Comment #12
drunken monkeyPersonally, I don't like the concept of returning "broken" plugin stubs much and wouldn't see the added value.
Would you be in favor of it, or did you just want to mention it?
Comment #13
amateescu CreditAttribution: amateescu commentedI'm not super familiar with all our codebase and its needs at the moment, so I just wanted to mention it :)
Comment #14
amateescu CreditAttribution: amateescu commentedAfter working a bit on the code affected by this decision, I can offer another option, taken from one of the issues referenced in #11: introduce a
$exception_on_invalid = TRUE
argument, which is pretty much self-explanatory :)Comment #15
drunken monkeyAh, yes, I saw that in that issue, too.
Personally, I find this a very ugly pattern, though, adding needless complexity and making the code harder to understand. I've also never seen this pattern anywhere before, though I have to admit that doesn't necessarily mean its not used. So, if there is no fervent support for this option, I wouldn't go with that but just continue with
hasValid*()
.Comment #16
BerdirThe pattern has been taken from Symfony's Container::get(), but I agree that it's ugly, that's why the referenced issue introduced a hasDefinition() that should be used in most cases for plugins managers. hasValid*() sounds like a similar pattern.
Agree with what's being said, we should both provide methods to check if something can be executed, then you can use those for example in the UI and in case the method is called anyway, then throwing an exception is the right thing to do.
Comment #17
amateescu CreditAttribution: amateescu commentedAfter all the cleanup that already went in with #2230931: Supporting multiple item types per index, this is all that I could find.
Comment #18
amateescu CreditAttribution: amateescu commentedFound one failing test :)
Comment #19
BerdirDo we really need this? In case it does fail, this will actually hide the exception/reason why it didn't work? I think that construct is only useful if an exception is the expected cases that you want to pass...
Comment #20
amateescu CreditAttribution: amateescu commentedYep, you're right :)
Comment #21
drunken monkeyI think
reindex()
should also work for read-only indexes. It only influences tracking after all.However,
clear
absolutely has to check whether the index is read-only before deleting the items on the server.So:
These also need new
@throws
tags.Also, the main focus of this issue was
getServer()
andgetTracker()
, which seem to not have been changed. So that should be added, too.But other than that, this looks good.
Doesn't it fail more horrible when there is an uncaught exception than just producing a fail? But the message would be handy, you're right – we could maybe just include it in the
fail()
message?Comment #22
amateescu CreditAttribution: amateescu commentedRight, updated
getServer()
andgetTracker()
and added @throws toreindex()
andclear()
.About the test, a fail is still a fail that needs to be looked at/fixed, so horrible or not doesn't really matter :)
Comment #23
drunken monkeyWhat I meant is that sometimes exceptions completely end the test, while with a fail you will also see the remaining assertions. I don't know if that would be the case here, though. Of course the developer will have to fix it in any case, but when the test doesn't error out, he/she might have more information to do that.
Comment #24
drunken monkeyThe patch looks good now, though. Thanks!
Comment #26
amateescu CreditAttribution: amateescu commentedYay :) Rerolled and committed.
Comment #28
BerdirIt's by test method, and there's nothing else in that test method anyway I think? The only thing that fails horribly are fatal errors, exceptions are caught and logged by Simpletest.
Also, PHPUnit by default stops on the first exception/assertion fail, because everything that follows after it is in at least 9 of 10 cases just noise that confused people and they try to fix things that are only a follow-up error of the actual problem :) I've seen that many times ;)
Comment #29
alarcombe CreditAttribution: alarcombe commentedWe appear to have this as a commonly used pattern now, but if getBackend() doesn't return a valid object then we just terminate with a fatal error trying to call removeIndex on a non-object.
This is where I get nervous about exceptions, no matter what the language or framework :) As I see it we have the following options, none of which are good
a) getBackend needs to throw a SearchApiException exception if it can't return an object (even though this is not an 'exceptional' circumstance for that method.
b) if getBackend returns null we return FALSE (or something semantically correct)
c) (my preference, for consistency), we throw (and catch) a SearchApiException if getBackend is null eg
Comment #31
drunken monkeyAh, you're right of course, I/we totally missed that method! So yes, that should definitely also throw an exception.
Yes it is. A server always needs to have a backend, otherwise someone has uninstalled a needed module. (Which reminds me: #2257081: Make sure to handle uninstalled modules providing plugins correctly.)
I also spotted now that
Index::getServer()
doesn't abide to its contract and also throws an exception if no server was set for the index (and not only when an non-existent one was set). This should now be fixed.Comment #32
amateescu CreditAttribution: amateescu commentedOops :)
i saw that you fixed
Index::getServer()
in http://drupalcode.org/sandbox/daeron/2091893.git/commit/f915803974581c05..., so I committed this patch forServer::getBackend()
.Comment #34
drunken monkeyExcellent, thanks!
Comment #36
drunken monkeyOops, a bit too quick: we still lacked the proper
@throws
documentation forgetBackend()
, and we didn't check all locations where it got called. Should be alright now, though.