Editing the configuration of configurable fields (e.g., "Aggregated fields" and "Rendered HTML output") will currently result in an uncaught exception (serializing the database connection), at least in some cases. This was most likely caused by #2650986: Fix entity loading in SearchApiFieldTrait.

CommentFileSizeAuthor
#2 2910918-2--fix_field_config_form.patch7.52 KBdrunken monkey
#2 2910918-2--fix_field_config_form--tests_only.patch5.03 KBdrunken monkey

Comments

drunken monkey created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Active » Needs review
StatusFileSize
new 2910918-2--fix_field_config_form--tests_only.patch5.03 KB
new 2910918-2--fix_field_config_form.patch7.52 KB

OK, apparently this is just due to the index being serialized with the $properties property, which can apparently contain a reference to the database under some circumstances. I regrettably wasn't able to reproduce this in the IntegrationTest, but just added a more direct check in the EntitySerializationTest. Also, doing this I stumbled over a few more bugs, and also improved IntegrationTest a bit, so still something, I'd say.

The last submitted patch, 2: 2910918-2--fix_field_config_form--tests_only.patch, failed testing. View results

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work

Only one thing that raised my eyebrows.

+++ b/tests/src/Kernel/System/SerializationTest.php
@@ -268,7 +293,7 @@ protected function createTestField($id = 'test', $datasource_id = NULL) {
-   * @return \Drupal\search_api\IndexInterface
+   * @return \Drupal\search_api\Entity\Index

Are you sure about this change?

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Thanks a lot for reviewing!

Are you sure about this change?

Kinda? I agree, normally we should use the interface. But a), we're explicitly testing that class, so it makes sense to specify it; and b), on a more practical note, __sleep() is otherwise "not found on this class" by the IDE, because it's of course not listed on the interface. So I think I'm more in favor of this change, though I know it looks a bit weird.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs work » Reviewed & tested by the community

Ok, that sounds good! I was just wondering to make sure that you didn't accidentally make that change :)

  • drunken monkey committed f3156f3 on 8.x-1.x 
    Issue #2910918 by drunken monkey, borisson_: Fixed error in fields...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Reviewed & tested by the community » Fixed

Sure, makes sense, of course. Thanks again for reviewing!
Committed.

Status: Fixed » Closed (fixed)

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