Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
As a follow-up of #2080791: Make data types into plugins, the Manage fields page should warn the user if the index uses data types that are not supported by the sever's backend. We also may add some overview page with all the build-in Search API and custom data types that are available, so users can understand what "Fulltext" means.
Comment | File | Size | Author |
---|---|---|---|
#21 | make_users_aware_of-2473211-21.patch | 8.16 KB | borisson_ |
Comments
Comment #1
mollux CreditAttribution: mollux commentedFirst try for this, I also attached a screenshot of how it looks.
I think for the moment this is enough, as the whole fields page has to have an overhaul anyway.
Comment #2
Nick_vh;;;;; too many :)
remove empty line
I read this that "THE" fallback type will be used. I would change this to say:
They are marked with a * and will be indexed using the data fallback type that is listed next to it.
Comment #3
mollux CreditAttribution: mollux commentedAll 3 fixed :)
Comment #4
mollux CreditAttribution: mollux commentedThere was a typo in the Utility class in the default data types array, generating errors and failing tests.
But fixed now :)
Comment #7
Nick_vhGood to go
Comment #8
drunken monkeyGood work again, thanks a lot!
However, as always, since I'm an old man I like to complain about stuff:
Not sure myself, but I think swapping the fallback and the support column would make things clearer for the user. That way it might be easier to see that only unsupported types show their fallbacks.
Even better, maybe we can just show the fallbacks for the unsupported types, if there are any, and otherwise hide the column?
Finally, "Supported by server's backend?" is quite a mouthful for a column header.
This should also have a description, I'd say, so users know what it's doing there. (Can be shorter, though, if we already have an explanation in the help text at the top.)
This and other places: Forgot the
t()
call.Shouldn't that just be shown if one of the fields actually does use one of the types? Showing a warning when everything is fine doesn't seem like a good UI pattern to me.
If you really want that information there, just include it in the general description.
Also, if we have the list of types, descriptions and fallbacks on the same page anyways (which is a good idea, I think – this way it can be index-specific and thus provide clearer information), I don't think listing the fallback type next to unsupported ones is necessary. Just link to the "Data types" fieldset in the warning (if its displayed) and keep the addition to the simple asterisk. Otherwise, the select list gets pretty wide.
(Just my opinion, though – if you two think the fallback types should be there, that's fine, too.)
Also, ideally, this would have a few lines just checking that the table is there and looking fine in
IntegrationTest
. But we can also do that in a (novice-level) follow-up.Comment #9
Nick_vhComment #10
dermarioI don't know what the status of this issue is, but after rerolling the patch in #8 i noticed, that all changes mentioned there are already in the patch. I should have read the interdiff ;)
I attached the rerolled patch. Is there any more to do here?
Comment #13
dermarioA i see that it needs some more work to do here. Before i start exploring and fixing i want to make sure, that this feature is still used. Thank you in advance :)
Comment #14
drunken monkeyThanks for working on this! Yes, as far as I'm aware this is still needed and desired.
However, we changed the way default data types are defined, back in #2493599: Make our data type system more easily understandable, so probably that causes the problems.
This seems to remove the 'file' key from the previous theme entry.
Comment #15
dermarioThank you for pointing me to right direction. Although its more complicated now, its a good point to start learning search_api and its plugin datatypes.
Hmm yes, seems like i didn't resolve the conflict properly. I will fix that too.
Comment #16
dermarioSorry guys i need some guidance here.
Assume following new code in IndexFieldsForm::buildForm():
I thought i could go with that to retrieve all fields and the status for the backend support but the supportsDataType Method always returns false in my case. I enabled *search_api_db* and *search_api_db_defaults*. In my case BackendPluginBase::supportsDataType() is called and that always returns false. I found no implementation (except in TestBackend) that would work. Is my approach wrong?
Comment #17
dermarioComment #18
dermarioI'd really like to work on here but i am stuck. In my opinion the supportsDataType() - Method should give us exact the information we need to for display. But this method seems to be implemented nowhere. I attached my current work as do-not-test.patch Maybe this helps someone giving feedback. :-)
Comment #19
dermarioI gave myself another try and i think i got it.
I removed the "description" column because its not part of the DataType plugin definition (any more):
The "Unsupported" - datatype was created locally for testing:
And here's the message if a index contains fields with unsupported data types:
I also added an integration test. I am very curious about your feedback.
Comment #20
drunken monkeyGreat job there, thanks a lot!
Did you figure out
supportsDataType()
, or did you just restructure so you don't need it?In the latter case: it only works for non-default data types, the defaults are always supported. (See
Utility::getDataTypeFallbackMapping()
for a usage example.) And since the database backend doesn't support any non-default data types at the moment, it just returnsFALSE
without checking.Now the
'file'
key is missing from the new entry.It should be
:url
.The attached patch should fix all of that, and would be RTBC from my point of view, I think.
One question would be, though, whether we don't want to re-add the data type descriptions after all, now that we have a place to display them. I guess I completely forgot about this issue while discussing #2596471: Use data type descriptions. And I think having the descriptions there could add a lot of value.
Comment #21
borisson_I haven't manually tested this, I did find a small coding style error and changed a short array syntax to long syntax for module consistency. Overall this looks good and that's why I've changed this to rtbc.
Comment #23
drunken monkeyGreat, thanks for reviewing, and for spotting those two mistakes!
Committed. Let's just discuss descriptions in the other issue.
Thanks again to everyone who worked on this!