Problem/Motivation

When using config overrides for config entities, Core will automatically ignore these on admin pages (since you want to edit the "saved" ones, not the overrides which you can't change anyways).
However, since our admin UI does a lot more than editing of entities, this leads to several problems like:
- Displaying the non-overridden settings on the "View" tabs
- Deleting items from the wrong server when clearing an index (or the server)
- Indexing with wrong settings when using "Index now"

Proposed resolution

The AdminPathConfigEntityConverter can be told, that a given route should get the overriden values:

  options:
    parameters:
      search_api_server:
        with_config_overrides: true

We should verify which routes should use the overridden entities and then add that definition to them.

CommentFileSizeAuthor
#126 2682369-126--fix_tests.patch2.17 KBdrunken monkey
#115 2682369-115--config_overrides.patch54.46 KBdrunken monkey
#115 2682369-115--config_overrides--interdiff.txt6.36 KBdrunken monkey
#113 2682369-113--config_overrides.patch54.46 KBdrunken monkey
#113 2682369-113--config_overrides--interdiff.txt6.51 KBdrunken monkey
#111 2682369-111--config_overrides--debug.patch53.13 KBdrunken monkey
#111 2682369-111--config_overrides--debug--interdiff.txt1.51 KBdrunken monkey
#109 2682369-109--config_overrides--debug.patch52.71 KBdrunken monkey
#109 2682369-109--config_overrides--debug--interdiff.txt1.02 KBdrunken monkey
#107 2682369-107--config_overrides.patch52.5 KBdrunken monkey
#107 2682369-107--config_overrides--interdiff.txt2.61 KBdrunken monkey
#104 2682369-104--config_overrides.patch52.35 KBdrunken monkey
#104 2682369-104--config_overrides--interdiff.txt1.4 KBdrunken monkey
#102 2682369-102--config_overrides--debug.patch52.18 KBdrunken monkey
#102 2682369-102--config_overrides--debug--interdiff.txt1.15 KBdrunken monkey
#100 2682369-100--config_overrides.patch51.95 KBdrunken monkey
#100 2682369-100--config_overrides--interdiff.txt1.57 KBdrunken monkey
#98 2682369-98--config_overrides.patch51.9 KBdrunken monkey
#98 2682369-98--config_overrides--interdiff.txt667 bytesdrunken monkey
#96 2682369-96--config_overrides--debug.patch52.09 KBdrunken monkey
#96 2682369-96--config_overrides--debug--interdiff.txt1.43 KBdrunken monkey
#94 2682369-94--config_overrides.patch51.9 KBdrunken monkey
#94 2682369-94--config_overrides--interdiff.txt4.88 KBdrunken monkey
#92 2682369-92--config_overrides.patch51.48 KBdrunken monkey
#92 2682369-92--config_overrides--interdiff.txt2.83 KBdrunken monkey
#90 2682369-90--config_overrides.patch51.55 KBdrunken monkey
#90 2682369-90--config_overrides--interdiff.txt7.9 KBdrunken monkey
#88 2682369-88--config_overrides.patch50.43 KBdrunken monkey
#88 2682369-88--config_overrides--interdiff.txt1.71 KBdrunken monkey
#86 2682369-86--config_overrides.patch49.72 KBdrunken monkey
#86 2682369-86--config_overrides--interdiff.txt887 bytesdrunken monkey
#84 2682369-84--config_overrides.patch49.67 KBdrunken monkey
#84 2682369-84--config_overrides--interdiff.txt28.49 KBdrunken monkey
#82 2682369-82--config_overrides.patch23.38 KBdrunken monkey
#82 2682369-82--config_overrides--interdiff.txt4.22 KBdrunken monkey
#80 2682369-80--config_overrides.patch23.37 KBdrunken monkey
#80 2682369-80--config_overrides--interdiff.txt4.52 KBdrunken monkey
#78 2682369-78--config_overrides.patch22.26 KBdrunken monkey
#78 2682369-78--config_overrides--interdiff.txt5.08 KBdrunken monkey
#76 2682369-76--config_overrides.patch22.26 KBdrunken monkey
#76 2682369-76--config_overrides--interdiff.txt14.38 KBdrunken monkey
#71 2682369-71--config_overrides.patch13.63 KBdrunken monkey
#71 2682369-71--config_overrides--interdiff.txt8.29 KBdrunken monkey
#68 2682369-68--config_overrides.patch6.49 KBdrunken monkey
#62 2682369-debug.txt1.03 KBswentel
#62 2682369-61.patch545 bytesswentel
#56 index-0-items-after-local-switch.png114.79 KBgeerlingguy
#51 2682369-config-entities.patch23.05 KBCrell
#44 interdiff.txt646 bytesAlumei
#44 search_api--2682369-44.patch25.14 KBAlumei
#42 interdiff.txt12.73 KBAlumei
#42 search_api--2682369-42.patch24.86 KBAlumei
#40 interdiff.txt571 bytesAlumei
#40 search_api--2682369-40.patch25.21 KBAlumei
#37 interdiff.txt8.33 KBAlumei
#37 search_api--2682369-37.patch25.21 KBAlumei
#35 search_api--2682369-35.patch17.97 KBAlumei
#32 2690373-32.patch5.16 KBrosinegrean
#27 2690373-27.patch6.24 KBdrunken monkey
#27 2690373-27--interdiff.txt1.22 KBdrunken monkey
#19 use_of_overridden_config_entities-2682369-19.patch5.16 KBstBorchert
#11 search_api--fix-server-status-page--2682369-11.patch892 bytesAlumei
#6 search_api--fix-server-status-page--2682369-6.patch493 bytesAlumei
#2 search_api--fix-server-status-page--2682369-2.patch613 bytesAlumei
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alumei created an issue. See original summary.

Alumei’s picture

borisson_’s picture

Status: Active » Needs review

Let's see what the test bot thinks.

Status: Needs review » Needs work

The last submitted patch, 2: search_api--fix-server-status-page--2682369-2.patch, failed testing.

The last submitted patch, 2: search_api--fix-server-status-page--2682369-2.patch, failed testing.

Alumei’s picture

Alumei’s picture

But i'm also wondering whether this affects other parts of the admin backend.
After all it should affect every admin-route that loades a config-entity.
Though for some it should be intended the way it works out of the box.

From what I gathered:
For normal configuration objects there ist \Drupal:config('') & \Drupal:configFactory()->getEditable('') for the overriden & non-overriden version respectively.

For config entities the ConfigEntityStorage provides load() / loadMultiple() & loadOverrideFree() / loadMultipleOverrideFree().

Additionally the aforementioned ParameterConverter works on admin routes to provide "OverrideFree" config entities per default.

Maybe i'm putting to much thought into it, but could this also affect routes like

entity.search_api_server.enable:
  path: '/admin/config/search/search-api/server/{search_api_server}/enable'

entity.search_api_server.disable:
  path: '/admin/config/search/search-api/server/{search_api_server}/disable'

entity.search_api_server.clear:
  path: '/admin/config/search/search-api/server/{search_api_server}/clear'

and some of the index related ones.
Like all the routes where something acts on the passed non-overriden config entity.

Alumei’s picture

Interesting to note is also that in

entity.search_api_index.canonical:
  path: '/admin/config/search/search-api/index/{search_api_index}'

The server related information in the index is "Overriden" because it is loaded with load() from the Storgage only when getServer() is called.

drunken monkey’s picture

Status: Needs work » Needs review

Thanks for reporting this problem!
I'd say, though, that at least wherever any values are actually changed we definitely want and have to use the non-overridden version of the entities. For "View", though, I'm not sure. If the user doesn't know what they're doing, this is bound to be confusing anyways. But I guess displaying the resulting, overridden entities there makes more sense. The optimum would probably be to mark all displayed fields that are overridden, but that would probably be a lot more work for a rather small gain.
In any case, though, I'd say we should apply the same logic to both server and index. Having that inconsistent could cause the most confusion of all.

Alumei’s picture

I also thought it would be nice to at least know whether a given config entity contains overriden data or not. But I couldn't find anything like that.
Like on a config entity form a notice that tells the user that he is editing an overriden entity.

For viewing / editing data I think it definitely should follow the ConfigurationForm approach.
But I was wondering how overriden server / index configuration would affect stuff like clearing the index. After all I think that does not change the configuration values themselves but rather act on them and which outcome might be affected by the use of overridden vs non-overriden values.

Alumei’s picture

This patch updates the entity.search_api_index.canonical route to also use the overriden version of a given index if it exists.

stBorchert’s picture

This problem affects a lot more than simply displaying wrong ("non-overridden") values. Because the ParamConverter of SearchAPI is never used for the route parameters ("server" and "index"), configuration overrides are not used in any case.
In #2690373: Param from route is loaded without overrides I came to the same solution as Alumei. To fix the problem for every case, it is required to add the route option to nearly all route definitions.

Alumei’s picture

I think there are 3 solutions for this.

1. Update all the necessary route definitions
2. Write our own param converter / update the existing to per default load overriden values (maybe for config that is a search_api_index / search_api_server on admin routes) + place it higher that the AdminPathConfigEntityConverter higher so that it is called earlier (AdminPathConfigEntityConverter is set up the same to be called before AdminPathConfigEntityConverter)
3. Update als the routes to at some point load the Config entity again wich would result in overriden entities like in the patch of: #2684977-2: Config overrides are sometimes ignored.

Also #2684977-8: Config overrides are sometimes ignored. suggest that there are some routes, mainly those with EntityConfim forms could be converted normal ConfirmForms.

I would propose to go with option 2 OR Do a combination of options 1 & 3 (convert those entity EntityConfim forms to normal ConfirmForms by just passing along the index- / server-name and load the overriden config in the final submit)

drunken monkey’s picture

Version: 8.x-1.0-alpha9 » 8.x-1.x-dev

@ stBorchert: Why didn't you just re-post your patch from #2690373-2: Param from route is loaded without overrides? That already seemed pretty good.

@ Alumei: With just adding the option to all necessary routed (your option 1), what issues would we then still face that would lead to us needing to manually reload the entities in some places, too?

drunken monkey’s picture

Issue summary: View changes

Let's use this issue as the canonical one for this problem.

drunken monkey’s picture

Title: Server status page shows non overriden values with config-overrides present » Problems when using overridden config entities
drunken monkey’s picture

Title: Problems when using overridden config entities » Problems when using config overrides for search entities
drunken monkey’s picture

Component: User interface » Framework
Priority: Normal » Major
Status: Needs review » Needs work
stBorchert’s picture

Well, ok. Thought it would be too easy ;)

