Server config import issue

This was removed in #2242361: Coding standards clean up, then added again the commit after it, but that is all the information that I have about this.

I can't reproduce this.
Can you provide steps for reproducing reliably?
Have you tried this with a vanilla installation of Drupal with just this module?

I can confirm I have the same use case. I have debugged it down to the same issue - \Drupal\search_api\Entity\Server::toArray
So even in the config file the 'backend_config' is different this method will always load it from the active config. Even if other developer does changes to the 'backend_config' they will never get updated as they are always replaced from this function so code sharing and deployment is broken and not working and requires to do all the changes manually in order to get the config in sync.
A start point would be Drupal\Core\Config\Entity\ConfigEntityStorage::importUpdate method.

Instead of re-iterating that you think this is broken, could you please give me steps to reproduce this, like I asked? Because I just tested again, and still can’t. It works fine for me.

It seems to be a bit more complicated after looking more into it. It is working ok on a vanilla install with just this module.
The way to break things:
1) implement hook_search_api_server_load
2) call $entity->getBackend() for each of the loaded entities, no need to do anything else
3) export current server config
4) edit the config file and change any settings
5) try to import the server config and observe it does not work
As described this is caused because the Config Import will call the toArray method, which would call the getBackend which checks for $this->backendPlugin , as this was already initialized in the hook_load and will use it instead of loading the one from the new configuration.

To me getBackend() method functions as it should so it does not reload. Is there any specific reason to load the backend config in the toArray() method? As written by @Spurlos "Discover the purpose of the bug code introduced in ::toArray() method" but no info has been provided why is this needed, maybe there is better approach here.

Thanks for providing this information! With this, I was finally able to reproduce the problem, and also easily write a test checking for it. (Great first step for any bug report.)

Is there any specific reason to load the backend config in the toArray() method? As written by @Spurlos "Discover the purpose of the bug code introduced in ::toArray() method" but no info has been provided why is this needed, maybe there is better approach here.

The info why this is needed is right in the @todo comment: If someone calls setConfiguration() on the backend plugin, instead of setBackendConfig() on the server entity, we still want to preserve those changes. This is, for example, important when submitting the backend plugin’s form, as that just stores the changed config on the plugin.

However, I see a lot of options for resolving this problem:

1. Using isSyncing() in Server::toArray() to avoid the call during config import.
2. Modifying the setConfiguration() implementation of BackendPluginBase to also propagate the change back to the server entity, eliminating the need for the code in Server::toArray(). (To avoid infinite loops, instead of using a $should_propagate argument, as the @todo comment suggests, it should be enough to check whether the two are actually different, and not do the call otherwise.)
   Problem with this is that, in theory, that method could be overridden. In practice, though, I think all overrides will/should call the parent method, so this shouldn’t actually be a problem.
3. We could also try to “forbid” calling $backend->setConfiguration() directly – i.e., do more or less the same as above, but also add a deprecation warning. In this case, we’ll also have to override submitConfigurationForm(), though, which is rather tricky because it’s in a trait, not on the plugin base class. So, probably not the best option.
4. We could also override Server::set() to call setBackendConfig() if it is called with 'backend_config' as the $property_name, which should eliminate the problem during import. (\Drupal\Core\Config\Entity\ConfigEntityBase::set() already has a very similar check for entities implementing EntityWithPluginCollectionInterface, but unfortunately this doesn’t look like an option for us (anymore), as we’d have to change the entity structure for that.

Attached are patches implementing options 1, 2 and 4, all of which seem to be working fine. Please test/review!
Also, any opinions of which of these make the most sense would be appreciated.

The hook_search_api_server_load() implementation caused an error in an unrelated test.

I started having this issue after updating acquia connector to `1.21` and I confirmed that the patch #11 fixed the issue.

I started having this issue after updating acquia connector to `1.21` and I confirmed that the patch #11 fixed the issue.

There are three patches. Which one did you try – or did you try all of them?

There are three patches. Which one did you try – or did you try all of them?

My mistake, I only tried the last patch "2976339-11--server_config_import_issue--Server_set_override.patch" and it seems to work fine so far.

Thanks for reporting back!
As no-one else has weighed in on the code aspect, I reviewed again myself and decided that I like the second patch best. Would you mind giving that a try, too, to make sure it works as expected? Then I can commit and finally resolve this issue.

As no-one else has weighed in on the code aspect, I reviewed again myself and decided that I like the second patch best. Would you mind giving that a try, too, to make sure it works as expected? Then I can commit and finally resolve this issue.

I tried the second patch as you suggest on version 1.17 and confirmed that it's working on my end as well.

I also tried to remove the patch and then updated acquia connector to 1.22 (the latest release) to see if it has been fixed somehow on their end and was still seeing the issue.

Excellent, thanks a lot for testing!
Committed. Let’s hope this fixes the issue.