Regression: not possible to delete source strings in Drupal (>7) interface translation

Yeah 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 :/

There are also other tables with location info for example to clear out. Also why did you specifically need to clear out strings?

Attached patch adds a checkbox to each row which allows the strings to be deleted and adds a deleteTranslationSource method to the LanguageManagerInterface and LanguageManager class.

It should probably be refactored to include a confirmation page to prevent accidental data destruction.

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

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

@facine: so basically fixing bogus behavior in a module... we should discuss whether the constant UI elements added are worth for that edge case :)

I would be very tempted to Won't fix this one and let this feature emerge in contrib. What do you think, Gábor?

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

Also, I don't think this issue should be major.

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

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

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

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

Having clean-up of outdated translation is separate issue

Having ability to delete could be useful for customized strings

Any news on this?

@thx jcisio

I tested it with 8.7.7 and it works like a charm.

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

Here's a screenshot. But I'm the wrong guy for tests (code).

/admin/config/regional/translate

Thanks, 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!

Added screenshot from #28 to IS so un-tagging.

Since we are doing this, should we not replace db_delete in deleteTranslationSource() with call to Drupal database service as db_delete() is removed in Drupal 9?

No reason to introduce a method in language manager as there's \Drupal\locale\StringDatabaseStorage::deleteStrings()

Also making delete as bulk operation

A bit of clean-up, still NW for tests

Adds support for multiple delete. As deleteStrings() in StringDatabaseStorage can get multiple values returned by dbStringSelect(), dbDelete() needs to check its condition with the IN operator.
Still NW for tests.

Patch in #36 applied well on Drupal 8.9.7

Adding test cases to validate the following things:
- Presence of delete checkbox
- Deletion process.

Verified 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

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

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

Re-roll for 9.3.x

1. +++ b/core/modules/locale/src/Form/TranslateEditForm.php
   @@ -47,6 +47,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $this->t('Delete'),
   
   @@ -138,6 +139,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $form['strings'][$string->lid]['delete'] = [
   +          '#type' => 'checkbox',
   +          '#title' => $this->t('Delete string'),
   +          '#title_display' => 'invisible',
   

   I think it needs better Label - kinda "Delete translation"

   Also the "Delete string" is disappointing as it will delete translation instead of source string

2. +++ b/core/modules/locale/src/StringDatabaseStorage.php
   @@ -526,7 +526,7 @@ protected function dbStringUpdate($string) {
   -      $query->condition($field, $value);
   +      $query->condition($field, (array) $value, 'IN');
   

   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

Addressed comments mentioned in #47 & added interdiff for reference.

I tested this on Drupal 9.3.x, standard install with a second language.

1. Navigate to admin/config/regional/translate
2. See the delete box on the right hand side
3. Tick the box for 'account settings'
4. Scroll to bottom
5. The button text is 'Save translation'
6. Click 'Save translation'
7. Message "The strings have been saved."
8. The source string and the translation 'account settings' are removed.

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.

Tested #48 on 9.3.16

Deleting a single item works as expected as experienced in #49. However when trying to delete multiple strings by selecting multiple checkbxoes, it throws following error,

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not
                                                match number of tokens: DELETE FROM "locales_target"
                                                WHERE "li

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

Tested #53 with php 8.1 and Drupal Core 9.5.4. Works perfectly both with single or multiple selections for delete.
RTBTC +1

Tested #53 with 9.5.9 and it works nicely

Same patch as #53 with a typo fixed.

I re-rolled #57 and created an MR for work going forward. The patch is the current state of the MR.

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

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

Perhaps we should also test multiple deletion?

Drupal Usability Meeting #3444954: Drupal Usability Meeting 2024-05-10 reviewed #2503893: Regression: not possible to delete source strings in Drupal (>7) interface translation

Overview of UI complexity with this patch:

• Checkboxes use javascript to hide/show the 'delete translation' button once any have been clicked
• A User has the ability to both update some items, while selecting other to delete.
• Separate button to delete does remove item from DB. Should a user have other changes on same form those changes are then lost -- this work would have to be redone by user.
• Save does both tasks, if any delete is checked. should this only save items? Unclear to Reviewers until tested through, felt awkward.
• Test of 'Home' string (breadcrumb parent string) item is re-created after a page load, this is confusing still.

As Drupal will automatically re-add the item when called (by code), removing the module/theme/twig (uninstall provider) would be the way to remove an item permanently.

Thoughts on Delete*:

• *Drupal's PO import (admin/config/regional/translate/import) is this the concern of added items?
• As t() is the source, as soon as that string is called for Drupal will re-create the DB item. user has no information around this.
• if user A deletes something, User B may need to undersand how to 'get it back'.
• 'translate' UI does not show t() context information.
• lacking provider information (same as lacking context of string) (was this from PO import form?)

Overall Reviewers proactive suggestions:
a. align 'delete' with checkbox column.
b. re-label as 'clear' checkbox Clarification of how an empty item works. Then no 'delete' button is needed, is user submits then backup clears item.
This is similar to input search clear (native html)]. other examples in core of 'clear' JS in frontend to 'empty item' then user can save form normally.
- VBO pattern currently used in Drupal content list


/admin/content using bulk operations footer to clear/delete

Comments around current issue title 'regression':
The form now allows users to 'clear' and item by emptying the textarea, then saving. This overall UI change into Drupal 8+ certainly could be allowed from D7 lifespan. Like other aspects of the Translation system have progressed as well.
->Regression implies we return both the 'edit' and 'delete'. Return to only displaying the label on this UI with an Operations column.

Generally, Reviewers brought up the suggestion that the addition of helpful text to outline how an ‘empty’ textarea/item would not translate the string below the Filter details item.

some dev's comments:
- Other comments from devs center around removing items from 'dev testing' senarios (module version changes, string not accounted for??). Can a PO import anything? Off hand, I think they need code lines to import to?) else it would not import a string not used?
^ do we need an purge orphans action?