drunken monkey’s picture

I'm still not sure I fully understand this issue, but from how I understand it we shouldn't use the overridden version of the entity when making any changes to it, right?
That would mean, then, that the enable/disable pages shouldn't use the overridden entities, and neither should the fields UI and the "Filters" tab.

The problem, then, is when disabling a server or index – in both cases, action on the linked entities (the server's indexes or the index's server) will be executing, even if the override would hard-wire the entity's status to true (and no real action would thus be taken).

Another case for the server: when disabling it, all its indexes will be disabled (and, thus, removed from the server), but that code should then definitely use the server's overridden values. (NB: I think this already works correctly, since the sever just disables the indexes, and they then take care of informing the server – loading a fresh, overridden copy of the server for the purpose, unless I'm mistaken. But still pretty shaky, it would seem.)

Any other problems we might run into?
And any good way to test any of this? Is it easy to apply config overrides within tests? Having a good set of test cases for this functionality would surely help a lot.

Alumei’s picture

This issue initially pointed out, that for admin routes which use the entity upcasting for config entities, the AdminPathConfigEntityConverter enforces them to be loaded in 'non overriden' state. This seems to be in line with the propably common use case: Change the config entity. As for that use-case the config should be edited 'non-overriden'.

This is also how config overrides are expected to work, looking at this from the default.settings.php:

Configuration overrides.
 *
 * To globally override specific configuration values for this site,
 * set them here. You usually don't need to use this feature. This is
 * useful in a configuration file for a vhost or directory, rather than
 * the default settings.php.
 *
 * Note that any values you provide in these variable overrides will not be
 * viewable from the Drupal administration interface. The administration
 * interface displays the values stored in configuration so that you can stage
 * changes to other environments that don't have the overrides.
 *
 * There are particular configuration values that are risky to override. For
 * example, overriding the list of installed modules in 'core.extension' is not
 * supported as module install or uninstall has not occurred. Other examples
 * include field storage configuration, because it has effects on database
 * structure, and 'core.menu.static_menu_link_overrides' since this is cached in
 * a way that is not config override aware. Also, note that changing
 * configuration values in settings.php will not fire any of the configuration
 * change events.

Now in #2684977: Config overrides are sometimes ignored. mkalkbrenner noted:

While the standard features like indexing and searching use the staging server in the override example above, a click on 'Delete all indexed data on this server' on the server status page triggers the ServerClearConfirmForm and deletes the production index!

Which shows that operations that act on the configuration need the overriden values.

Alumei’s picture

In the following i tried to analyse each existing routes that use entity upcasting and how configuration is used on them:

=> <kind of use>, <Actual class> [ / <Base Form>]

entity.search_api_server.canonical:
  path: '/admin/config/search/search-api/server/{search_api_server}'

