Use exceptions instead of "return FALSE;"

First 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.

Shouldn't we catch the exceptions in this case and inform the user through a drupal_set_message() or something?

Yes, 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.

Why would we want to catch an exception if we can prevent on from occurring and show a message?

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.

OK, 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?

Personally, 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?

Ah, 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*().

1. +++ b/src/Entity/Index.php
   @@ -937,11 +937,9 @@ class Index extends ConfigEntityBase implements IndexInterface {
      public function reindex() {
   -    if ($this->status() && !$this->isReadOnly() && $this->hasValidTracker()) {
   +    if ($this->status() && !$this->isReadOnly()) {
          $this->getTracker()->trackAllItemsUpdated();
   -      return TRUE;
        }
   -    return FALSE;
      }
    
      /**
   @@ -990,11 +988,10 @@ class Index extends ConfigEntityBase implements IndexInterface {
   
   @@ -990,11 +988,10 @@ class Index extends ConfigEntityBase implements IndexInterface {
       * {@inheritdoc}
       */
      public function clear() {
   -    if ($this->reindex() && $this->isServerEnabled()) {
   +    if ($this->isServerEnabled()) {
   +      $this->reindex();
          $this->getServer()->deleteAllItems($this);
   -      return TRUE;
        }
   -    return FALSE;
      }
   

   I 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:

   public function clear() {
     if (status()) {
       reindex();
       if (!isReadOnly()) {
         getServer()->deleteAllItems();
       }
     }
   }
   

2. +++ b/src/Index/IndexInterface.php
   @@ -390,9 +390,6 @@ interface IndexInterface extends ConfigEntityInterface {
       * Marks all items in this index for reindexing.
   -   *
   -   * @return bool
   -   *   TRUE if the operation was successful, FALSE otherwise.
       */
      public function reindex();
    
   @@ -446,9 +443,6 @@ interface IndexInterface extends ConfigEntityInterface {
   
   @@ -446,9 +443,6 @@ interface IndexInterface extends ConfigEntityInterface {
    
      /**
       * Clears all items in this index and marks them for reindexing.
   -   *
   -   * @return bool
   -   *   TRUE if the operation was successful, FALSE otherwise.
   

   These also need new @throws tags.

Also, the main focus of this issue was getServer() and getTracker(), which seem to not have been changed. So that should be added, too.
But other than that, this looks good.

Do 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...

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?

About the test, a fail is still a fail that needs to be looked at/fixed, so horrible or not doesn't really matter :)

What 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.

The patch looks good now, though. Thanks!

Ah, you're right of course, I/we totally missed that method! So yes, that should definitely also throw an exception.

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.

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.

Excellent, thanks!

Oops, a bit too quick: we still lacked the proper @throws documentation for getBackend(), and we didn't check all locations where it got called. Should be alright now, though.