Problem/Motivation

Got '[object Object]' as initial machine name when:

  • the field label has a default value that must to be transliterated (contains spaces, etc.)
  • there are no 'exists' callback, or it is not valid

As an exists callback is commonly used, I mark it with minor priority.

I found a bug in machine-name.es6.js (line 116) that produces this behaviour.

File: core/misc/machine-name.es6.js
...
    109         // Determine the initial machine name value. Unless the machine name
    110         // form element is disabled or not empty, the initial default value is
    111         // based on the human-readable form element value.
    112         if ($target.is(':disabled') || $target.val() !== '') {
    113           machine = $target.val();
    114         }
    115         else if ($source.val() !== '') {
    116           machine = self.transliterate($source.val(), options);
    117         }
...

self.transliterate returns an object, not a string. At the same file:

...
    transliterate(source, settings) {
      return $.get(Drupal.url('machine_name/transliterate'), {
        text: source,
...

Sample form to reproduce

  $form['label'] = [
    '#type' => 'textfield',
    '#title' => t('Test label'),
    '#default_value' => 'Custom field label test',
    '#size' => 15,
    '#required' => TRUE,
  ];

  $form['machine'] = [
    '#type' => 'machine_name',
    '#size' => 15,
    '#description' => t('A unique machine-readable name containing letters, numbers, and underscores.'),
    '#required' => FALSE,
    '#maxlength' => 32,
    '#machine_name' => [
      'source' => [
        'label',
      ],
    ],
  ];

Proposed resolution

There is an issue that proposes to refactor the machine-name.es6.js: #1686174: Refactor machine-name.js. I did not test it but probably this bug is not present in the new code. However should be good to solve on current code in the meantime.

Remaining tasks

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

manuel.adan created an issue. See original summary.

manuel.adan’s picture

Status: Active » Needs review
FileSize
2.78 KB
andypost’s picture

Issue tags: +Needs tests, +JavaScriptTest
GrandmaGlassesRopeMan’s picture

- cleans up comment format
- fixes loose equal
- fixes unnamed function (should be arrow anyways)
- fixes duplicate variable name in scope

samuel.mortenson’s picture

The last submitted patch, 5: 2942663-5-test-only.patch, failed testing. View results

tedbow’s picture

+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameForm.php
@@ -48,6 +48,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+      '#default_value' => 'Yet another machine name',

Should we make this default use a comma or something to make sure it has a more complex value to convert for testing?

Otherwise looks great!

manuel.adan’s picture

FileSize
41.67 KB
36.26 KB

#5 works for me, I've tested it on transaction module, where the bug is really noticeable, as shown in the attached after / before screenshots

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

Re comment in #7. I realize that we already have specific tests for this \Drupal\Tests\system\Unit\Transliteration\MachineNameControllerTest

So what is there now is fine.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

The code comments got mixed up somehow(?)

  1. +++ b/core/misc/machine-name.es6.js
    @@ -109,12 +109,10 @@
             // Determine the initial machine name value. Unless the machine name
             // form element is disabled or not empty, the initial default value is
             // based on the human-readable form element value.
    +        // Initial machine name from field default value.
    

    What does this added line add to the docs?

  2. +++ b/core/misc/machine-name.es6.js
    @@ -136,6 +134,19 @@
    +         * Determine the initial machine name value. Unless the machine name
    +         * form element is disabled or not empty, the initial default value is
    +         * based on the human-readable form element value. If it is editable,
    +         * append an edit link.
    +         */
    +        if (machine === '' && $source.val() !== '') {
    +          self.transliterate($source.val(), options).done((machineName) => {
    +            self.showMachineName(machineName.substr(0, options.maxlength), eventData);
    +          });
    +        }
    

    We are not actually appending anything here, that happens below and already has that comment.

manuel.adan’s picture

Status: Needs work » Needs review
FileSize
6.15 KB
2.79 KB

Comments reviewed. Also initial value assignment according to the updated logic.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

manuel.adan’s picture

jmsosso’s picture

jmsosso’s picture

Status: Needs review » Reviewed & tested by the community

Also I saw a weird behavior, with or without the patch. Two AJAX request to machine_name/transliterate are fired for the same field. Because of #1686174 I didn't go deep into it.

The bug I saw in the original machine-name.es6.js is that, in line 126, the promise returned by trasliterate() is directly used as if it is the new machine name, but it isn't. It is an object.

} else if ($source.val() !== '') {
     machine = self.transliterate($source.val(), options);
}

But that is fixed with the patch.

The last submitted patch, 11: 2942663-11.patch, failed testing. View results

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

There are some prettier and eslint violations that should be fixed in the patch.


+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameForm.php
@@ -48,6 +48,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
+        'source' => ['machine_name_3_label']

Also, according to our coding standards, comma should follow the last multiline array item.

manuel.adan’s picture

Status: Needs work » Needs review
FileSize
5.93 KB
1.75 KB

CS and prettier review.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

jungle’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
78.28 KB
75.74 KB

Confirm the patch #18 fixed the known issue in transaction module: Machine name in field creation form takes an initial value of "[Object object]".

Drupal core: 8.8.1

Before:

before

After:

after

lauriii’s picture

Here's also a Drupal 9.0.x version of the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c9c542f and pushed to 9.0.x. Thanks!
Committed 847f9ec and pushed to 8.9.x. Thanks!

Didn't backport to 8.8.x because this bug is very minor and changes javascript which is not the best tested part of our ecosystem. So this gives us a chance to find out if this introduces anything unexpected.

  • alexpott committed c9c542f on 9.0.x
    Issue #2942663 by manuel.adan, samuel.mortenson, alwaysworking, lauriii...

  • alexpott committed 847f9ec on 8.9.x
    Issue #2942663 by manuel.adan, samuel.mortenson, alwaysworking, lauriii...

Status: Fixed » Closed (fixed)

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