Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#47 | 2575401-47.patch | 36.15 KB | Seonic |
| |||
#46 | screencast-2021.10.10-04_11_17.mp4 | 768.23 KB | Seonic |
#46 | 2575401-46.patch | 35.72 KB | Seonic |
| |||
#39 | l10_client-2575401-38.patch | 24.66 KB | Musa.thomas |
#37 | 2575401-37.patch | 24.49 KB | SebCorbin |
Issue fork l10n_client-2575401
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3.x changes, plain diff MR !5
- 2575401-localization-ui-submodule changes, plain diff MR !2
Comments
Comment #2
rvilarFirst step can be to create a meta issue (or transform this one) and try to identify every component on D7 that need to be ported to D8. Once this is done, it should be more simply to organize the port process
Comment #3
Gábor HojtsyThere is an 8 branch with a fancy new UI, you should try it out. It does not save translations and the remote connection does not work.
Comment #4
rvilarI'm working on that
Comment #5
rvilarAttached is a first approach for the functionality of saving a new translation. Now It creates the new translataion via an ajax call. Please, review it to know if I'm doing something wrong.
I will continue with trying to save in the l.d.o server (as a first approach).
Comment #7
rvilarFixed a double call in JS file
Comment #9
Gábor HojtsyComment #10
Gábor Hojtsy@rvilar: the remote submission feature is "feature complete" in terms of initial port, available in https://www.drupal.org/node/2630990. Please do not add the permission to the UI module, its managed by the contributor module already. It can/should tie into the UI module.
Comment #11
rvilarAttached is a patch with the progress on that removing the permission. I'm working on fixing JS to copy all strings transalted in joyride to the final Modal. Then the user can review it and if all is correct, send it.
Comment #12
rvilarChanging state so people can have a look on this progress.
Comment #14
Gábor HojtsyHuge thanks for working on this!
Primarily this needs to be a FormBase extension (use form tokens) so it is protected from CSRF attacks. "Just" implementing it as a controller does not suffice for security unfortunately.
Comment #15
Gábor HojtsyComment #16
Gábor HojtsyLet's focus this only on the saving part then.
Comment #17
Gábor HojtsyOpened #2730817: Localization client UI needs refactoring for modern JS as a higher level issue for refactoring.
Comment #18
rafal.cygnarowski CreditAttribution: rafal.cygnarowski commentedI have applied and checked 2575401-11-l10n_client-d8-port.patch. With this patch translation is saved properly but translation is not send to localize.drupal.org server. On the other hand, "Save and contribute" button on /admin/config/regional/translate page sends translation to server but not saves translation in site configuration (it should do this even if sending translation failed!).
Update: I was wrong. Patch worked for a while but now translation are not saved again. Have to check why...
Comment #19
rafal.cygnarowski CreditAttribution: rafal.cygnarowski as a volunteer commentedI've made some work to make UI a little bit more usable. Short list of changes:
- possibility to hide strings not found on page,
- strings searching process does not freeze browser and a progress message is displayed,
- title attributes are searched for untranslated strings,
- JoyRide will start every time after "Translate page" button click, but already translated strings are omitted,
- translated string after JoyRide step is copied to translation UI,
- message is displayed if translation was saved/unsaved (or any other condition occures),
- stem for contributing translations included...
Comment #20
rafal.cygnarowski CreditAttribution: rafal.cygnarowski as a volunteer commentedBugfix and cosmetic changes.
Comment #21
Gábor HojtsyComment #24
Gábor Hojtsy@rafal.cygnarowski does not apply now.
Comment #25
Gábor Hojtsy#2730919: Port tests to Drupal 8 landed, so it should be easier now to expand on that test coverage to make this work.
Also it would be quite important to limit this to the changes needed, not other UI changes, which seem to be included in the issue.
Comment #26
Gábor HojtsyComment #27
segi CreditAttribution: segi at Cheppers commentedI will review and reroll the last patch.
Comment #28
kndrI rerolled #11 patch but I hadn't sucess with rerolling #19 patch. I realized soon that Gabor Hojtsy was right saying that we need CSRF protection (#14) and focus on the topic (#25). According to this I've decided to create a new patch with the following changes:
- the whole form is created inside TranslationForm (instead of javascript function),
- translated strings are saved to {locales_target} in submitForm,
- Drupal Ajax API is harnessed for data exchanging between the form and the dialog window,
- arrays have short syntax because of the new code standards introduced in Drupal 8.3,
- a few minor fixes to match code standards (white spaces, comments, commas, etc.).
There are still major issues, though. Some of them was mentioned by rafal.cygnarowski (i.e. strings are not copied from JoyRide tour to the dialog window, there are no option to send strings to localization server). I'm convinced that it needs to be solved in separate issues.
Comment #29
kndrComment #31
kndrPlease, don't bother about #28 interdiff-2575401-11-28.patch. I've made mistake in the file extension (it should be .txt). Fortunately 2575401-28.patch has passed tests.
Comment #32
kndrComment #33
zaporylieComment #34
geek-merlinWhat about creating a branch or sandbox to collaborate on this?
Monster-patch-interdiffing is a PITA imho.
Comment #35
kndrThe range of necessary work may be very wide, especially if we take #2730817: Localization client UI needs refactoring for modern JS into consideration. Separate branch or sanbox would be a good choice. My contribution experience is small so I would like to count on experienced developers in such type of task. Anyway I will do my best because I find this module very usefull in work with translations.
Comment #36
Gábor HojtsySince the 8.x version of the UI does not work at all, a separate branch may not be necessary. I am happy to hand out maintainer rights to interested parties :)
Comment #37
SebCorbin CreditAttribution: SebCorbin at Makina Corpus commentedReroll #28
Comment #38
svenryen CreditAttribution: svenryen at Ramsalt Lab commentedI applied the last patch from #37 to -dev, but the module still won't save my translations to the local database. Submissions to l.d.o still works, though.
Comment #39
Musa.thomashello there seems there is problem with css when it's aggregate, i just move down
Also I add call to clear cache when translation is submiting inside TranslateEditForm.
Here the patch start from #37
Comment #40
Gábor HojtsyMake title more specific
Comment #41
Lukas von BlarerJust tested this on a project with lots of modules installed. Opening the l10n_client_ui makes Firefox and Chrome crash. The UI never becomes visible. After debugging this for a bit I think that the
Drupal.l10n_client_ui.findClosest
is causing this performance issue. It takes 1350ms to execute and loops all the sources array (in my case 578 items big).Comment #43
Lukas von BlarerI created a merge request with the existing patch and added the following changes:
Should we get rid of the joyride feature for now and handle that in a follow-up issue? I think that would make sense, since the module does what I want without it and it is currently completely unusable.
We'd need to have a look at CS. I think there is a bit of work left to clean up the code.
Comment #44
Lukas von BlarerJust a side note: I guess we could create a follow-up issue once #3103380: Allow rendering dialogs using the admin theme got committed. Using the admin theme would solve a lot of styling issues we currently have.
Comment #45
Gábor Hojtsy@Lukas: moving forward with a limited feature set is fine, it was/is not working for a while at all anyway. How does your MR work? Can you post some screenshots or a short gif/mp4?
Comment #46
Seonic CreditAttribution: Seonic commentedMerge request 2 doesn't contain a SaveTranslationCommand from the previous patches.
After applying the patch #38 I faced in a few problems to make it works.
1. Translation form uses numeric indexes from form['list'] array to get a real string data on form submit, but after ajax call the strings position is different and a wrong string saves to a database.
It was fixed by changing a numeric index by a string id.
2. To retrieve a list of translatable strings the module uses a string_translator service. But it retrieves strings in hook_prepocess_page after call of $interface_recorder->getRecordedData(), at this moment not all strings passed through this service, and it loses some strings from entities which is rendered later.
To fix it, translation form moved to a new custom block, this block uses lazy_builder to render content, and it is added in hook_preprocess_html to a page_bottom region. This allows to collect all strings from a page and render it in the form.
3. Render cache ruins everything, the string_translator service can't collect strings gotten from a render cache, because it's html.
It's a main problem, I could fix it only by override a ConfigFactory service, new service is a clone of original ConfigFactory and only checks if user has access to localization client ui and replaces a render cache by cache.backend.null if user has access. But always completely disabled render cache for users with a permission to use the module isn't good, I have an idea to add an option "translation mode", it will toggle in the toolbar. In this mode render cache will be suppressed and allows to use the module with all strings on a page. I'm not sure if it's a good approach, what you think?
Comment #47
Seonic CreditAttribution: Seonic commentedRegarding the third point, drupal ajax doesn't use the render cache, and it allows us to return an actual list of strings by simply trigger the ajax on the form upon opening.
Comment #48
claudiu.cristeaYes, please!
Comment #49
Gábor Hojtsy@claudiu.cristea: did you try this patch? Does it work for what it promises? I'm especially interested in strings from potentially cached parts of the page, which is more tricky in modern Drupal :)
Comment #50
claudiu.cristea@Gábor Hojtsy, I've tested briefly (locally) the patch from #47 and seemed to work. I admit, I didn't read the comments regarding the render cache. So, reverting to Needs review. Unfortunately, I don't have time to do a full test & review but I've deployed the module & #47 to a production and hope will get some feedback from translators. They will notify my if there are strings in the page not exposed for translation. In that case I can look in deep.
Comment #54
sanduhrs* Removed deprecated libraries joyride, jquery.cookie and jquery.once
* Removed the complete joyride experience ftb
* Added a clearfix in the translation form to adjust for Olivero
* Switched to a keyup event in the search form to trigger filter
* Retitled the Translation form from 'Translate interface' to 'Translation interface'
* Resized modal to 66% instead of the very limiting 50%
The ajax call workaround to render cache appears to be working.
Lots of strings are found, translated and untranslated.
Translating works like a charm.
Great work :)
Comment #56
sanduhrs