=> Read only config, ServerController
entity.search_api_server.edit_form:
  path: '/admin/config/search/search-api/server/{search_api_server}/edit'

=> Modify config, ServerForm / EntityForm
entity.search_api_server.delete_form:
  path: '/admin/config/search/search-api/server/{search_api_server}/delete'

=> Modify config, ServerDeleteConfirmForm / EntityDeleteForm
entity.search_api_server.enable:
  path: '/admin/config/search/search-api/server/{search_api_server}/enable'

=> Modify config, ServerController
entity.search_api_server.disable:
  path: '/admin/config/search/search-api/server/{search_api_server}/disable'

=> Modify config, IndexDisableConfirmForm / EntityConfirmFormBase
entity.search_api_server.clear:
  path: '/admin/config/search/search-api/server/{search_api_server}/clear'

=> Act on config => ServerClearConfirmForm / EntityConfirmFormBase
entity.search_api_index.canonical:
  path: '/admin/config/search/search-api/index/{search_api_index}'

=> Read only config, IndexController
entity.search_api_index.edit_form:
  path: '/admin/config/search/search-api/index/{search_api_index}/edit'

=> Modify config, IndexForm / EntityForm
entity.search_api_index.delete_form:
  path: '/admin/config/search/search-api/index/{search_api_index}/delete'

=> Modify config, IndexDeleteConfirmForm / EntityDeleteForm
entity.search_api_index.enable:
  path: '/admin/config/search/search-api/index/{search_api_index}/enable'

=> Modify config, IndexController
entity.search_api_index.disable:
  path: '/admin/config/search/search-api/index/{search_api_index}/disable'

=> Modify config, IndexDisableConfirmForm / EntityConfirmFormBase
entity.search_api_index.fields:
  path: '/admin/config/search/search-api/index/{search_api_index}/fields'

=> Custom param converter: SearchApiConverter
entity.search_api_index.add_fields:
  path: '/admin/config/search/search-api/index/{search_api_index}/fields/add'

=> Custom param converter: SearchApiConverter
entity.search_api_index.remove_field:
  path: '/admin/config/search/search-api/index/{search_api_index}/fields/{field_id}/remove'

=> Custom param converter: SearchApiConverter
entity.search_api_index.break_lock_form:
  path: '/admin/config/search/search-api/index/{search_api_index}/fields/break-lock'

=> Config not affected, act's on tempstor only
entity.search_api_index.processors:
  path: '/admin/config/search/search-api/index/{search_api_index}/processors'

=> Modify config, IndexProcessorsForm / EntityForm
entity.search_api_index.reindex:
  path: '/admin/config/search/search-api/index/{search_api_index}/reindex'

=> Act on config => IndexReindexConfirmForm / EntityConfirmFormBase 
entity.search_api_index.clear:
  path: '/admin/config/search/search-api/index/{search_api_index}/clear'

=> Act on config => IndexClearConfirmForm / EntityConfirmFormBase
Alumei’s picture

From that I would say as before, that the following "Act on config"-routes should get the special treatment from patch #19:

  • entity.search_api_server.clear
  • entity.search_api_index.reindex
  • entity.search_api_index.clear
Alumei’s picture

Now based on my previous quote form the default.settings.php:

Note that any values you provide in these variable overrides will not be viewable from the Drupal administration interface. The administration interface displays the values stored in configuration so that you can stage changes to other environments that don't have the overrides.

It could be argued, that for the for the "Read only config" we do not need to change anything as the user should not expect to see overriden values any way.

But I would argue that the above quote can also be read as " these variable overrides will not be viewable from the Drupal administration interface when editing configuration".
I also think the data on both "canonical" - pages would not be intuitive without overrides:

  • entity.search_api_server.canonical: would show the status of the actual server being used (Especially nice with a solr backend when viewing the latency)
  • entity.search_api_index.canonical: Is a bit more complex

Config usage on entity.search_api_index.canonical

This route contains two parts: "The theming of the index related data", "The form below for index related operations".
This mainly only shows data. But one possible use-case i could imagine would be overriding the "server"-key of the index configuration.

The theming of the index related data

In that use-case this would show the index-state of the wrong server both in the table & on the status-bar above. Also the link to the corresponding server-config-entity would be wrong.

The form below for index related operations

This form does two things:

  • Redirect to the appropriate routes when using the links for the operations: 'reindes' & 'clear'. This would not be a problem if those routes are beeing modified as noted in #23.
  • Act on the 'non-overriden' version for the operations: 'index_now' & 'track_now'

Note: this could also be fixed by loading the config-entity again, before passing it only to Form, instead of changing the route.

... This clearly shows that I'm researching on the go as I write & contradic my initial categorisation for 'entity.search_api_index.canonical'.

Now only 'entity.search_api_server.canonical' could remain with using 'non-overriden'-values but to keep it uniform should also use the 'overriden' version.

Alumei’s picture

Therefore I would update my proposed list from #23 as follows:

  • entity.search_api_server.clear
  • entity.search_api_index.reindex
  • entity.search_api_index.clear
  • entity.search_api_index.canonical
  • (entity.search_api_server.canonical)
Alumei’s picture

I hope you can somewhat follow my reasoning ...

But this still leaves the other "modifying" routes which are a bit tricky because "modifying" them might have unexpected side effects (preSave / postSave / preDelete)

From what I can tell from the code, the Server might call ->preUpdate(), ->postUpdate(), ->postInsert(), ->preDelete() on an Backen-Plugin which was instantiated with 'non-overriden' values. But would change / act on overriden index data.

If I have some time tomorrow I will try to analyse Index as well for such side-effects.

drunken monkey’s picture

I did a bit of pondering and testing of my own and have to conclude that Core's concept of config overrides is just fundamentally flawed. It might work for Core's own entities, but will break if more complex reactions to config changes are necessary – as they are in our case. I don't think it's really possible for us to fix this.

