As a follow-up to #567832: Transliteration in core, we now have a Transliteration class in Core.

This issue is to:

a) Figure out where this class should be used in core. Ideas:
- Uploaded files [??? not sure how/where? Check the existing contrib module to see?]
- Machine name generation
- ???

b) Patch core to use this class in these spots.

Remaining tasks

  • (done in #22) routing review
  • (done in #43) js review
  • (done in #47) code standards review

API changes

new \Drupal\system\MachineNameController class

UI changes

screenshot from #25

New content type

content-type-translitirate.png

New vocabulary

vocabulary-translitirate.png

Follow-up Issues

Files: 
CommentFileSizeAuthor
#72 d8-machinename-1842718-71.patch9.68 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-machinename-1842718-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#72 interdiff-63-71.txt596 bytesYesCT
#63 60-63-interdiff.txt646 bytesalexpott
#63 d8-machinename-1842718-63.patch9.72 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,865 pass(es).
[ View ]
#60 53-60-interdiff.txt4.37 KBalexpott
#60 d8-machinename-1842718-60.patch9.71 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es).
[ View ]
#53 d8-machinename-1842718-53.patch7.05 KBalippai
PASSED: [[SimpleTest]]: [MySQL] 53,980 pass(es).
[ View ]
#53 d8-machinename-1842718-inter.txt2.53 KBalippai
#51 d8-machinename-1842718-51.patch15.76 KBalippai
PASSED: [[SimpleTest]]: [MySQL] 53,608 pass(es).
[ View ]
#51 d8-machinename-1842718-inter.txt3.66 KBalippai
#48 interdiff.txt2.1 KBandypost
#48 d8-machinename-1842718-48.patch7.19 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 53,299 pass(es).
[ View ]
#43 core-js-transliteration-1842718-42.patch7.19 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 53,335 pass(es).
[ View ]
#43 interdiff.txt1.52 KBnod_
#40 1842718-interdiff-40.txt2.06 KBandypost
#40 d8-machinename-1842718-40.patch7.02 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 53,314 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#33 d8-machinename-1842718-6.patch7.02 KBalippai
PASSED: [[SimpleTest]]: [MySQL] 52,860 pass(es).
[ View ]
#33 interdiff.txt511 bytesalippai
#29 d8-machinename-1842718-6-do-not-test.patch7.02 KBalippai
#26 d8-machinename-1842718-5.patch7.01 KBalippai
PASSED: [[SimpleTest]]: [MySQL] 52,242 pass(es).
[ View ]
#25 content-type-translitirate.png23.15 KBandypost
#25 vocabulary-translitirate.png12.77 KBandypost
#23 d8-machinename-1842718-4.patch7.03 KBalippai
PASSED: [[SimpleTest]]: [MySQL] 50,847 pass(es).
[ View ]
#20 d8-machinename-1842718-3.patch7.17 KBalippai
PASSED: [[SimpleTest]]: [MySQL] 50,787 pass(es).
[ View ]
#19 d8-machinename-1842718-2.patch4.89 KBalippai
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#13 d8-machinename-1842718.patch5.66 KBalippai
PASSED: [[SimpleTest]]: [MySQL] 49,045 pass(es).
[ View ]
#11 d8-machinename-1842718.patch5.58 KBalippai
FAILED: [[SimpleTest]]: [MySQL] 49,077 pass(es), 0 fail(s), and 78 exception(s).
[ View ]
#10 d8-machinename-1842718.patch5.58 KBalippai
FAILED: [[SimpleTest]]: [MySQL] 49,075 pass(es), 0 fail(s), and 80 exception(s).
[ View ]

Comments

Issue tags:+D8MI

I think the posted two ideas (uploaded files, machine names) sound like a good start :)

I'm afraid that if we don't act on this soon, we'll have no actual transliteration in core :/

yet i already see a smaller victory in having the module in core anyways. lets bring up morale ;)

@klonos: want to get started with a simple use case porting code from the contrib module to D8?

To apply transliteration on file names require to add a {file_managed}.original_filename column like File (field) paths does

I'm now working on the machine-name.js rewrite.

Component:base system» transliteration system

New issue component! :)

tagging

StatusFileSize
new5.58 KB
FAILED: [[SimpleTest]]: [MySQL] 49,075 pass(es), 0 fail(s), and 80 exception(s).
[ View ]

An initial patch with sample for file upload and with a machine name controller.

