Handle potential null on Index::getServerInstance() in Index::postSave()

Shouldn’t the server be set to null instead of ''? The former means “no server”, the latter looks to most code like a server with ID ''.
In your setup, there also seems to be something else going wrong as an index without server cannot be enabled.

If you try to import invalid configuration it is clear that errors will occur, I wouldn’t say that is a bug in the code.

Shouldn’t the server be set to null instead of ''? The former means “no server”, the latter looks to most code like a server with ID ''.

Good question, I was also thinking about that, but the configuration that I have tried to import via Recipe was created by selecting " - No server - " in a search index and that lead to server: '' in the config instead of server: null.

Please also note that my proposed fix also already occurs in the postSave() method, so my theory was that it was just forgotten to be added to this specific place as a guarding condition.

      elseif (!$this->status() && $original->status()) {
        if ($this->hasValidTracker()) {
          $index_task_manager->stopTracking($original);
        }
        if ($original->isServerEnabled()) {
          $original->getServerInstance()->removeIndex($original);
        }
      }

I tested what happens when an index’s server assignment changes, and wanted to clarify the resulting config values:

- If an index is explicitly set to “- No server -” in the UI, the config is saved as server: ''.
- If the server that an index is assigned to is deleted, the index entity ends up with server: null.
--- If you then edit and re-save such an index (now with no server) and again select “- No server -”, the value becomes server: ''.

Huh, you’re right, good catch! Then I think that is the real bug here, the problem in Index::postSave() is just a symptom. “No server” should definitely be represented as null, not ''.
I think we should make sure an index is never saved with '' as the server ID and probably also provide an update hook that sanitizes existing indexes.

Would it be OK with you if we converted this issue accordingly, or would you prefer a new issue and would be in favor of still adding this check (which I’d argue against, but am prepared to be convinced/outvoted)?

For the record, having server: null in the config also triggers this bug. So other than the inconsistency between '' and null as potential values, I think the fix in the MR is still necessary.

Do you mean it appears when importing an index config where you have server: null and status: true?
That is an invalid config, so I don’t think we can be held responsible for any errors that occur consequently.

I think we need test coverage for this to proceed.
Right now, I cannot even see how the original problem would occur without config overrides being present. And if they are present, see above: I don’t think we can be made responsible for people forcing invalid config. Then the only real bug here would be that server: '' makes no sense.