Problems I see (even with the patch in #23):

  • When editing an index and disabling it, it will be removed from the correct server. However, when enabling it again, it will be added to the wrong, non-overridden one. That's because the saved entity is non-overridden, but the loaded original entity set on it is overridden.
  • Because of the same discrepancy, some changes that won't even take effect (since the property that is being changed is overridden) will still trigger a reaction – which might break the whole search setup.
  • When initially adding an override, manipulating or removing it, no config/entity update hooks/methods will be called, which means we cannot react to the change. (Effect: things might break.)
  • When applying overrides everywhere, on the other hand, all the overridden values will be saved to the underlying config, too (when saving unrelated part of the entity – i.e., an overridden server is saved when editing an index's fields).

Probably, the best thing we can do is just use the overridden values everywhere in the UI, even when editing entities. This will be confusing to users, but at least most of our reaction code will be adequate. (I think using the non-overridden values when editing servers wouldn't be a problem actually – but having that not even consistent within our own module is probably even worse.)
However, the incorrect detection of changes will still be a problem – hardly possible to fix this, as far as I can see, I'm afraid.
In the end, we should probably just warn users against using config overrides at all with this module.
Patch attached.

Alumei’s picture

@drunken monkey I don't think that is a good Idea because it goes against the default Drupal-UIx to provide more of an edge case functionality.
But how about this solution I came up with:

  • Remove all react logic from the config entities, converting them to a simple data container
  • Create one ore more services containing that react logic
  • Use entity hooks to trigger the above services (With this it could be ensured that the entity has already been saved & thus could be loaded overridden in every case)

This should reach the same result while still retaining the expected Drupal-UIx.

Though it would still have the following problems:
A:

Because of the same discrepancy, some changes that won't even take effect (since the property that is being changed is overridden) will still trigger a reaction – which might break the whole search setup.

B:

When initially adding an override, manipulating or removing it, no config/entity update hooks/methods will be called, which means we cannot react to the change. (Effect: things might break.)

Problem B is effectively hinted on in the settings.php so anyone using more complex overrides should be aware that it might break some things so at least that one I don't see as much of an issue.

drunken monkey’s picture

I think your proposal goes against the usual Drupal DX the same way mine goes against the UX. There are CRUD reaction methods for entities now, so we should use them and have all that logic and code bundled in one place.
Also, re-writing this now would be a much too large change for just this small problem.

But I do agree that my solution is definitely not ideal, either. I just don't see a better one at the moment – but I'm definitely open for suggestions.

Dane Powell’s picture

I'm not going to weigh in on the shoulds and shouldn'ts here, but just want to report that patch #27 solved a big problem on our project.

A very common scenario is to have a single search index spanning dev, test, and prod environments, and to make that search index read-only on non-prod environments by overriding the configuration via settings.php. This was done quite easily in D7, but isn't possible in D8 without this patch.

If anyone can propose an alternative solution to this problem besides the currently proposed patch, I'd love to hear it.

Alumei’s picture

I just had a look at it again.

I think we could also try extending the config entity storage, overriding it's doPostSave() behaviour.

As @drunken monkey pointed out the original is loaded overriden which is caused by the current doPostSave() implementation of the config entity storage.

My proposal would be to try overriding that behaviour in the following way:
1) The entity was already saved so load it by id to get the overriden version (even for newly created entities this would work)
2) Get the overriden original from the entity passed as an argument
3) Set the original from (2) as -》original for the entity (1)
4) Call postSave on the overriden entity (3)

With this the actual changes should be accurately calculated between two actual overriden versions.
What do you say? I think it could be a solution wich hopefully does not break anything & which does not require any changes to the existing realisation with postSave on the entity.

Edit: For reference this is the current implementation of doPostSave() in EntityStorageBase - https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

rosinegrean’s picture

Until another solution is found and implemented, here is an updated version of the patch from #27 because after the last update, the patch does not apply anymore.

drunken monkey’s picture

Thanks a lot, Alumei! I think this might be the best solution we have so far – maybe combined with loading the non-overridden entity in those cases where no update occurs but we still need the "effective" values (e.g., indexing).

Since this sounds like it could be a problem for other config entity types as well, I created #2744057: Inconsistencies when updating overridden config entities in Drupal Core to discuss this problem. If it doesn't get fixed in Core, the issue might at least shine new light on this issue with the additional input.
The probably biggest problem I point out in that issue, that of having the overridden values during pre-save, would actually not be one for us, luckily, since we don't need the original values during pre-save anyways. (Which might actually be true for most entity types.)

drunken monkey’s picture

Issue tags: +Release blocker
Alumei’s picture

I made a patch containing the classes / interface mentioned in my proposed solution in [2744057-27], as a temporary storage layer.
The patch also contains a storage class based on my proposal in that issue, as well as corresponding changes to the postSave() methods on Server & Index entity classes.

Let's see ...

Status: Needs review » Needs work

The last submitted patch, 35: search_api--2682369-35.patch, failed testing.

Alumei’s picture

I tried addressing some of the problems, primarily the ImmutableConfigException's.

Alumei’s picture

Status: Needs work » Needs review

For the testbot ...

Status: Needs review » Needs work

The last submitted patch, 37: search_api--2682369-37.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
25.21 KB
571 bytes

Oh ..

Status: Needs review » Needs work

The last submitted patch, 40: search_api--2682369-40.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
24.86 KB
12.73 KB

I tried addressing the errors by updating the tests to only use editable version. As far as I could see these test were not explicitly testing with overrides so is does not make a difference & preserves readablility IMHO.

To easiliy get to the editable version i also added 2 static methods to OverrideSupportingConfigEntityInterface. When the tests work I will update the core issue with that as well.

Status: Needs review » Needs work

The last submitted patch, 42: search_api--2682369-42.patch, failed testing.

Alumei’s picture

Status: Needs work » Needs review
FileSize
25.14 KB
646 bytes

Forgot again to implement the new methods in UnsavedIndexConfiguration ...

Status: Needs review » Needs work

The last submitted patch, 44: search_api--2682369-44.patch, failed testing.

Alumei’s picture

Seems there is an issue with dependency removal handling that I missed in my proposal within the core issue :-/

SchnWalter’s picture

I would suggest that we go with KISS and always use the overridden configuration entities, and just keep the configuration form as it is, and maybe just display a warning, see this core issue: #2408549: There is no indication on configuration forms if there are overridden values

Here is my reasoning: It's better to use default values for local environments and then override them as needed. For example if the Solr core for local environments is example_local and you override it with example_preprod, then you are OK if the config override somehow gets ignored or is accidentally removed on preprod since you are providing a default value that probably does not exist on that server (which might contain staging/preprod and production environments, but not a local environment).

If you have the production settings as default and provide overrides for local/staging/preprod/test environments; and you have another environment on the same server as production, then you are out of luck if the overrides from that environments gets messed up.

And if you use this approach then you won't need to make changes on environments where you have configuration overrides. Also, if you are lucky and don't need to change things on production, maybe you can add something link this in settings.local.php for those environments:


/**
 * The current project environment.
 * NOTE: Use only the following values: local, staging, preprod, production, other.
 */
define('PROJECT_ENVIRONMENT', 'production');

// ...

if (PROJECT_ENVIRONMENT != 'local') {
  /**
   * Lock any configuration changes done via the Drupal admin UI. This can be
   * useful in scenarios where for example configuration changes should not be
   * done on the production environment, but only on local environments.
   *
   * @see: https://www.drupal.org/project/config_readonly
   */
  $settings['config_readonly'] = TRUE;
}
Crell’s picture

Coming here from #2838116: Cannot override config values from settings.php.

