- Issue created by @mxr576
- 🇦🇹Austria drunken monkey Vienna, Austria
drunken monkey → made their first commit to this issue’s fork.
- 🇦🇹Austria drunken monkey Vienna, Austria
Thanks for reporting this problem!
As you already saw by the test failure, the
Index
class uses$this->datasourceInstances
as the “source of truth” for its datasources. Keeping this consistent helps avoid a lot of thorny problems when changing index settings (see #2638116: Clean up caching of Index class method results (especially fields) → for background on this decision). For instance, a similar change as the one forremoveDatasource()
would have to be made toaddDatasource()
orgetDatasourceIds()
would return incorrect data when called after adding a datasource.However, I guess this doesn’t really apply when the datasource plugins aren’t loaded yet, so maybe using either
$datasourceInstances
or$datasource_settings
based on whether the former is initialized would work in all cases. The only “break” in functionality would now be thatIndex::getDatasourceIds()
will never throw an exception, and since that exception wasn’t even documented I guess this is acceptable. As you say, it might even improve performance in rare cases.Please give the new code in the MR a try!
- 🇭🇺Hungary mxr576 Hungary
Good thinking! I can confirm that the changes you made still mitigates the reported issue. Should I just RTBC this then? :thinking-face: