Problem/Motivation

After a recent commit of a core issue (#2537928: MySQL index normalization only works on table create), I have test fails in any module that has SearchAPI as it's dependency. It complains that the required 4th parameter is missing in \Drupal\search_api_db\Plugin\search_api\backend\Database on line 583 and 586. The required parameter is a (partial) specification.

Proposed resolution

Add a matching 4th parameter.

Remaining tasks

Review patch, discuss, commit when ready.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#10 fix_database_backend-2545464-10.patch1.44 KBLKS90
#8 fix_database_backend-2545464-7.patch1.44 KBLKS90
#8 interdiff_5-7.txt348 bytesLKS90
#5 interdiff_1-5.txt0 bytesLKS90
#5 fix_database_backend-2545464-5.patch1.1 KBLKS90
#1 fix_database_backend-2545464-1.patch1.07 KBLKS90

Comments

LKS90’s picture

LKS90 commented
Status: Active » Needs review
StatusFileSize
new fix_database_backend-2545464-1.patch1.07 KB

Here is a patch, I used $this->database->schema()->getFieldTypeMap() to get an array that kind of resembles the example in Schema.php. It fixed the issue in a really simple test that only had SearchAPI as a dependency but didn't actually do that much, therefore, feedback is welcome!

LKS90’s picture

LKS90 commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 1: fix_database_backend-2545464-1.patch, failed testing.

LKS90’s picture

LKS90 commented

I think the ViewsTest fails are not related to this issue but this one: #2510564: Indexing is broken. No results are found and the page displays an error that complains about the database_search_index being unknown. The others are mostly accessing array indices that don't exist (anymore?).

LKS90’s picture

LKS90 commented
Status: Needs work » Needs review
StatusFileSize
new fix_database_backend-2545464-5.patch1.1 KB
new interdiff_1-5.txt0 bytes

Alright, I update the $table each time I add a field and use the $table as a specification when adding indices.

Edit: This patch made the BackendTest pass for me, so it would be nice to get it tested :).

Status: Needs review » Needs work

The last submitted patch, 5: fix_database_backend-2545464-5.patch, failed testing.

LKS90’s picture

LKS90 commented
Status: Needs work » Needs review

I fixed the schema errors. It's a tiny fix, so we can probably squeeze it in this patch. I fixed the schema errors by switching from type: mapping to type: config_entity, see the interdiff.

LKS90’s picture

LKS90 commented
StatusFileSize
new interdiff_5-7.txt348 bytes
new fix_database_backend-2545464-7.patch1.44 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/config/schema/search_api.schema.yml
@@ -1,5 +1,5 @@
 search_api.settings:
-  type: mapping
+  type: config_entity

This should be config_object, actually, as it's not a config entity, just a plain config object.

LKS90’s picture

LKS90 commented
Status: Needs work » Needs review
StatusFileSize
new fix_database_backend-2545464-10.patch1.44 KB

Switched to config_object.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Thanks a lot for reporting this issue and working on it, looks quite good already!
I've just made a few style changes, more or less, and then, since it passed for me locally, committed it. (I'm going on vacation tomorrow, for a week, so I thought it better to commit this right away.)

And yes, that test bot glitch really is annoying – not testing patches while HEAD is broken makes some kind of sense, I guess, but it's pretty stupid for actually fixing those fails.

+++ b/config/schema/search_api.schema.yml
@@ -1,5 +1,5 @@
 search_api.settings:
-  type: mapping
+  type: config_entity

It's config_object in this case, not config_entity. But interesting to see, didn't know about that change. Thanks for spotting this!

Edit: Ah, sorry, had the post and patch prepared even before Sascha posted, but the tests took so long to run … :-/

  • drunken monkey committed 73a55d2 on 8.x-1.x authored by LKS90 
    Issue #2545464 by LKS90, drunken monkey: Adapted to recent Core changes.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I guess that was a crosspost with the last two comments :)

Yeah, I can manually force patches to run on testbot with admin permissions, I usually try to do that in issues like this.

The problem with committing without that is that testbot will re-open the issue once testbot is green again and then this won't apply and will open the issue again :(

The last submitted patch, 8: fix_database_backend-2545464-7.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 10: fix_database_backend-2545464-10.patch, failed testing.

LKS90’s picture

LKS90 commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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