If I understand correctly, this issue prevents Search API D8 from working correctly on Platform.sh. We provide service configuration values at runtime via PHP environment variables, which can then get mapped into configuration via $settings, $config, etc. That means we need to be able to override the config entity from settings.php. Telling people to just update the config entities directly is naturally not useful as that configuration would get lost on every new config-import (which happens on every deploy by default).

Indicating on the form that overridden values are in use seems useful, but also seems like it should be a core issue, no? You could override any config entity in concept. I don't have a strong opinion about how that gets displayed as long as the underlying bug is fixed.

How can we help move this forward so that we can support D8 Solr properly on Platform.sh?

drunken monkey’s picture

How can we help move this forward so that we can support D8 Solr properly on Platform.sh?

The best thing to do for this problem, I think, would be if it was fixed in Core: see #2744057: Inconsistencies when updating overridden config entities. If you could prod any config (entity) experts in that direction, that would be great.
As you say, lots of other config entities might have similar problems. (Maybe just not as pronounced, since we have a lot of reactions on entity changes, some even Drupal-external.) And Alumei seems to have already done a lot of great work there, as here. It's just a very complex issue, so it's hard to get people involved enough to review patches, verify approaches and give feedback.

The core of the issue, I think, is this: when you make a change to an entity in the UI which doesn't have any effect since the property is overridden anyways, the framework (either Core or Search API) shouldn't react as if there was a change. Therefore, we need to compare the entity's effective properties pre-save to its effective properties post-save. (Please correct me if I'm getting this wrong, Alumei!)

If this won't get fixed in Core (or not any time soon – i.e., not in 8.3) #44 is probably the way to go. If you could get some sites testing this patch to see if everything works correctly with it (and the Solr config overrides) that would be great! Reviewing and fixing the test fails would of course also help – but mainly I want to make sure this actually fixes the problem(s) and doesn't break anything in real-life applications.

And, I have to admit, this is a bit over my head, too, which is why I haven't been very active in either issue.

borisson_’s picture

If this won't get fixed in Core (or not any time soon – i.e., not in 8.3) #44 is probably the way to go. If you could get some sites testing this patch to see if everything works correctly with it (and the Solr config overrides) that would be great! Reviewing and fixing the test fails would of course also help – but mainly I want to make sure this actually fixes the problem(s) and doesn't break anything in real-life applications.

And, I have to admit, this is a bit over my head, too, which is why I haven't been very active in either issue.

This is also the reason I haven't been very active here. I agree with @drunken monkey that #44 is a decent approach if we can't fix it in core. However, this adds a lot of complexity and I'm sure other maintainers have similar problems so fixing it here feels wrong.

Crell’s picture

Here's a straight reroll of #44. Some jitter, and there are 3 test files that it touches that seem to no longer exist so I simply left those out. I will actually test it shortly and see if it works. :-)

Crell’s picture

Status: Needs work » Needs review

Bah, forgot to send it to the bot.

Manual testing of this patch seems to show no effect. I'm still poking at it but at the moment settings.php $config overrides still don't appear to be taking hold. (Suggestions for how to further investigate are most welcome.)

Status: Needs review » Needs work

The last submitted patch, 51: 2682369-config-entities.patch, failed testing.

Crell’s picture

