Problem/Motivation
When user is on User Interface translation page then user is not able to see Delete string.
Original report
Hello, In D7 we had the option to edit and delete text strings in theme_locale_languages_overview_form
Now, in D8 we have the option to edit in-line, but where is the option to delete? (in TranslateEditForm)
Any alternative to deleting strings?
Thanks
Steps to reproduce
- Goto: Configuration -> User Interface Tranlsation
- Delete string is not appearing at right side.
Proposed resolution
Introduce Delete checkbox to delete the translation.
Once the checkbox is checked and the form is saved then it will delete the string.
Remaining tasks
Needs a Usability review.
Patch review
Commit
User interface changes
New checkbox will be added on string translation form.
After
Screenshot after patch in #25 (screenshot from #28)
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#59 | 2503893-59-fix-string-regression.patch | 6.12 KB | ptmkenny |
#57 | 2503893-54.patch | 6.09 KB | AshM |
#53 | 2503893-53.patch | 6.09 KB | k-l |
| |||
#48 | interdiff-2503893-45-48.txt | 1.21 KB | yogeshmpawar |
#48 | 2503893-48.patch | 4.88 KB | yogeshmpawar |
Issue fork drupal-2503893
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:
Comments
Comment #1
Gábor HojtsyYeah I think that's an oversight. Not sure if it would be a good idea to integrate in this form but also not sure how to do it otherwise :/
Comment #2
facine CreditAttribution: facine commentedWhile there is no solution, if someone needs to clear a string ...
Comment #3
Gábor HojtsyThere are also other tables with location info for example to clear out. Also why did you specifically need to clear out strings?
Comment #4
legolasboAttached patch adds a checkbox to each row which allows the strings to be deleted and adds a
deleteTranslationSource
method to theLanguageManagerInterface
andLanguageManager
class.It should probably be refactored to include a confirmation page to prevent accidental data destruction.
Comment #5
Gábor HojtsyIf we are to do this, we need screenshots, a confirmation form and tests. Also the UI needs to be made clear that not only the translation being seen is removed but all translations and the source string included.
Honestly I am not sure that the added UI shown on all string edit screens will be that valuable. You would barely use this UI to delete things. In fact why do you think its important to delete single strings? Drupal 8 core itself has over 8 thousand strings. Removing one or two would not make your site more performant or anything like that.
Comment #6
Gábor HojtsyAlso, if a string is actually used somewhere it will show up as a source again (and if it was part of the project originally, it will get its translation update from localize.drupal.org as well), so practically restore itself soon always at least as a source and as translations too unless you actively disabled translation updates.
While I see this is a feature lost, it would be great to understand what do you want to use this feature for.
Comment #7
facine CreditAttribution: facine commented@Gábor Hojtsy a module created me a string in the wrong language.
Comment #8
Gábor Hojtsy@facine: so basically fixing bogus behavior in a module... we should discuss whether the constant UI elements added are worth for that edge case :)
Comment #9
penyaskitoI would be very tempted to Won't fix this one and let this feature emerge in contrib. What do you think, Gábor?
Comment #10
legolasboI've been thinking about this some more and I think both @Gábor and @penyaskito have a valid point.
Adding these UI elements is not an improvement visually, given the clean layout of the translation form and this functionality would only serve it's purpose in edge cases.
It is also really easy to implement this functionality in a contrib module. Therefor I would vote in favour of Won't fixing this issue.
Comment #11
legolasboAlso, I don't think this issue should be major.
Comment #14
jcisio CreditAttribution: jcisio at Axess Open Web Services for ARTE G.E.I.E. commentedRe #6 actually I think this feature is useful to delete a string generated by an old version of a module (mostly custom module, because a contrib module has all strings of all released versions on l.d.o). While it won't help much in performance, it makes it possible to have 100% strings translated (without adding bogus translation), and search won't return a string which is never used.
Comment #15
Gábor Hojtsyl.d.o serves version specific translations the same way for contrib and core, there is no difference, so outdated core strings may have the same problem as outdated contrib strings.
Comment #18
ptmkenny CreditAttribution: ptmkenny commentedI agree with @jcisio, having the ability to delete strings is a useful feature when working with a custom module. If you update the code of the module and change some strings, it's nice to be able to delete the originals to get the 100% translation.
Comment #20
MichelleAdding another reason. I just ran into this because I discovered the translation was done wrong on a template and ended up adding a new source string for every term that has been searched on. I didn't see any way in the UI to clean it up so I went through and deleted all the junk from the PO file and imported only to find that doesn't work, either. Would be nice to have a way to bulk delete them all.
Comment #21
andypostHaving clean-up of outdated translation is separate issue
Having ability to delete could be useful for customized strings
Comment #23
handkerchiefAny news on this?
Comment #24
andypostComment #25
jcisio CreditAttribution: jcisio at Axess Open Web Services for ARTE G.E.I.E. commentedComment #26
handkerchief@thx jcisio
I tested it with 8.7.7 and it works like a charm.
Comment #27
jcisio CreditAttribution: jcisio at Axess Open Web Services commentedThanks handkerchief, it was a reroll of a 4 year old patch. Next time anyone tests could you post screenshots of the new feature please? This patch needs also tests in code.
Comment #28
handkerchiefHere's a screenshot. But I'm the wrong guy for tests (code).
/admin/config/regional/translate
Comment #30
maxplus CreditAttribution: maxplus commentedThanks, patch #25 is working great with 8.9.1.
It would be nice to have a "select all" checkbox like we have in a view using VBO, but for now it is very handy!
Comment #31
pameeela CreditAttribution: pameeela commentedAdded screenshot from #28 to IS so un-tagging.
Comment #32
Vivek Panicker CreditAttribution: Vivek Panicker commentedSince we are doing this, should we not replace
db_delete
indeleteTranslationSource()
with call to Drupal database service as db_delete() is removed in Drupal 9?Comment #33
andypostNo reason to introduce a method in language manager as there's
\Drupal\locale\StringDatabaseStorage::deleteStrings()
Also making delete as bulk operation
Comment #34
andypostA bit of clean-up, still NW for tests
Comment #35
pameeela CreditAttribution: pameeela commentedComment #36
colorfieldAdds support for multiple delete. As
deleteStrings()
inStringDatabaseStorage
can get multiple values returned bydbStringSelect()
,dbDelete()
needs to check its condition with the IN operator.Still NW for tests.
Comment #38
markus_petrux CreditAttribution: markus_petrux commentedPatch in #36 applied well on Drupal 8.9.7
Comment #39
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedAdding test cases to validate the following things:
- Presence of delete checkbox
- Deletion process.
Comment #41
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Zyxware Technologies commentedVerified and tested patch#39 on the drupal 9.3.x-dev version. Patch applied successfully and looks good to me.thanks @mohit_aghera.Adding screenshot for the reference
Comment #42
quietone CreditAttribution: quietone as a volunteer commented@Rinku Jacob 13, thanks for the screenshots. They should be put into the Issue Summary. It helps anyone working on an issue if the Summary is always up to date.
Tagging for an issue summary update for having up to date screenshots in the IS and hopefully someone will add the template. Also tagging novice because the updates to the issue summary for this issue are straightforward.
Comment #43
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedComment #44
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedHi, @quietone,
Added the Issue summary.
Hi @Rinku Jacob 13
I have applied the #39 patch on the 9.3.x-dev version.
The patch is not applied successfully.
Steps to reproduce:
# Goto: Configuration -> User Interface Translation
# Delete string is not appearing on the right side.
Please refer attached screenshots for the same.
Needs to be re-roll to the patch.
Comment #45
mohit_aghera CreditAttribution: mohit_aghera at QED42 commentedRe-roll for 9.3.x
Comment #46
andypostComment #47
andypostI think it needs better Label - kinda "Delete translation"
Also the "Delete string" is disappointing as it will delete translation instead of source string
that's changing behavior for key fields so will affect performance
See #2031261: Make SQLite faster by combining multiple inserts and updates in a single query
Comment #48
yogeshmpawarAddressed comments mentioned in #47 & added interdiff for reference.
Comment #49
quietone CreditAttribution: quietone as a volunteer commentedI tested this on Drupal 9.3.x, standard install with a second language.
I did not expect to see 'save translation' as the button text when I am trying to delete something. There was no confirm screen for the deletion and after the delete operation the message is 'The strings have been saved'. I expect a button that is clearly a 'delete' and a confirm screen and then a confirmation telling me what I just deleted.
#47.1 points out that this deletes the string and the translation, it turns out that is how it behaves in Drupal 7. At least by my testing.
This needs more work on the user experience, tagging for a usability review to get direction.
I moved things around in the IS a bit for readability.
Comment #51
D34dMan CreditAttribution: D34dMan as a volunteer and at Factorial GmbH commentedTested #48 on 9.3.16
Deleting a single item works as
expectedas experienced in #49. However when trying to delete multiple strings by selecting multiple checkbxoes, it throws following error,Comment #53
k-l CreditAttribution: k-l commentedTested #48 on 9.4.9
Same experience as in #49 and same error as in #51.
Applied some changes to #48 patch:
- introduced separate `Delete translations` button that would handle just deletion. It appears when `delete` checkbox is checked. For now I didn't need confirmation step.
- fixed database error when deleting multiple strings.
Note: not sure if `testLocaleStringDelete()` test would pass.
Comment #54
a.sotirov CreditAttribution: a.sotirov at FFW commentedTested #53 with
php 8.1
and Drupal Core9.5.4
. Works perfectly both with single or multiple selections for delete.RTBTC +1
Comment #56
DuneBLTested #53 with 9.5.9 and it works nicely
Comment #57
AshM CreditAttribution: AshM commentedSame patch as #53 with a typo fixed.
Comment #59
ptmkenny CreditAttribution: ptmkenny commentedI re-rolled #57 and created an MR for work going forward. The patch is the current state of the MR.
Comment #60
Bohus UlrychFYI Patch #57 works with the latest Drupal 10.1.6
But #59 can't be applied, maybe works only with the latest core dev?
Drush error: Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2023-12-02/2503893-59-fix-string-reg...
Comment #61
ptmkenny CreditAttribution: ptmkenny commentedPatch #59 is for the 11.x-dev branch, because that's the current commit target. It should also work with Drupal 10.2.x, which will be out in a few days.