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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI

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

klonos’s picture

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

lpalgarvio’s picture

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

Gábor Hojtsy’s picture

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

andypost’s picture

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

alippai’s picture

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

jhodgdon’s picture

Component: base system » transliteration system

New issue component! :)

jhodgdon’s picture

YesCT’s picture

Issue tags: +challenging, +needs initial patch

tagging

alippai’s picture

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.

alippai’s picture

Status: Active » Needs review
FileSize
5.58 KB

Let's see the tests.

Status: Needs review » Needs work

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

alippai’s picture

Seems I cannot depend on availability of #language.

alippai’s picture

Status: Needs work » Needs review
andypost’s picture

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

alippai’s picture

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?

andypost’s picture

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?

Gábor Hojtsy’s picture

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

alippai’s picture

Status: Needs work » Needs review
FileSize
4.89 KB

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.

alippai’s picture

Ooops, missing files.

YesCT’s picture

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

Crell’s picture

+++ 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!

alippai’s picture

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

alippai’s picture

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

andypost’s picture

Status: Needs review » Needs work
FileSize
12.77 KB
23.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

alippai’s picture

Status: Needs work » Needs review
FileSize
7.01 KB

With the fixed code style rerolled.

andypost’s picture

Issue tags: -needs initial patch +JavaScript

+1 RTBC here
Needs javascript maintainers review

andypost’s picture

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

Also route name suggested by @Crell not addresed

alippai’s picture

I hope this won't broke anything. :)

klonos’s picture

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

alippai’s picture

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

YesCT’s picture

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.

alippai’s picture

Created the interdiff, let's see the testbot.

alippai’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Issue tags: +SprintWeekend2013

Add sprintweekend tag.

podarok’s picture

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

podarok’s picture

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

woops
didn`t see #25

klonos’s picture

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

Gábor Hojtsy’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
7.02 KB
2.06 KB

quick re-roll

Gábor Hojtsy’s picture

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.

nod_’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.52 KB
7.19 KB

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 !==).

Gábor Hojtsy’s picture

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

Retitle for specific scope.

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-base

Put on D8MI sprint board for tracking.

Gábor Hojtsy’s picture

YesCT’s picture

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.

andypost’s picture

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

Crell’s picture

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

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
alippai’s picture

Status: Needs work » Needs review
FileSize
3.66 KB
15.76 KB

Re-rolled with the changes regarding the ControllerInterface.

andypost’s picture

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

alippai’s picture

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.

alippai’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

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

Gábor Hojtsy’s picture

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

alexpott’s picture

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -challenging, -SprintWeekend2013
FileSize
9.71 KB
4.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...

alippai’s picture

+++ 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?

alippai’s picture

Also the code changes and tests are RTBC.

alexpott’s picture

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

alippai’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, if Gabor doesn't complain. :P

alippai’s picture

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.

cweagans’s picture

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

alexpott’s picture

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

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

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC I guess ;)

Gábor Hojtsy’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

All right, let's get this sucker in!

Committed and pushed to 8.x. WOOHOO!

YesCT’s picture

Status: Fixed » Needs review
FileSize
596 bytes
9.68 KB

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.

YesCT’s picture

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.

webchick’s picture

Status: Needs work » Fixed

D'oh! Cross-post.

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

Back to fixed!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks all folks!

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

Anonymous’s picture

Issue summary: View changes

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

klonos’s picture

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

...testing issue relationships ;)

sashken2’s picture

Hello!

I'm setup clear Drupal 8 (last beta).
Why files uploaded using Image field and CKEditor Image Button not transliterated? And how make trasliterate it?

jhodgdon’s picture

This issue is now closed. If you think there is a bug in the Image field or Image button, please file a ***new*** issue and put it into one of those components to request that they use transliteration.

Otherwise... It looks like you may need some Drupal support? If so, we don't really handle support requests in the Drupal Core issue queue as a regular practice (that option is mostly there for filing support issues for contributed modules and themes).

There are several support options listed if you click on "Support" at the top of Drupal.org, which will take you to:
http://drupal.org/support

There you can find out about the Drupal IRC channels, and the Forums, which are our two main support mechanisms in the Drupal community. You might also try http://drupal.stackexchange.com/

Good luck with your issue!