I'm afraid I'm not going to be much help in pushing this patch forward at the code level without more direction. I've been trying to dig through it but I'm still rather lost. :-( I can confirm at least some of the test fails, though (specifically, I can't disable a server because it involves modifying an immutable object), and that it's still not picking up overrides.

The most recent patch in #2744057: Inconsistencies when updating overridden config entities doesn't resolve the issue either, although I'm unclear from the discussion there if more work would be needed in this module. My config-entity-fu is not strong, sadly.

drunken monkey’s picture

@ Crell: Ah, that's bad news. Do you maybe have a colleague then who might be good enough with the config entity system?
It's a really complicated problem, though, I'm afraid. (At least I keep telling myself that to make me feel better about zoning out every time I try to wrap my head around it.)

@ Alumei: Any chance you could maybe clarify further (probably rather in the Core issue than here) whether the Core patch is supposed to already work as-is, or would need modifications in this module, too?
Your comment #2744057-28: Inconsistencies when updating overridden config entities looks to me like it should work as-is (albeit with some unsolved problems) already.

geerlingguy’s picture

Coming over here from #2847092: Data of read-only indexes is deleted when they are removed from the server.

In my case, I thought we had solved our particular problem (the production search indexes got wiped out whenever a developer switched to using a local index) with the fix in that ticket, but it turns out if the developer uses the UI instead of Drush to switch the indexes, the production index still gets wiped out, even if it's set to 'read only' mode.

Could we potentially bump this to Critical? It's not fun seeing no search results—especially since the prod site gets all out of whack since Search API thinks the index is still up to date, even though all the contents just got wiped out:

Solr search index cleared out

borisson_’s picture

Priority: Major » Critical

I don't mind bumping this to critical, Thomas and I have discussed this several times but have no clue how to fix this. We'll need help. Not sure how to signal people with better config knowledge that this needs help though.

geerlingguy’s picture

Title: Problems when using config overrides for search entities » Config overrides for search entities aren't used when performing operations in the admin UI

Changing the title to be a little more specific.

Two times in the past week, our production search index was deleted due to devs using the Admin UI (instead of Drush) to toggle indexes for local environment testing. Technically speaking, it would be best if local environments didn't have any potential access to prod (e.g. via IP-based restrictions, or other auth mechanisms that only exist on prod), but the default here should be safety (if a server is set to 'readonly' then it should not be possible to delete indexed content for that server!), which is why I still agree this is critical.

drunken monkey’s picture

Sure, we can mark this "Critical". Won't help with moving the issue ahead, though – it's already one of the top priorities by being among the few stable release blockers left.
However, I also commented on #2574827-12: [search_api] Search API to point people to this issue (and the Core one) – maybe that'll help.

geerlingguy’s picture

@drunken monkey — totally understand! I just want to make sure that others using Search API recognize the speed bumps that are inherent in a beta version of the module. Since this one can (and does) lead to production data loss in many use cases (though I'd argue it's better to restrict access via IP or on the network layer so local developers _can't_ see prod solr :P), it's probably the largest gap I've noticed before I'd consider the module stable (so many things about Search API are already so much better in D8!).

drunken monkey’s picture

You're right, of course, as a warning to others it does indeed make a lot of sense. Didn't really think of it from that perspective.

swentel’s picture

Status: Needs work » Needs review
FileSize
545 bytes
1.03 KB

So yeah, this can be brutal if you don't pay attention. Now, as long when you are using the drush commands you're going to be fine.

The pacth in #44 seems overly complex and hard to maintain. The other patch in #11 which adds the option to the route is a potential way to go, but I'd rather prefer getting in the non-overidden config object first.

Now, there's a relatively easy workaround though by simple loading it using Entity Api which will load the entity with its overrides.
I've attached two patches for changing the Index Clear confirm form. The 'debug' patch will not clear the index, but instead output the server value. The first message will be the the server from the stored config object. The second message will output the overriden value (given you have put an overridde in settings.php, cf the patch).

The same concept can be used in other places where needed which I think will be easier to maintain and to test.

Regarding tests, check ConfigFormOverrideTest::testFormsWithOverrides() how it uses writeSettings() to write settings to settings.php in a test.

borisson_’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

No idea why this patch works, but I believe swentel. I do think we should really add a test though, we've had so much trouble with this in the past (as mentioned by @geerlingguy) so that test will deliver a lot of value. NW for the test.

mkalkbrenner’s picture

Since more than a year I don't run any production site without the patch at https://www.drupal.org/node/2684977#comment-10954863
It's very similar to swentel's in #62.

But just one more time again I have to repeat a simple question: Why is the status page an entity form (which is override free per definition)?

I still think the status page should be driven by a controller and the actions should be callbacks instead of submit handlers.

drunken monkey’s picture

Assigned: Unassigned » drunken monkey

Not being able to avoid it any longer, I'm now trying to work on this, using the following approach:

  • Add with_config_overrides: true for all paths where entity values aren't displayed to be edited. I.e., all except "Edit", "Fields" (along with sub pages) and "Processors".
  • Add a custom storage that re-loads the entity before doing $entity->postSave() – i.e., which uses the effective, overridden values for post-save hooks.

Essentially, a combination of Alumei's comments #25 and #31.
Of course, I'll also add tests (both Functional and Kernel) to verify this works as it should.

@ swentel: That's a good hot fix for that one specific problem, but hardly a solution for the overall issue discussed here. I think at this point (this being the only release blocker left, more or less) we have to find a overall solution for all problems discussed here.

But just one more time again I have to repeat a simple question: Why is the status page an entity form (which is override free per definition)?

It isn't.

entity.search_api_index.canonical:
  path: '/admin/config/search/search-api/index/{search_api_index}'
  defaults:
    _controller: '\Drupal\search_api\Controller\IndexController::page'
    _title_callback: '\Drupal\search_api\Controller\IndexController::pageTitle'
  requirements:
    _entity_access: 'search_api_index.view'
class IndexStatusForm extends FormBase {
class ServerStatusForm extends FormBase {

Not sure what you mean?

swentel’s picture

Status: Needs work » Needs review

@drunken monkey

Given the issue summary for the two most annoying problems:

- Deleting items from the wrong server when clearing an index (or the server)
- Indexing with wrong settings when using "Index now"

Those two problems are fixed by one line patches. I'm using that as argument for a KISS approach. I don't consider them as hot fixes, but I understand your point of view.

Also, I think switching storage is going to open up rabbit holes where not many people would like to enter (and hard to understand/debug). And AFAICS, what is described in #2744057: Inconsistencies when updating overridden config entities is probably valid, but has no consequences at this moment, which you already mentioned in a #33. I can't make my case hard, just a gut feeling and I also fail to write a failing test right now even though I know the fix is right, so I won't make my case to hard :)

(fwiw, core should move to an approach where overriding simple config or entities simply ain't possible at all from settings.php, but we're far off from that right now unless you use something as config split for instance)

swentel’s picture

Status: Needs review » Needs work

oops - didn't mean to reset the status (I almost had a failing patch, but getting stuck somewhere to make it 'green')

drunken monkey’s picture

Those two problems are fixed by one line patches. I'm using that as argument for a KISS approach. I don't consider them as hot fixes, but I understand your point of view.

These two problems, yes, but there are others to consider, too, no less severe if triggered (e.g., think about switching an index's server on the "Edit" form – this can currently also delete the index's data from the production server) and also hard to understand/debug. Looking at all of these potential problems combined, I think having a single, consistent solution for all of them is preferrable to having to remember to re-load the entity every time we want to do specific operations on admin pages.

I don't think the storage handler override will be such a big deal – it's just a single method we override, after all, that shouldn't have too much of an impact, or too many side effects. Since we end up with two seperate objects representing the same index*, though, this might indeed lead to some problems, at least when an already overridden index is saved. I don't really see this causing any major trouble, though – in my opinion, this can just be annoying or confusing in tests.

Also, the storage handler solves the editing problems, it doesn't have any effect on the two problems you cite. For those, the proper solution is just to specify with_config_overrides: true in the routing definition. No need to reload the entity on the spot – we don't want the overrides on those pages in any case.
(Conceptually, both of these solutions, the routing entry change and the line reloading the entity, are roughly the same, though: just one-off fixes for that specific place in the code. They need to be applied wherever such a problem occurs, no way to fix them all at once, as far as I can see. So me preferring the routing entry is mostly a matter of taste, I guess.)

Using a storage handler to take care of the other problems, with the entities' "post save" hooks/methods, might seem overkill, but I don't really see a better option. After all, this can't really be solved just by using the overridden entity for saving: we'd also have to make sure to ignore changes to properties that are overridden (i.e., where the change won't have an actual effect). So either we manually retrieve the overridden properties and refuse to make changes to them – and do that everywhere in the code where an index is saved – or we just reload the entity afterwards to make sure we have the actual, effective property values both for the index's current state and the original. (We can get away with this, as said in #33, because we don't have to care about overrides during pre-save, in our case (unless I'm mistaken).)

Anyways, I already have code that should more or less work, it's really pretty simple – see the attached patch.
The tricky part is writing the tests, which I'm in the process of doing now. Once we have those, it might also be easier to think of alternative solutions for all of this.

One other issues that's also mixed with this one is the problem of communicating clearly to users which entities they see in the UI are with or without overrides – #2408549: There is no indication on configuration forms if there are overridden values, in essence. However, this might just be confusing when editing entities, it won't have any horrible consequences like the problems discussed here, so I think we can ignore this for now, maybe wait what Core does, or try to solve this later, after the stable release.

* All of this applies to servers, too, of course.

drunken monkey’s picture

Status: Needs review » Needs work

Still needs tests, just wanted the test bot's opinion.

mkalkbrenner’s picture

It isn't.
Not sure what you mean?

@drunken_monkey: you're right. Sorry, seems like I was confused ;-)

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
8.29 KB
13.63 KB

This should take care of the existing tests. Seems working with two index instances is trickier than expected – who would have thought?
On the plus side, reindexing detection has improved, to even detect unnecessary reindexing across multiple page requests. (Practical use small, I fear, though.)

Status: Needs review » Needs work

The last submitted patch, 71: 2682369-71--config_overrides.patch, failed testing.

drunken monkey’s picture

Assigned: drunken monkey » Unassigned

Damn, ran into further problems (apart from the weird testing fails), in \Drupal\search_api\Entity\Index::preSave(). It seems my assessment that we don't need to worry about the overrides for pre-save was a bit off after all:

  public function preSave(EntityStorageInterface $storage) {
    // …

    // Prevent enabling of indexes when the server is disabled.
    if ($this->status() && !$this->isServerEnabled()) {
      $this->disable();
    }

    // …

    // Call the preIndexSave() method of all applicable processors.
    foreach ($this->getProcessorsByStage(ProcessorInterface::STAGE_PRE_INDEX_SAVE) as $processor) {
      $processor->preIndexSave();
    }

When a different server is set for the index, this will currently check the status of the wrong server (or even fail to find it) when saving an index without overrides (or changing the server).
Also, if processors are overridden (probably less of a use case, but still), the wrong processors' preIndexSave() methods will be called.

Things also don't look good for the server:

  public function preSave(EntityStorageInterface $storage) {
    // …

    $this->getBackend()->preUpdate();

    // If the server is being disabled, also disable all its indexes.
    if (!$this->isSyncing() && !$this->status() && $this->original->status()) {
      foreach ($this->getIndexes(array('status' => TRUE)) as $index) {
        /** @var \Drupal\search_api\IndexInterface $index */
        $index->setStatus(FALSE)->save();
      }
    }
  }

Depending what the backend plugin does in preUpdate(), it might use wrong values. (I guess this could be "fixed" by just documenting that backends have to be aware of this problem.)
And the second block could mis-detect a "disabling" if the status is overridden (e.g., as a safety precaution). (However, in this case it would seem logical that the indexes' status would also be overridden, mitigating this problem.)

Worse, though, I fear it's just impossible to get any reliable introspection into the config overrides present on a site. While you can just look into $GLOBALS['config'] for the settings-based overrides, you can also add arbitrary overrides using \Drupal::configFactory()->addOverride() – for which there doesn't seem to be any introspection at all. Seems to me that \Drupal\Core\Config\ConfigFactory::loadOverrides() could just be public, which would make this a lot easier (or, rather, possible at all), but it isn't, so that's that. (We can make a Core issue just for this small change, but would probably still have to wait for Drupal 8.4 to use it.)

With that, I really have no idea how to resolve the pre-save problems. (In addition to my proposed solution for post-save getting out of hand a bit.) Any ideas?

In any case, I fear that, having spent two whole days on this, I have to get some paid work in again now. Unassigning for now, therefore. Maybe I'll come back to this later this week – otherwise, this will definitely be my primary focus in Sevilla.

drunken monkey’s picture

Added #2860819: Make ConfigFactory::loadOverrides() public for Core – maybe we'll get lucky and it gets added for 8.3. I wouldn't bet on it, though. (Maybe I can lobby a bit in Sevilla.)

Otherwise, though, I've now discovered that the other, arbitrary overrides are (by default) added through services with the config.factory.override tag. While nothing prevents anyone from adding their overrides directly, we should be able to get away with just using that (plus $GLOBALS['config']) to retrieve all overrides. So a work-around is possible, and actually not much more arduous than without that Core patch.
Probably we'll just have to go that way (with or without the Core patch) – unless, as said, someone has another idea?

drunken monkey’s picture

Assigned: Unassigned » drunken monkey

Test fails are because I forgot to implement the new method for UnsavedIndexConfiguration. Duh.
On it again to also implement the override detection workaround.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
14.38 KB
22.26 KB

This should/might fix the tests and also contains the workaround for the preSave() methods.
If this works, only tests are still needed. Working on those.

drunken monkey’s picture

Status: Needs review » Needs work
drunken monkey’s picture

Status: Needs work » Needs review
FileSize
5.08 KB
22.26 KB

One character …

drunken monkey’s picture

Status: Needs review » Needs work
drunken monkey’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
23.37 KB
drunken monkey’s picture

Status: Needs review » Needs work
drunken monkey’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
23.38 KB

The ! character strikes again …
Maybe I should be sleeping.

drunken monkey’s picture

Status: Needs review » Needs work
drunken monkey’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
28.49 KB
49.67 KB

Tests are looking good, if I do say so myself. Let's hope the test bot god is feeling generous today. (Unfortunately, I still can't run functional tests locally. Should probably try to fix that soon.)

Status: Needs review » Needs work

The last submitted patch, 84: 2682369-84--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs review » Needs work

The last submitted patch, 86: 2682369-86--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
50.43 KB

Two rather silly errors, let's see whether this fixes them.

Status: Needs review » Needs work

The last submitted patch, 88: 2682369-88--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
Issue tags: +DevDaysSeville, +drupaldevdays
FileSize
7.9 KB
51.55 KB

Learned something new again, very interesting.
And annoying. Hopefully page requests in web tests have access to the test classes …

Status: Needs review » Needs work

The last submitted patch, 90: 2682369-90--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
51.48 KB

Status: Needs review » Needs work

The last submitted patch, 92: 2682369-92--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
4.88 KB
51.9 KB

Status: Needs review » Needs work

The last submitted patch, 94: 2682369-94--config_overrides.patch, failed testing.

drunken monkey’s picture

Debug patch to maybe find out what's going wrong.

Status: Needs review » Needs work

The last submitted patch, 96: 2682369-96--config_overrides--debug.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
667 bytes
51.9 KB

Ah. Stupid strikes again.
Interdiff compared to #94.

Status: Needs review » Needs work

The last submitted patch, 98: 2682369-98--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
51.95 KB

Status: Needs review » Needs work

The last submitted patch, 100: 2682369-100--config_overrides.patch, failed testing.

drunken monkey’s picture

Another debug excursion.

Status: Needs review » Needs work

The last submitted patch, 102: 2682369-102--config_overrides--debug.patch, failed testing.

drunken monkey’s picture

I'm not quite sure whether I should be happy or sad that all of these are such stupid little mistakes.
(Interdiff based on #100.)

Status: Needs review » Needs work

The last submitted patch, 104: 2682369-104--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review

(Disregard this.)

drunken monkey’s picture

Status: Needs review » Needs work

The last submitted patch, 107: 2682369-107--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs review » Needs work

The last submitted patch, 109: 2682369-109--config_overrides--debug.patch, failed testing.

drunken monkey’s picture

Now it's getting confusing again. I think I liked "stupid" better.

Status: Needs review » Needs work

The last submitted patch, 111: 2682369-111--config_overrides--debug.patch, failed testing.

drunken monkey’s picture

Oh, an actual bug! Even better … (All of this would be easier at home, where I at least have a web server running …)
Interdiff based on #104.

Status: Needs review » Needs work

The last submitted patch, 113: 2682369-113--config_overrides.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
6.36 KB
54.46 KB

Ah, OK, the input field default value apparently doesn't count as text, makes sense.
Probably this means we should make all the negative assertions search the whole HTML?

borisson_’s picture

responseContains is an option, or $this->getSession()->getPage()->findField($fieldName)->getValue();; that's more explicit I think.

drunken monkey’s picture

Ohmygodicantbelieveitactuallypassedohmygod!
Do you want to review?
Getting the field value explicitly would be a good alternative, yes, thanks! However, since we really only want to ensure that the saved value is used, not the overridden one, we don't actually care where we find the field value, I'd say. (As long as it's something that's not likely to pop up from elsewhere by pure chance.)
But if you much prefer your way, feel free to change it.

borisson_’s picture

Ohmygodicantbelieveitactuallypassedohmygod!

Yes, woo, great work!

I'd prefer to defer to someone with more knowledge on the topic to do a review, but I'll open the patch up when I'm done with work.

But if you much prefer your way, feel free to change it.

I don't care enough to roll a patch ;)

swentel’s picture

+++ b/search_api.drush.inc
@@ -5,6 +5,7 @@
@@ -530,6 +531,7 @@ function search_api_drush_set_index_state(IndexInterface $index, $enable = TRUE)

@@ -530,6 +531,7 @@ function search_api_drush_set_index_state(IndexInterface $index, $enable = TRUE)
     return;
   }
 
+  $index = _search_api_drush_reload_entity_override_free($index);
   $index->$method()->save();

It took me a while to understand why you want to load the override free version here in drush (and others). Since depending on the context (say dev vs prod), you may or may not want to use the overrides, so this change might lead to confusion too as this will save the config entity, but its status can be overridden, so the actual action might not even make a difference on the environment.

I probably miss the background for this change, but it seems unnecessary to me, especially since the admin routes will now load them /with/ the overrides.

I'm slowly looking through the other changes as well, expect more updates :)

drunken monkey’s picture

I probably miss the background for this change, but it seems unnecessary to me, especially since the admin routes will now load them /with/ the overrides.

If you save the overridden version, all of the overrides will be persisted, so you should always use the override-free version for saving.
E.g., you have the name overridden, you load the version with overrides, you change the status, you save: the overridden name is now stored in the database, too.
Figured this out pretty late myself (see the fails for #111), but it's pretty essential for this problem, actually. I went through the whole module looking for ->save( to make sure I get all instances.

swentel’s picture

Hmm you're right. Damn, overrides are really not a safe thing at all.

drunken monkey’s picture

Damn, overrides are really not a safe thing at all.

Yep. :D This issue in a nutshell.
That's also why Core ensures only override-free config can be saved (getEditable()), and why Alumei thinks it would be important that config entities follow the same principle.

Anyways, thanks a lot for reviewing! (Meant to add that in my previous comment, but forgot.)

bkosborne’s picture

I'm trying to get a grasp on all this. I think my problem is represented in this issue but I can't be sure:

I have a search index that is set to read_only and stored in config like that. On my production environment, I have this in my production-only settings.php file:

$config['search_api.index.acquia_search_index']['read_only'] = FALSE;

However, when cron runs on my production environment and indexes a few items, I'm observing that the overwritten config value is written into active configuration after cron completes, thus "poisoning" active config with a config override.

Does the patch above aim to resolve this, or is this a separate issue? It was sort of my understanding that this issue just pertained to issues relating to actions taken in the UI , but this was a cron run that executed some code and modified active config.

Assuming I instruct all the devs to NOT use the UI for any search API tasks, is there actually a safe way to have non-prod environments set to read only?

  • drunken monkey committed 229727a on 8.x-1.x authored by Alumei
    Issue #2682369 by Alumei, drunken monkey, swentel, Crell, stBorchert,...
drunken monkey’s picture

Title: Config overrides for search entities aren't used when performing operations in the admin UI » Fix problems with overridden config entities
Status: Needs review » Fixed

Yes, I think your issue should be solved by this. But to be sure – why not just apply the patch and see for yourself whether your issue is fixed by it? Would also help us a lot by verifying that the patch works as intended.
(The issue outgrew the "UI-only" part at some point, so it really should resolve all kinds of (or, ideally, all) problems related to overridden servers/indexes.)

Assuming I instruct all the devs to NOT use the UI for any search API tasks, is there actually a safe way to have non-prod environments set to read only?

Yes – by using this patch (I hope).

Anyways, in the interest of getting on with our path to the stable release, I'm just going ahead now and committing this. It would have been ideal to get a review by Alumei, or some other config system expert, but if we discover a remaining problem later, I'd say we can still fix that then. For now, the tests should suffice to ensure that we really did fix the problem, at least in all ways that I thought of testing.
So: committed.

Thanks a lot again to everyone involved who worked on this, reviewed, or helped us understand what the actual problems were – first and foremost, of course, to Alumei!

drunken monkey’s picture

Status: Fixed » Needs review
FileSize
2.17 KB

Need to re-open as this broke tests, due to a conflict with #2641388: AJAXify and generally improve the new Fields UI (apparently).

drunken monkey’s picture

Weird stuff: #2866130: "Waiting for branch to pass" can't be circumvented anymore.
Just taking a chance and committing this now.

Status: Needs review » Needs work

The last submitted patch, 126: 2682369-126--fix_tests.patch, failed testing.

drunken monkey’s picture

Status: Needs work » Fixed

Patch worked.

chriscalip’s picture

@bkosborne @drunken_monkey
Using rc2 and I still see bugs in play for divergences on indexes. I realize the complexity in play on nested indexes.
I had a problem with search index --> read only.
My advice to anyone encountering trouble with diverging between environments for search_api configs is to do it via servers.

/**
 * Enforce Live Search API "search_api.server.algolia_capacitype_server" -- assets2algolia & topics2algolia
 * Enforce Dev Search API "search_api.server.algolia_capacitype_server_for_dev" xxassets2algolia & xxtopics2algolia
 *
 * Only on live pantheon environment that search api (assets2algolia & topics2algolia) are enabled.
 * For Non-Live pantheon envinronment the search api (xxassets2algolia & xxtopics2algolia) are enabled
 *
 */
if (isset($_ENV['PANTHEON_ENVIRONMENT']) && $_ENV['PANTHEON_ENVIRONMENT'] === 'live' ) {
  $config['search_api.server.algolia_capacitype_server']['status'] = TRUE;
}
else {
  $config['search_api.server.algolia_capacitype_server_for_dev']['status'] = TRUE;
}

Have
(search_api server config) Live Server : live_index_blogs, live_index_pages
(search_api server config) Dev Server : dev_index_blogs, dev_index_pages

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.