Problem/Motivation

During the usability study, we noticed that the widget settings exposes SQL syntax to the user with the uppercased "CONTAINS" on the Manage form display page:

This was also seen in an error message after the user tried to add a content page that had required a taxonomy term field.

Proposed resolution

The initial suggestion is to simply lowercase the word "CONTAINS". There might be other instances in which this occurs, as well as other SQL syntax verbiage that really doesn't need to be in uppercase in the UI.

Remaining tasks

None.

User interface changes

Lowercase text.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#17 Screen Shot 2015-07-03 at 8.42.30 PM.png56.64 KBdavidhernandez
#17 lowercase_the_word-2513612-17.patch2.1 KBdavidhernandez
Manage_form_display___drupal8.png11.31 KBivanstegic
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Issue tags: +Novice

Easy fix, easy issue! :) (As long as you can figure out where this text is coming from, which I haven't successfully yet.)

cilefen’s picture

cilefen CreditAttribution: cilefen commented

I know - I tried.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/EntityReferenceAutocompleteWidget.php

CONTAINS and STARTS_WITH should both be fixed, I assume.

dorficus’s picture

dorficus CreditAttribution: dorficus commented
Assigned: Unassigned » dorficus
ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented
Issue tags: -Novice

I don't think this is an easy fix, actually. The "CONTAINS" and "STARTS_WITH" SQL keywords are actually the Keys in a dropdown somewhere, and to me, at least at this high level, it looks like those KEYS are being used in the widget, and not the VALUES. The only reason I say this is because it looks like there are already translate-able values in the referenced file:

  public function settingsForm(array $form, FormStateInterface $form_state) {
    $element['match_operator'] = array(
      '#type' => 'radios',
      '#title' => t('Autocomplete matching'),
      '#default_value' => $this->getSetting('match_operator'),
      '#options' => array(
        'STARTS_WITH' => t('Starts with'),
        'CONTAINS' => t('Contains'),
      ),
      '#description' => t('Select the method used to collect autocomplete suggestions. Note that <em>Contains</em> can $
    );

Removing the novice tag because I couldn't find where this is in the code, neither could @webchick and because this really should be a translate-able, right?

dorficus’s picture

dorficus CreditAttribution: dorficus commented

I'll go ahead and take myself off of it then. Thanks, ivanstegic.

dorficus’s picture

dorficus CreditAttribution: dorficus commented
Assigned: dorficus » Unassigned
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

The text in question is printed below in settingsSummary().

$summary[] = t('Autocomplete matching: @match_operator', array('@match_operator' => $this->getSetting('match_operator')));
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

The settings form is what you actually see when you click the gear to change the settings.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

Right, so it seems like maybe we need a helper method that returns an array of settings in 'KEY' => t('Friendly') format, and then just call it from both places?

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

Sure. getMatchOperatorOptions() or something? Use the array directly in settingsForm() and search it in settingsSummary().

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented

Is this solution also going to fix the error message? IIRC this was in the error message as well.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

I'm not sure which error message that would be. Can you provide steps to produce it?

ivanstegic’s picture

ivanstegic CreditAttribution: ivanstegic at TEN7 commented

@webchick do you remember how this was reproduced? I was trying to remember and checked the spreadsheet but could not for the life of me figure it out. Maybe the video will contain it?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented

No, sorry... I didn't actually see it happen. :( I think it was either you or Link who noted it.

Looks like it happened in session 1 around 9:22am. See Lewis's column at https://docs.google.com/spreadsheets/d/1iCcj2E4F4Nsns_HkDUnutCgEKWQc7kEu...

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

Can someone translate that doc? It looks like the person was editing the term reference field, (that is where you would see the setting to make Tags the default vocabulary) but there are no settings related to the autocomplete there.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented
Title: Lowercase the word "CONTAINS" » Make the autocomplete form option text for an entity reference not be all caps
Issue summary: View changes
Status: Active » Needs review
FileSize
lowercase_the_word-2513612-17.patch2.1 KB
Screen Shot 2015-07-03 at 8.42.30 PM.png56.64 KB

This should at least function.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Component: field_ui.module » entity_reference.module
Status: Needs review » Reviewed & tested by the community
Issue tags: +D8 Accelerate London

That's the correct fix, thanks @davidhernandez!

cilefen’s picture

cilefen CreditAttribution: cilefen commented

Is there any need for an automated test on this? I think not, but it is worth discussing.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented

I also don't think that this specific issue is worth testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez as a volunteer commented

I don't think it needs a test.

  • webchick committed 5e557e6 on 8.0.x 
    Issue #2513612 by davidhernandez, ivanstegic, Bojhan, eliza411,...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Great!

Committed and pushed to 8.0.x. Another one bites the dust! :)

Status: Fixed » Closed (fixed)

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