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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mollux’s picture

Status: Active » Needs review
FileSize
3.96 KB
91.41 KB
42.93 KB

First 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.

Nick_vh’s picture

  1. +++ b/search_api.theme.inc
    @@ -68,6 +68,57 @@ function theme_search_api_admin_fields_table($variables) {
    +  $header = array(t('Data Type'), t('Description'), t('Fallback data type'), t('Supported by server\'s backend?'));;
    

    ;;;;; too many :)

  2. +++ b/src/Form/IndexFieldsForm.php
    @@ -107,6 +107,18 @@ class IndexFieldsForm extends EntityForm {
    +    $fallback_mapping = Utility::getDataTypeFallbackMapping($index);
    +
    

    remove empty line

  3. +++ b/src/Form/IndexFieldsForm.php
    @@ -130,6 +142,14 @@ class IndexFieldsForm extends EntityForm {
    +      drupal_set_message("Some of the used data types aren't supported by the server's backend. They are marked with a * and the fallback type will be used when indexing.", 'warning');
    

    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.

mollux’s picture

FileSize
3.99 KB

All 3 fixed :)

mollux’s picture

There was a typo in the Utility class in the default data types array, generating errors and failing tests.
But fixed now :)

The last submitted patch, 1: 2473211-1--warning-unsupported-data-types.patch, failed testing.

The last submitted patch, 3: 2473211-3--warning-unsupported-data-types.patch, failed testing.

Nick_vh’s picture

Status: Needs review » Reviewed & tested by the community

Good to go

drunken monkey’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
FileSize
7.16 KB
6.76 KB

Good work again, thanks a lot!
However, as always, since I'm an old man I like to complain about stuff:

  1. +++ b/search_api.theme.inc
    @@ -68,6 +68,57 @@ function theme_search_api_admin_fields_table($variables) {
    +  $header = array(t('Data Type'), t('Description'), t('Fallback data type'), t('Supported by server\'s backend?'));
    

    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.

  2. +++ b/src/Form/IndexFieldsForm.php
    @@ -106,6 +106,17 @@ class IndexFieldsForm extends EntityForm {
    +    $form['data_type_explanation'] = array(
    +      '#type' => 'details',
    

    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.)

  3. +++ b/src/Form/IndexFieldsForm.php
    @@ -106,6 +106,17 @@ class IndexFieldsForm extends EntityForm {
    +      '#title' => 'Data types',
    

    This and other places: Forgot the t() call.

  4. +++ b/src/Form/IndexFieldsForm.php
    @@ -129,6 +140,14 @@ class IndexFieldsForm extends EntityForm {
    +    if (count($fallback_types)) {
    +      foreach ($fallback_types as $data_type => $fallback) {
    +        $types[$data_type] .= ' * [' . $types[$fallback] . ']';
    +      }
    +      drupal_set_message("Some of the used data types aren't supported by the server's backend. They are marked with a * and will be indexed using the data fallback type that is listed next to it.", 'warning');
    +    }
    

    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.

Nick_vh’s picture

Status: Needs review » Needs work
dermario’s picture

Assigned: mollux » Unassigned
Status: Needs work » Needs review
FileSize
10.91 KB

I 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?

Status: Needs review » Needs work

The last submitted patch, 10: make_users_aware_of-2473211-10.patch, failed testing.

The last submitted patch, 10: make_users_aware_of-2473211-10.patch, failed testing.

dermario’s picture

A 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 :)

drunken monkey’s picture

Thanks 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.

+++ b/search_api.module
@@ -252,6 +252,10 @@ function search_api_theme() {
       'function' => 'theme_search_api_form_item_list',
+    ),
+    'search_api_admin_data_type_table' => array(
+      'variables' => array('data_types' => array(), 'fallback_mapping' => array()),
+      'function' => 'theme_search_api_admin_data_type_table',
       'file' => 'search_api.theme.inc',

This seems to remove the 'file' key from the previous theme entry.

dermario’s picture

Thank 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.

This seems to remove the 'file' key from the previous theme entry.

Hmm yes, seems like i didn't resolve the conflict properly. I will fix that too.

dermario’s picture

Sorry guys i need some guidance here.

Assume following new code in IndexFieldsForm::buildForm():

    // Retrieve all datatype plugins.
    $instances = $this->dataTypePluginManager->getInstances();
    $data_types = [];
   
    foreach($instances as $name => $type) {
      $data_types[$name] = [
        'label' => $type->label(),
        'supported' => $this->entity->getServerInstance()->supportsDataType($name),
      ];
    }

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?

dermario’s picture

Assigned: Unassigned » dermario
dermario’s picture

Assigned: dermario » Unassigned
FileSize
4.18 KB
53.08 KB

I'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. :-)

dermario’s picture

I 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:

/**
 * Provides an unsupported data type.
 *
 * @SearchApiDataType(
 *   id = "unsupported",
 *   label = @Translation("Unsupported"),
 *   fallback_type = "integer"
 * )
 */
class UnsupportedDataType extends DataTypePluginBase {

}

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.

drunken monkey’s picture

Great 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 returns FALSE without checking.

  1. +++ b/search_api.module
    @@ -249,6 +249,10 @@ function search_api_theme() {
    +    'search_api_admin_data_type_table' => array(
    +      'variables' => array('data_types' => array(), 'fallback_mapping' => array()),
    +      'function' => 'theme_search_api_admin_data_type_table',
    +    ),
    

    Now the 'file' key is missing from the new entry.

  2. +++ b/src/Form/IndexFieldsForm.php
    @@ -246,6 +269,18 @@ class IndexFieldsForm extends EntityForm {
    +          drupal_set_message($this->t("Some of the used data types aren't supported by the server's backend. See the <a href=\"@url\">data types table</a> to find out what types are supported.", array('@url' => '#search-api-data-types-table')), 'warning');
    

    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.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.1 KB
8.16 KB

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.

  • drunken monkey committed 845e707 on 8.x-1.x authored by mollux
    Issue #2473211 by mollux, dermario, drunken monkey, borisson_: Added an...
drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Great, 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!

Status: Fixed » Closed (fixed)

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