Refactor machine-name.js

I'm taking over this task. Will return once a review is in order.

OK, refactoring done.

Now, when using machine-name.js you make a new Drupal.machineName(source_id, settings) which gives you an object to work with. There is a default _init function which implements the default behavior, and takes context as argument.

Its also possible to simply make use of some of the functionality in Drupal.machineName, such as the transliterate function, or eventDataHandler. I considered adding an example of how this was done, but i think thats more of a documentation thing than a coding thing. So perhaps we should add this to the d.o documentation for d8 instead.

Looking forward to some constructive reviews of my first ever patch to drupal :D

-- Lorique

Forgot to add "needs review"

@nod_

Alright, I should have implemented all the requested changes. I also changed source_id to sourceId just to keep things in line with the coding standard. I don't know why it was source_id before.

I tested locally, and it works as intended with the new changes, including caching the created objects.

- Added support for handling multiple errors using DrupalBehaviorError
- Implemented standardized extending behavior for Drupal.MachineName
- Renamed eventDataHandler to eventData because handlers should be events methods
- Made minor stylistic changes on request from @nod_

+++ b/core/misc/machine-name.js
@@ -27,103 +27,193 @@ Drupal.behaviors.machineName = {
+    ¶

There a a lot of whitespaces both on empty lines as at the end of sentences

+++ b/core/misc/machine-name.js
@@ -27,103 +27,193 @@ Drupal.behaviors.machineName = {
+    for (sourceId in settings.machineName) {
+      Drupal.MachineName.names.push(new Drupal.MachineName(sourceId, settings, context));
     }

Shouldn't we use if (settings.machineName.hasOwnProperty(sourceID))?

I found 1 line where it had whitespaces at the end of the line, and removed it. I also removed whitespaces on empty lines, these were added by my editor because it keeps my current identation level when i add a new line, and i didn't think they were important.

The if sentence you are referencing is in the constructor of the MachineName object.

Fixes are in the new patch.

+++ b/core/misc/machine-name.js
@@ -27,100 +27,189 @@ Drupal.behaviors.machineName = {
+   */    ¶
+  var self = this;

Just one white-space left.

+++ b/core/misc/machine-name.js
@@ -140,10 +229,12 @@ Drupal.behaviors.machineName = {
-  transliterate: function (source, settings) {
+  transliterate : function (source, settings) {

The : (colon) should not have a space before or is this a new code style?
(I'm new to javascript coding standards)

White space removed.

You're right, there should be no white space in front of the colon, my bad. I just checked with the coding standards. I just do it for readability, bad habit i guess :)

+})(jQuery);
\ No newline at end of file

+    var $preview = $('<span class="machine-name-value">' + options.field_prefix + Drupal.checkPlain(machine) + options.field_suffix + '</span>');

(readability) Could this be modified into a less longer line? Something like

+    var text = options.field_prefix + Drupal.checkPlain(machine) + options.field_suffix;
+    var $preview = $('<span class="machine-name-value">' + text + '</span>');

+    var $link = $('<span class="admin-link"><a href="#">' + Drupal.t('Edit') + '</a></span>').bind('click', eventData, self.clickEditHandler);

Don't we want to use .on() here? And again, divide over multiple lines?

+    var $link = $('<span class="admin-link"><a href="#">' + Drupal.t('Edit') + '</a></span>')
+                   .on('click', eventData, self.clickEditHandler);

+      eventData.$source.bind('keyup.machineName change.machineName', eventData, self.machineNameHandler)

Don't we want to use .on() here?

+        data.$preview.html(data.options.field_prefix + Drupal.checkPlain(machine) + data.options.field_suffix);

(readability) Could this be modified into a less longer line? Something like

+        var html = data.options.field_prefix + Drupal.checkPlain(machine) + data.options.field_suffix;
+        data.$preview.html(html);

changing status

Converting to the .on() method seems like a good idea, but we should do it everywhere as a group then. Perhaps _nod can confirm or deny if we want to do this?

I don't see a point in doing it unless we do it for all core js, to provide it as a guideline to everyone creating contrib.

New patch with some added readability as requested by Kiphaas7

I glanced over it quickly, and it looks pretty darn nice. The error handling is also nice!

One minor gripe, though this might be personal preference:

// Used for caching objects
$.extend(MachineName, {names : []});

// Used for caching objects
$.extend(MachineName, {
  names : []
});

BONUS: Investigate if adding basic events and detach makes sense, as described in #1763812: [META] Provide complete attach/detach with basic events

New patch up. Bind has been replaced by .on, and at the request of Kip i have beautified the extend section where names cache are added.

Patch failed testing because there was a change to machine-name.js from a different patch. I found it and brought the one line fix into my refactoring.

+})(jQuery);
\ No newline at end of file

Needs to end with a newline

New line added at end of file

I'm confused as to why the hasOwnProperty check resides within the constructor, why not check for it before creating a new instance?

+++ b/core/misc/machine-name.jsundefined
@@ -27,100 +27,193 @@ Drupal.behaviors.machineName = {
+    var machine = Drupal.MachineName.prototype.transliterate($(e.target).val(), data.options);

why is this being called on the prototype and not on the instance?

patch no longer applies

@nod_ should we use this man for js testing? https://www.drupal.org/node/1780104

Rerolled patch from #25

Not sure if this is in scope for this issue or not... But I was having problems with the transliterate function flooding the server with requests when typing in the field that generates a machine name. If you're like me and end up correcting your spelling a lot, the machine name JS falls so far behind that I've ended up submitting the form with only part of the machine name entered correctly.

I added a cache and a delay so that the server is only pinged once ever 250ms. Bonus: we could make this delay configurable. Bigger bonus: we should cache based on the source and settings parameters in addition to the delay.

What do you think? Is this outside the scope of this issue?

@_nod: Thanks for the call on in vs. out of scope. I figured as much... I've opened a new issue: #2353015: machine-name.js floods server with requests.

And thank you for the code review! Re: where to store values associated with this: This change would attach variables to the MachineName function at the global scope, not the behavior object. Or am I missing something? (Javascript is not my strong suit...)

re: Yoda conditionals: sorry about that -- old habit, though the code style guide is silent on the issue. Agreed, I couldn't find other examples in core so we shouldn't be introducing them here. Anyhow, I've fixed them in the patch on the issue referenced above.

Removing my uploads as that part has been moved elsewhere. Also resetting the status to where it was before I stuck my nose into this issue. :)

rerolling a patch :)

Also, the reroll should be against #33 as the changes I introduced in #36 were deemed out of scope. #43 is rerolling the wrong patch.

Rerolled the patch on the basis of #33 comment.

As already mentioned by @mikeker, all the changes that he has introduced in #36has been deemed out of scope.

@Nitesh Sethia Thanks for rerolling the patch. Please check my comments below.

+++ b/core/misc/machine-name.js
@@ -38,6 +38,7 @@
+<<<<<<< HEAD

@@ -49,109 +50,194 @@
+=======
...
+>>>>>>> Issue #1686174 by lorique, mikeker, Temoor, Manjit.Singh: Refactor machine-name.js

Can you Please check this in your patch

@Manjit didnt realise that I forgot to resolve the merge conflict.
Working on the patch.

Needs JavaScript test case for big refactoring and new features

I created a JavaScript test for the original machine-name.js and looked into the refactoring for this file.

As the original refactor is a bit outdated I decided to look into it again. The result is a bigger refactor than the first attempt.

The main difference is:

• Seperation of UI, Drupal behavior and machine name logic. In theory the conversion of a title is now useable outside of the machine name field.
• Better (?) code separation in various methods.
• Merged all actions for converting a string to a machine name into 1 function.
• Proper behavior attach AND detach.
• More documentation, all methods as JSdoc and there are more (useful) comments in code.
• Small performance optimization (less DOM interactions on init and no timeout when server side transliteration is not required).

The rewrite still passes the created test but the test should ofcourse be reviewed separately.

This post only contains the test. The next comment will contain the test and rewrite.

Test + refactor.

Now in the correct format (hopefully).

Welcome to JS World in Drupal.

I think you might be interested in this mission, take a look: #2809281: Use ES6 for core JavaScript development

Not a full review but few basic typo:

1. +++ b/core/misc/machine-name.js
   @@ -12,7 +12,8 @@
   +   * @prop {Drupal~behaviorAttach} a
   +   *ttach
   

   typo

2. +++ b/core/misc/machine-name.js
   @@ -43,165 +44,233 @@
   +    detach: function (context, settings) {
   ...
   +        var $source = $context.find(source_id);
   

   $context, not defined yet?

3. +++ b/core/misc/machine-name.js
   @@ -43,165 +44,233 @@
   +  Drupal.machineName.prototype.convert = function (source, callback) {
   

   Needs a better name I think

4. +++ b/core/misc/machine-name.js
   @@ -43,165 +44,233 @@
   +        this.xhr = $.get(Drupal.url('machine_name/transliterate'), {
   

   separated function for Transliteration

5. +++ b/core/misc/machine-name.js
   @@ -43,165 +44,233 @@
   +    detach: function (context, settings) {
   

   I expected one function call

Also, run ESLint :)

Thanks for the quick feedback!

I'll start reading into the ES6 for core issue and run ESlint. After that I'll create another version that hopefully matches the new core standards.

Discussed the test with @michielnugter offline, here are the improvements we came up with:

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,103 @@
   +    // Visit the add role page which contains a machine name field.
   +    $this->drupalGet('admin/people/roles/add');
   

   Please add a form to the form_test module to test this so you can lose the dependency on the user module

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,103 @@
   +      $transliterated = $this->transliteration->transliterate($test_value);
   +      $transliterated = Unicode::strtolower($transliterated);
   +      $transliterated = preg_replace('@' . strtr('[^a-z0-9_]+', [
   +          '@' => '\@',
   +          chr(0) => ''
   +        ]) . '@', '_', $transliterated);
   

   Use hardcoded expected values so any change in the output of the transliteration service is detected

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,103 @@
   +    $this->assertTrue($id->isVisible(), 'The ID field must now be visible');
   

   super nitpick assertTrue is deprecated

Thanks for all great work. Could you both review this simple patch also. It also helps this refactoring.

#2668596: No delays update for Machine Name

I updated the testcase to fix the mentioned issues. I didn't include an interdiff because the change is quite large.

Changes to the testcase:
- No more dependencies on the Transliteration service.
- Custom test form for the machine name so no dependency on the user add role form.
- No more deprecated testing methods
- Added testing for 2 machine name fields on a page
- Cleanup and coding standards issues fixed

I reviewed the other patch and will post my findings next, thank you for asking me to review and involving me in the process.

I have a question: as multiple issues now require a testcase for the machine name, is it maybe better to split the testcase into a separate issue and postpone this issue and the other issue(s) until the testcase is in?

As for the rewrite: I'm diving deep into ES6, Mocha and other changes happing to JavaScript in core, exiting stuff! I'll try to master the ES6 and Mocha a bit and then post a proposal for a ES6 version for the machine-name.js. Maybe the rewrite would also be a usefull testcase for implementing Mocha unit testing.

I have a question: as multiple issues now require a testcase for the machine name, is it maybe better to split the testcase into a separate issue and postpone this issue and the other issue(s) until the testcase is in?

Yes, we can split the test to a new issue (in fact, it should be. this is a standard test, not just for this refactoring). and keep this active also. No conflicts :)

1. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestMachineNameForm.php
   index 3199c45..c359ea2 100644
   --- a/core/modules/user/src/RoleForm.php
   
   --- a/core/modules/user/src/RoleForm.php
   +++ b/core/modules/user/src/RoleForm.php
   
   +++ b/core/modules/user/src/RoleForm.php
   +++ b/core/modules/user/src/RoleForm.php
   @@ -15,26 +15,31 @@ class RoleForm extends EntityForm {
   

   Don't think these changes to RoleForm were intended. They cause the test fails.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,113 @@
   +    $this->assertEquals(TRUE, $machine_name_1_field->isVisible(), 'The ID field must now be visible');
   

   We should add a check before the click that the element is not visible before the click so we are actually detecting a change (Tried this and it fails, isVisible() is wonky)

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,113 @@
   +    $this->assertEquals($test_info['expected'], $machine_name_1_field->getValue(), 'The ID field value must be equal to the php generated machine name');
   

   $test_info is set in the foreach and now used outside it, not so nice.

Love the two machine name fields on the same page cause it shows the current code is broken for multiple machine name fields on a page:

Only one edit link!

And ++ on splitting off the test and opening a new issue for that and postponing the other issues/refactor on getting that in first.

1. Wow, whoops.. Glad to have tests :)
2. Completely right, a before check must be added. As to why the isVisible() doesn't work: the element is not actually hidden but positioned out of the way (to make sure any containing value is submitted to the server on submit). I changed the check to validate for the visually-hidden class.
3. Very true, I restructured the testinfo array so I don't need to be dirty anymore..

I'll post my new version with interdiff in a newly created issue.

Set to postponed untill the test is in.

So after reading and learning a lot I found out ES6 is very nice indeed!

I'm not completely familiar with ES6 yet but I tried my best at a version of machine-name.js is ES6... Something to shoot at and it almost certainly not the thing you're looking for but hey, it might just be a start :)

Some explaination:
- I used classes and separated the machine name and transliteration into separate classes.
- I tried a modular approach although it currently doesn't work (I can't get Drupal to add the type="module" yet.
-- eslint is complaining about the import/export
- I tried to find a directory structure to place the classes in.
- The MachineName class is a minimal effort convert to ES6 and classes, much of the original code is still there. I thought it a little less relevant for now.

The first patch is how I intented it but that one doesn't work. The -working version actually works in browsers supporting ES6. I intentionally didn't use babel yet to convert them to see if the ES6 version actually works.

One important downsides to classes is that they're less easily partially overwritten. Views UI overwrites some drag-n-drop logic to add it's own stuff. This would be harder when using classes.

Very curious to hear what you (@droplet / @nod_ / anyone else with ES6 knowledge) think of this approach!

BTW: I am planning on adding a unit test using Mocha, that's the next step for me. I really want and would like to contribute to th ES6 effort.