- any effects on caches states does deleting 'one item at a time' have? any similar circumstance from D7 (possibly why this was 'removed')?

For the record, the attendees at the usability meeting (Comment #64) were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell, @skaught, and @worldlinemine.

We all agreed that the two submit buttons ("Save translations" and "Delete translations") were confusing. Given that, I do not think we should implement a confirmation form.

Deleting a translatable string is temporary. As soon as someone visits a page where the string is used, the string is re-added to the list of strings available for translation. This whole approach seems confusing from a usability perspective.

Opinions at the usability meeting were divided. Some of us were in favor of each of these proposals:

1. Close this issue as "won't fix".
2. Add a checkbox as in the current MR, but not a second submit button. When the form is submitted, the checkbox has the same effect as deleting the text in the "Translation for ..." column.
3. Add help text to clarify that deleting the translation means the source string will be used.

I have used this patch with great success in several projects. It is great to delete some translations that were added by mistake due to inexperienced developers passing dynamic strings to $this->t().

Now, this is really helpful in these cases, but in _normal_ usage this issue shouldn't really arise. When using good quality stable modules, and custom code that has been peer-reviewed, in theory no "bad" source strings should be present. So I think this is solving an edge case and should not be included in core. It would be great to have this in a small contrib module though.

I am in favor of closing this as "Won't fix" and move it to contrib.

Even if I might agree this could happen in contrib first, strings are added/removed in different module versions without only being a result of bad practice per se.

Sooner than later, with the different page builders being built like XB, we will have SDC components strings here too, and SDC components will evolve too.
Also recipes will have translations (hopefully) soon, which might be affected by the same.
So it's not a priority for me, but Close (Won't fix) is probably excessive :-)

I rerolled the #39's patch to apply against 10.5.1 including the last commit from #63