machine-name.js rewrite is more challenging than I thought, have to dive into backbone.js.
I'll try to do my best, but it can take a while. Any help appreciated.

Status:Active» Needs review
StatusFileSize
new5.58 KB
FAILED: [[SimpleTest]]: [MySQL] 49,077 pass(es), 0 fail(s), and 78 exception(s).
[ View ]

Let's see the tests.

Status:Needs review» Needs work

The last submitted patch, d8-machinename-1842718.patch, failed testing.

StatusFileSize
new5.66 KB
PASSED: [[SimpleTest]]: [MySQL] 49,045 pass(es).
[ View ]

Seems I cannot depend on availability of #language.

Status:Needs work» Needs review

Status:Needs review» Needs work

Not sure it's right to make triggering translitiration for filename btw.
Suppose better implement machine-name related part and proceed with filename is another issue

@alippai machinename part nearly rtbc

+++ b/core/includes/file.incundefined
@@ -1140,6 +1142,11 @@ function file_save_upload($source, $validators = array(), $destination = FALSE,
+  // Transliterate the filename according the given language parameter.
+  if($language) {
+    $file->filename = drupal_container()->get('transliteration')->transliterate($file->filename, $language, '_');

As I said above this needs to save original filename

+++ b/core/modules/file/file.moduleundefined
@@ -1153,7 +1153,14 @@ function file_managed_file_save_upload($element) {
-  if (!$file = file_save_upload($upload_name, $element['#upload_validators'], $destination)) {
+  if(isset($element['#language'])) {
+    $language =  $element['#language'] == 'und' ? 'en' : $element['#language'];
+  }
+  else {
+    $language = FALSE;

We have more laguages now.

+++ b/core/modules/system/system.routing.ymlundefined
@@ -4,3 +4,9 @@ system.cron:
+    _permission: 'access content'
\ No newline at end of file

just make add \n

I can store the orginal filename, that'd be easy. Sorry but I can't decide, I'm new to the D8 dev and the multilanguage component. I'll implement what you decide on. :)

"We have more laguages now." What do you mean here? The Transliteration class defaults to English language, so I mapped 'und' to the Transliterations default fallback 'en'. Is that wrong?

I just mean to file another issue about introducing orig_name fo file entity and use translitiration fot it.

About language - I'm not sure that $element['#language'] is right place to get language, probably $request should be better place to get the language of filename. Also take a look at original Translitiration modue implentation and #586816: How to save the original/unaltered filename?

@andypost: well, the element language if I'm not mistaken is supposed to refer to the language for the specific element and might depend on entity/field translation setup for the field. Reality is People might want to upload a photo of a person with a foreign name in an English document for example, so edge cases are not really possible to avoid. But the element language sounds like might be a better approximation vs. the request language.

Status:Needs work» Needs review
StatusFileSize
new4.89 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Removed the filefield filename transliteration, added the AJAXified machine-name.js.
I think if we want a transliterated filefield filename that should be a pluggable, optional so it's out of the scope of this patch.

StatusFileSize
new7.17 KB
PASSED: [[SimpleTest]]: [MySQL] 50,787 pass(es).
[ View ]

Ooops, missing files.

I canceled the test run for #2. Lets see how 3 does (which has the file you wanted)

+++ b/core/modules/system/lib/Drupal/system/MachineNameController.php
@@ -0,0 +1,70 @@
+  /**
+   * Get matches for the autocompletion of taxonomy terms.
+   *
+   * @param string $text
+   *   The input string which needs to be transliterated.
+   * @param string $language
+   *   The input's language.
+   * @param string $replace_pattern
+   *   The disallowed characters list, which will be replaced.
+   * @param string $replace
+   *   The disallowed characters will be replaced with this character.
+   * @param boolean $replace
+   *   Whether the result should be lowercased or not.
+   *
+   * @return string
+   *   The transliterated string.
+   */

These @params are wrong, since the only @param is the request itself. What you're actually documenting is the query parameters that will be requested, which is an entirely different thing.

At the moment we don't have a way to directly get at query parameters in the controller's method signature, sadly. Maybe someday.

Also, what is returned is not a string. It's a JsonResponse object, and should be documented as such.

+++ b/core/modules/system/system.routing.yml
@@ -4,3 +4,9 @@ system.cron:
     _access_system_cron: 'TRUE'
+machine_name:
+  pattern: '/machine_name/generate'
+  defaults:
+    _controller: 'system.machine_name_controller:generate'
+  requirements:
+    _permission: 'access content'

"machine_name" seems like an odd name for this route. Shouldn't it be machine_name_lookup, or machine_name_transliterate, or something like that?

Also, "generate" seems like the wrong name for this method, unless I'm misunderstanding what this does. It seems like it could get confused with the UrlGenerator. I think the other autocompletes have been using match() as the method name.

Aside from those nitpicks the route-system-usage here seems good. Well done, alippai!

StatusFileSize
new7.03 KB
PASSED: [[SimpleTest]]: [MySQL] 50,847 pass(es).
[ View ]

You are right, that documentation was garbage, I think this is a better one.
Also renamed the generate() to transliterate().

Can somebody give it a try with non-latin alphabet?

Status:Needs review» Needs work
StatusFileSize
new12.77 KB
new23.15 KB

@alippai Works awesome! Only a small code-style issues.

Not sure we able to add a "original filename" field for file entity - this cold be a feature but Very useful in success D8MI so probably we need a follow-up here.

New content type
content-type-translitirate.png
New vocabulary
vocabulary-translitirate.png

+++ b/core/misc/machine-name.jsundefined
@@ -42,19 +42,18 @@ Drupal.behaviors.machineName = {
+        });
+      } else {
+        self.showMachineName(expected, data);

else should be on new line

+++ b/core/misc/machine-name.jsundefined
@@ -124,6 +123,22 @@ Drupal.behaviors.machineName = {
+    }  ¶
@@ -141,8 +156,13 @@ Drupal.behaviors.machineName = {
+    });    ¶

trailing whitespace

Status:Needs work» Needs review
StatusFileSize
new7.01 KB
PASSED: [[SimpleTest]]: [MySQL] 52,242 pass(es).
[ View ]

With the fixed code style rerolled.

+1 RTBC here
Needs javascript maintainers review

+++ b/core/modules/system/system.routing.ymlundefined
@@ -4,3 +4,9 @@ system.cron:
+machine_name:

Also route name suggested by @Crell not addresed

StatusFileSize
new7.02 KB

I hope this won't broke anything. :)

...any reason for that patch having the "do-not-test" suffix?

Also, here's the follow-up that Andrey suggested back in #25: #1925684: Provide a "original filename" field for file entity.

There was no change in code since #26, only the route name changed in the #30 as it was requested in #28.

Status:Needs review» Needs work

@alippai thanks for keeping this moving forward.

We use interdiffs often, especially for reviews after small changes.

Try making an interdiff. http://drupal.org/node/1488712

Also, since the testbot is a robot :) it's ok to give it a little extra work, and make a patch for it that will be tested, especially it is one we are going to try and get committed. And this seems pretty close to addressing concerns, so lets have the testbot show it is green.

StatusFileSize
new511 bytes
new7.02 KB
PASSED: [[SimpleTest]]: [MySQL] 52,860 pass(es).
[ View ]

Created the interdiff, let's see the testbot.

Status:Needs work» Needs review

Issue tags:+#SprintWeekend

Add sprintweekend tag.

Issue tags:+Needs manual testing

looked at #33 and it is fine for me
But we need post here some screenshots cause there are a few javascript additions
+1RTBC if manual testing good

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs manual testing

woops
didn`t see #25

The patch works fine for me. Tested Greek names in the add content type form and the add view form.

Status:Reviewed & tested by the community» Needs work

The patch looks great. however, one problem:

+++ b/core/includes/form.incundefined
@@ -3765,6 +3765,9 @@ function form_validate_table($element, &$form_state) {
+  // We need to pass the langcode to the client.
+  $language = language(LANGUAGE_TYPE_INTERFACE);
@@ -3831,6 +3834,7 @@ function form_process_machine_name($element, &$form_state) {
+      'language' => $language->langcode,
+++ b/core/misc/machine-name.jsundefined
@@ -141,8 +157,13 @@ Drupal.behaviors.machineName = {
+        language: drupalSettings.language,
+++ b/core/modules/system/lib/Drupal/system/MachineNameController.phpundefined
@@ -0,0 +1,63 @@
+    $language = $request->query->get('language');
...
+    $transliterated = $this->transliteration->transliterate($text, $language, '_');

Drupal 8 uses language where a language object is involved. The only proper use of this pattern is the first segment of the quoted parts.

Everywhere else you say 'language', but you actually don't have a full language object, you have a langcode. See how obscure would that end up: $language = language_load($language); - load a language by its language?! Yes, right? So language codes are exclusively named "langcode" all around the codebase. Should be applied here too.

Should be possible to quickly get back to RTBC once this is fixed.

Status:Needs work» Needs review
StatusFileSize
new7.02 KB
FAILED: [[SimpleTest]]: [MySQL] 53,314 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.06 KB

quick re-roll

Status:Needs review» Reviewed & tested by the community

Great, thanks for the fix!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, d8-machinename-1842718-40.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.52 KB
new7.19 KB
PASSED: [[SimpleTest]]: [MySQL] 53,335 pass(es).
[ View ]

minor changes to the JS file, still working.

added Drupal and drupalSettings to the closure and got rid of the JSHint error in a condition (it was using != and should have used !==).

Title:Use new Transliteration functionality in coreUse new Transliteration functionality in core for machine names

Retitle for specific scope.

Issue tags:+sprint, +language-base

Put on D8MI sprint board for tracking.

nothing in here to block commit afaik.
--------

+++ b/core/modules/system/lib/Drupal/system/MachineNameController.phpundefined
@@ -0,0 +1,63 @@
+ * Contains Drupal\system\MachineNameController.

non-blocking nit: \Drupal
http://drupal.org/node/1354#classes
"If you use a namespace in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)."

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.phpundefined
@@ -21,6 +22,8 @@ class SystemBundle extends Bundle {
+    $container->register('system.machine_name_controller', 'Drupal\system\MachineNameController')
+      ->addArgument(new Reference('transliteration'));

is it related to this register?

+++ b/core/modules/system/system.routing.ymlundefined
@@ -4,3 +4,9 @@ system.cron:
+  defaults:
+    _controller: 'system.machine_name_controller:transliterate'

why is the pattern different for this _controller?

Others:

[~/foo/d8-git/core/modules]
550 $ grep _controller */*routing.yml
edit/edit.routing.yml:    _controller: '\Drupal\edit\EditController::metadata'
edit/edit.routing.yml:    _controller: '\Drupal\edit\EditController::fieldForm'
editor/editor.routing.yml:    _controller: '\Drupal\editor\EditorController::getUntransformedText'
rest/rest.routing.yml:    _controller: '\Drupal\rest\RequestHandler::csrfToken'
system/system.routing.yml:    _controller: '\Drupal\system\CronController::run'
user/user.routing.yml:    _controller: '\Drupal\user\UserAutocompleteController::autocompleteUser'
user/user.routing.yml:    _controller: '\Drupal\user\UserAutocompleteController::autocompleteUserAnonymous'

--
updating the issue summary with the screenshot, the follow-up and that js and that route system usage got reviewed.

StatusFileSize
new7.19 KB
PASSED: [[SimpleTest]]: [MySQL] 53,299 pass(es).
[ View ]
new2.1 KB

Re-roll for HEAD with minor code-fixes for #47

is it related to this register?
why is the pattern different for this _controller?

Because we need here service with injection of translitiration

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.php
@@ -21,6 +22,8 @@ class SystemBundle extends Bundle {
+    $container->register('system.machine_name_controller', 'Drupal\system\MachineNameController')
+      ->addArgument(new Reference('transliteration'));

The core convention is to use the create() factory method rather than registering controllers as full on services. We should probably be doing that here, too, for consistency. That still allows the transliteration service to be injected. That works for any _content, _controller, or _form class specified in the route.

Status:Reviewed & tested by the community» Needs work

Status:Needs work» Needs review
StatusFileSize
new3.66 KB
new15.76 KB
PASSED: [[SimpleTest]]: [MySQL] 53,608 pass(es).
[ View ]

Re-rolled with the changes regarding the ControllerInterface.

Status:Needs review» Needs work

+++ b/.gitignoreundefined
@@ -0,0 +1,25 @@
+# To use this file simply copy it to .gitignore, and it will cause files like
+++ b/d8-machinename-1842718-48.patchundefined
@@ -0,0 +1,209 @@
+diff --git a/core/includes/form.inc b/core/includes/form.inc
+index 27f450e..ec07d6e 100644
+--- a/core/includes/form.inc
++++ b/core/includes/form.inc
+@@ -3765,6 +3765,9 @@ function form_validate_table($element, &$form_state) {
+  *     name must not be changed after initial creation.
+  */
+ function form_process_machine_name($element, &$form_state) {
++  // We need to pass the langcode to the client.
++  $language = language(LANGUAGE_TYPE_INTERFACE);
...
+ ¶
+ "use strict";
+ ¶
+@@ -42,19 +42,19 @@ Drupal.behaviors.machineName = {
+ ¶
+     function machineNameHandler(e) {
...
+ namespace Drupal\system;
+ ¶
+ use Symfony\Component\DependencyInjection\ContainerBuilder;
++use Symfony\Component\DependencyInjection\Reference;
+ use Symfony\Component\HttpKernel\Bundle\Bundle;

looks like a patch have wrong contents

StatusFileSize
new2.53 KB
new7.05 KB
PASSED: [[SimpleTest]]: [MySQL] 53,980 pass(es).
[ View ]

oops, wrong files uploaded. (old patch and gitignore...)
Attached a version without the garbage. It works locally for me, but +1 needed again because of the change of the controller.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

This looks to me directly resolves Crell's concerns from #49, so back to RTBC.

#53: d8-machinename-1842718-53.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

We need to have some tests around the new MachineNameController especially for the optional functionality... i.e lowercase and replace_pattern.

As Gabor says "this is a cool one"... let's get this in.

@alippai: I've heard you are working on this. Any updates?

Issue tags:-sprint

I'm pretty sad there is no work going on on this. There is no point in keeping it on the sprint without it being worked on, so removing from sprint.

Status:Needs work» Needs review
Issue tags:-Needs tests, -challenging, -#SprintWeekend
StatusFileSize
new9.71 KB
PASSED: [[SimpleTest]]: [MySQL] 55,401 pass(es).
[ View ]
new4.37 KB

New patch changes:

  • Programme to an interface and not a class :)
  • Some tests for the machine name controller
  • Removes an unnecessary change so the patch applies

I tried to use PHPUnit but this proved impossible due to the drupal_strtolower in MachineNameController...

+++ b/core/modules/system/lib/Drupal/system/MachineNameController.phpundefined
@@ -0,0 +1,74 @@
+  /**
+   * Constructs a MachineNameController object.
+   *
+   * @param \Drupal\Core\Transliteration\PHPTransliteration $transliteration
+   *   The transliteration helper.
+   */
+  public function __construct(TransliterationInterface $transliteration) {
+    $this->transliteration = $transliteration;
+  }

Shouldn't we change the docs to TransliterationInterface?

Also the code changes and tests are RTBC.

StatusFileSize
new9.72 KB
PASSED: [[SimpleTest]]: [MySQL] 55,865 pass(es).
[ View ]
new646 bytes

@alippai how right you are :) thanks for the review!

Status:Needs review» Reviewed & tested by the community

RTBC, if Gabor doesn't complain. :P

https://drupal.org/node/1938670 for the problem with PHPUnit you mentioned before.

P.S.: This doesn't change the fact it's RTBC, if I'm right we do "commit early, commit often". We can convert this to use PHPUnit and String::lowerCase() later, there are issues for that.

+1 for RTBC.

Now all we need to do is wait for 200 more comments, an epic bikeshed about the naming of the interface, and at least three proposals for complete changes in direction (with like 70 layers of abstraction). =D

Status:Reviewed & tested by the community» Needs work
Issue tags:-JavaScript, -D8MI, -language-base

The last submitted patch, d8-machinename-1842718-63.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+JavaScript, +D8MI, +language-base

#63: d8-machinename-1842718-63.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

Back to RTBC I guess ;)

Issue tags:+sprint

Thanks for jumping on this, it proves it helps if we try to call the neglected state of an important issue to attention :) Putting back on sprint for tracking. Looks like @alexpott disqualified himself from committing though, hah.

Status:Reviewed & tested by the community» Fixed

All right, let's get this sucker in!

Committed and pushed to 8.x. WOOHOO!

Status:Fixed» Needs review
StatusFileSize
new596 bytes
new9.68 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-machinename-1842718-71.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updating for new standard http://drupal.org/node/1354#inheritdoc

Status:Needs review» Needs work

The last submitted patch, d8-machinename-1842718-71.patch, failed testing.

something wrong with my d8. sorry. the interdiff should be good though.

I'll go sudo rm -r my d8 and get a clean one.

Status:Needs work» Fixed

D'oh! Cross-post.

Applied the interdiff to 8.x to get that in there too. :)

Back to fixed!

Issue tags:-sprint

Thanks all folks!

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

Issue summary:View changes

added list of reviews done, added screenshot, added follow-up issue section, first draft at api changes

Issue summary:View changes
Parent issue:» #567832: Transliteration in core

...testing issue relationships ;)