the initialization is a big mess, there is an unrelated method in the Drupal.behavior.machineName object added.

Needs to be taken apart, and added to Drupal.machineName for easier reuse and a more modular approach.

Files: 

Comments

lorique’s picture

Assigned: Unassigned » lorique

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

lorique’s picture

FileSize
9.79 KB
PASSED: [[SimpleTest]]: [MySQL] 40,633 pass(es). View

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

lorique’s picture

Status: Active » Needs review

Forgot to add "needs review"

nod_’s picture

Status: Needs review » Needs work

Yep, I like the separation of the different methods.

A couple of things, have a look at the tableheader.js file, there is the same kind of pattern we should be using here.

first the main constructor Drupal.MachineName = function () {}; (it needs to have a capital letter first)
Then we add a cache of all the created objects with $.extend(Drupal.MachineName, {names : []});
Finally we add all the methods to the object: Drupal.MachineName.prototype = {evenDataHandler: function () {}, clickEditHandler: function () {}};

And everything you have in the _init function can directly be in the constructor Drupal.MachineName. so in the attach loop (which should be _after_ the Drupal.MachineName it's not after in the tableheader file so we can keep it that way.) you'll do something like Drupal.MachineName.names.push(new Drupal.MachineName(sourceId, settings, context));

Still, that's a good start, thanks!

lorique’s picture

Status: Needs work » Needs review
FileSize
10.76 KB
PASSED: [[SimpleTest]]: [MySQL] 40,631 pass(es). View

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

lorique’s picture

FileSize
10.62 KB
PASSED: [[SimpleTest]]: [MySQL] 40,630 pass(es). View

- 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_

clemens.tolboom’s picture

Status: Needs review » Needs work
+++ 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))?

lorique’s picture

Status: Needs work » Needs review
FileSize
10.51 KB
PASSED: [[SimpleTest]]: [MySQL] 40,629 pass(es). View

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.

clemens.tolboom’s picture

Status: Needs review » Needs work
+++ 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)

lorique’s picture

Status: Needs work » Needs review
FileSize
10.34 KB
PASSED: [[SimpleTest]]: [MySQL] 40,628 pass(es). View

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

aspilicious’s picture

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

Kiphaas7’s picture

+    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);
Jelle_S’s picture

Status: Needs review » Needs work

changing status

lorique’s picture

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.

nod_’s picture

We're in the process of changing everything to .on and .off, go for it. Also, we won't use .click and related shortcuts anymore. If you find some around change them as well please :)

lorique’s picture

Status: Needs work » Needs review
FileSize
10.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_core-refactor_matchine_name-1686174-15.patch. Unable to apply patch. See the log in the details link for more information. View

New patch with some added readability as requested by Kiphaas7

Status: Needs review » Needs work

The last submitted patch, drupal_core-refactor_matchine_name-1686174-15.patch, failed testing.

Kiphaas7’s picture

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 : []
});
Kiphaas7’s picture

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

lorique’s picture

Status: Needs work » Needs review
FileSize
10.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_core-refactor_matchine_name-1686174-20.patch. Unable to apply patch. See the log in the details link for more information. View

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.

Status: Needs review » Needs work

The last submitted patch, drupal_core-refactor_matchine_name-1686174-20.patch, failed testing.

lorique’s picture

FileSize
10.45 KB
PASSED: [[SimpleTest]]: [MySQL] 41,104 pass(es). View

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.

lorique’s picture

Status: Needs work » Needs review
aspilicious’s picture

Status: Needs review » Needs work

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

Needs to end with a newline

lorique’s picture

FileSize
10.41 KB
PASSED: [[SimpleTest]]: [MySQL] 41,102 pass(es). View

New line added at end of file

lorique’s picture

Status: Needs work » Needs review
nod_’s picture

Issue tags: +Needs JS testing

tag

seutje’s picture

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

seutje’s picture

Status: Needs review » Needs work
+++ 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?

martin107’s picture

Issue summary: View changes
Issue tags: +Needs reroll

patch no longer applies

m1r1k’s picture

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

nod_’s picture

Assigned: lorique » Unassigned

for now we're going manual. The testswarm stuff is on the back burner.

Temoor’s picture

FileSize
10.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,144 pass(es). View

Rerolled patch from #25

Temoor’s picture

Status: Needs work » Needs review
m1r1k’s picture

Issue tags: +#ams2014contest
mikeker’s picture

Issue tags: -Needs reroll, -
FileSize
12.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,021 pass(es). View
1.57 KB

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?

mikeker’s picture

mikeker’s picture

nod_’s picture

Status: Needs review » Needs work

Thanks for the improvement but that's clearly a new feature. It's out of scope for this issue. Please open a new issue with the patch.

On the patch itself I have a couple of comments. We're adding yet more things to the behavior object (which is not encouraged) instead of exposing that in it's own object with explicit and documentable configuration. One nitpick, the conditions are backwards, please refer to the rest of the source code, we don't have yoda conditions anywhere else in the JS.

Clearly a needed improvement but out of scope for this :)

mikeker’s picture

@_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.

mikeker’s picture

Status: Needs work » Needs review

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

nod_’s picture

Issue tags: +Needs reroll
Manjit.Singh’s picture

FileSize
12.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,776 pass(es). View

rerolling a patch :)

googletorp’s picture

Issue tags: -Needs reroll
nod_’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Reroll needs work, it doesn't apply properly, the resulting JS code is broken.

mikeker’s picture

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.

Nitesh Sethia’s picture

Status: Needs work » Needs review
FileSize
12.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,773 pass(es). View

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’s picture

Issue tags: -Needs reroll
Manjit.Singh’s picture

Status: Needs review » Needs work

@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

Nitesh Sethia’s picture

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

Nitesh Sethia’s picture

Status: Needs work » Needs review
FileSize
11.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,798 pass(es). View

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

droplet’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Needs work
Issue tags: +JavaScriptTest

Needs JavaScript test case for big refactoring and new features

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
3.81 KB

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.

michielnugter’s picture

Status: Needs review » Needs work

The last submitted patch, 56: refactor_machine_name_js_1686174_56.patch, failed testing.

michielnugter’s picture

Now in the correct format (hopefully).

droplet’s picture

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

michielnugter’s picture

Status: Needs review » Needs work

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.

Lendude’s picture

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

droplet’s picture

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

michielnugter’s picture

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.

michielnugter’s picture

Status: Needs work » Needs review
michielnugter’s picture

Status: Needs review » Needs work
droplet’s picture

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

The last submitted patch, 63: refactor_machine_name_js_1686174_62-test.patch, failed testing.

Lendude’s picture

  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.

michielnugter’s picture

Status: Needs work » Postponed
Related issues: +#2821320: Add test coverage for machine-name.js

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.

michielnugter’s picture

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.

The last submitted patch, 70: refactor_machine_name_js_1686174_70.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 70: refactor_machine_name_js_1686174_70-working.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.