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
API changes
new \Drupal\system\MachineNameController class
UI changes
screenshot from #25
New content type
New vocabulary
Follow-up Issues
Comment | File | Size | Author |
---|---|---|---|
#72 | d8-machinename-1842718-71.patch | 9.68 KB | YesCT |
#72 | interdiff-63-71.txt | 596 bytes | YesCT |
#63 | 60-63-interdiff.txt | 646 bytes | alexpott |
#63 | d8-machinename-1842718-63.patch | 9.72 KB | alexpott |
#60 | 53-60-interdiff.txt | 4.37 KB | alexpott |
Comments
Comment #1
Gábor HojtsyI think the posted two ideas (uploaded files, machine names) sound like a good start :)
Comment #2
klonosI'm afraid that if we don't act on this soon, we'll have no actual transliteration in core :/
Comment #3
lpalgarvio CreditAttribution: lpalgarvio commentedyet i already see a smaller victory in having the module in core anyways. lets bring up morale ;)
Comment #4
Gábor Hojtsy@klonos: want to get started with a simple use case porting code from the contrib module to D8?
Comment #5
andypostTo apply transliteration on file names require to add a
{file_managed}.original_filename
column like File (field) paths doesComment #6
alippai CreditAttribution: alippai commentedI'm now working on the machine-name.js rewrite.
Comment #7
jhodgdonNew issue component! :)
Comment #8
jhodgdonRegarding uploaded files, I found some very old related (duplicate?) issues:
#791780: Special chars should be removed/replaced from filenames on upload.
#837322: htmlspecialchars - Invalid multibyte sequence in argument
#513198: Files with a + in their name cannot be download via private method.
Comment #9
YesCT CreditAttribution: YesCT commentedtagging
Comment #10
alippai CreditAttribution: alippai commentedAn 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.
Comment #11
alippai CreditAttribution: alippai commentedLet's see the tests.
Comment #13
alippai CreditAttribution: alippai commentedSeems I cannot depend on availability of
#language
.Comment #14
alippai CreditAttribution: alippai commentedComment #15
andypostNot 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
As I said above this needs to save original filename
We have more laguages now.
just make add \n
Comment #16
alippai CreditAttribution: alippai commentedI 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?
Comment #17
andypostI 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?Comment #18
Gábor Hojtsy@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.
Comment #19
alippai CreditAttribution: alippai commentedRemoved 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.
Comment #20
alippai CreditAttribution: alippai commentedOoops, missing files.
Comment #21
YesCT CreditAttribution: YesCT commentedI canceled the test run for #2. Lets see how 3 does (which has the file you wanted)
Comment #22
Crell CreditAttribution: Crell commentedThese @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.
"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!
Comment #23
alippai CreditAttribution: alippai commentedYou are right, that documentation was garbage, I think this is a better one.
Also renamed the generate() to transliterate().
Comment #24
alippai CreditAttribution: alippai commentedCan somebody give it a try with non-latin alphabet?
Comment #25
andypost@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
New vocabulary
else should be on new line
trailing whitespace
Comment #26
alippai CreditAttribution: alippai commentedWith the fixed code style rerolled.
Comment #27
andypost+1 RTBC here
Needs javascript maintainers review
Comment #28
andypostAlso route name suggested by @Crell not addresed
Comment #29
alippai CreditAttribution: alippai commentedI hope this won't broke anything. :)
Comment #30
klonos...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.
Comment #31
alippai CreditAttribution: alippai commentedThere was no change in code since #26, only the route name changed in the #30 as it was requested in #28.
Comment #32
YesCT CreditAttribution: YesCT commented@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.
Comment #33
alippai CreditAttribution: alippai commentedCreated the interdiff, let's see the testbot.
Comment #34
alippai CreditAttribution: alippai commentedComment #35
Gábor HojtsyAdd sprintweekend tag.
Comment #36
podaroklooked 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
Comment #37
podarokwoops
didn`t see #25
Comment #38
klonosThe patch works fine for me. Tested Greek names in the add content type form and the add view form.
Comment #39
Gábor HojtsyThe patch looks great. however, one problem:
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.
Comment #40
andypostquick re-roll
Comment #41
Gábor HojtsyGreat, thanks for the fix!
Comment #43
nod_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!==
).Comment #44
Gábor HojtsyRetitle for specific scope.
Comment #45
Gábor HojtsyPut on D8MI sprint board for tracking.
Comment #46
Gábor Hojtsy#43: core-js-transliteration-1842718-42.patch queued for re-testing.
Comment #47
YesCT CreditAttribution: YesCT commentednothing in here to block commit afaik.
--------
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)."
is it related to this register?
why is the pattern different for this _controller?
Others:
--
updating the issue summary with the screenshot, the follow-up and that js and that route system usage got reviewed.
Comment #48
andypostRe-roll for HEAD with minor code-fixes for #47
Because we need here service with injection of translitiration
Comment #49
Crell CreditAttribution: Crell commentedThe 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.
Comment #50
Gábor HojtsyComment #51
alippai CreditAttribution: alippai commentedRe-rolled with the changes regarding the ControllerInterface.
Comment #52
andypostlooks like a patch have wrong contents
Comment #53
alippai CreditAttribution: alippai commentedoops, 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.
Comment #54
alippai CreditAttribution: alippai commentedComment #55
Gábor HojtsyThis looks to me directly resolves Crell's concerns from #49, so back to RTBC.
Comment #56
Gábor Hojtsy#53: d8-machinename-1842718-53.patch queued for re-testing.
Comment #57
alexpottWe 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.
Comment #58
Gábor Hojtsy@alippai: I've heard you are working on this. Any updates?
Comment #59
Gábor HojtsyI'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.
Comment #60
alexpottNew patch changes:
I tried to use PHPUnit but this proved impossible due to the drupal_strtolower in MachineNameController...
Comment #61
alippai CreditAttribution: alippai commentedShouldn't we change the docs to TransliterationInterface?
Comment #62
alippai CreditAttribution: alippai commentedAlso the code changes and tests are RTBC.
Comment #63
alexpott@alippai how right you are :) thanks for the review!
Comment #64
alippai CreditAttribution: alippai commentedRTBC, if Gabor doesn't complain. :P
Comment #65
alippai CreditAttribution: alippai commentedhttps://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.
Comment #66
cweagans+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
Comment #68
alexpott#63: d8-machinename-1842718-63.patch queued for re-testing.
Comment #69
aspilicious CreditAttribution: aspilicious commentedBack to RTBC I guess ;)
Comment #70
Gábor HojtsyThanks 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.
Comment #71
webchickAll right, let's get this sucker in!
Committed and pushed to 8.x. WOOHOO!
Comment #72
YesCT CreditAttribution: YesCT commentedUpdating for new standard http://drupal.org/node/1354#inheritdoc
Comment #74
YesCT CreditAttribution: YesCT commentedsomething wrong with my d8. sorry. the interdiff should be good though.
I'll go sudo rm -r my d8 and get a clean one.
Comment #75
webchickD'oh! Cross-post.
Applied the interdiff to 8.x to get that in there too. :)
Back to fixed!
Comment #76
Gábor HojtsyThanks all folks!
Comment #77.0
(not verified) CreditAttribution: commentedadded list of reviews done, added screenshot, added follow-up issue section, first draft at api changes
Comment #78
klonos...testing issue relationships ;)
Comment #79
sashken2 CreditAttribution: sashken2 commentedHello!
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?
Comment #80
jhodgdonThis 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!