SID 2776211 contains div element

Created on 2 September 2023, about 1 year ago
Updated 20 January 2024, 10 months ago

Problem/Motivation

The original String oof SID 2776211 contains a html div tag with css class attributes.
We do not allow

Steps to reproduce

  1. Install Drupal 8, 9, or 10
  2. Install the module
  3. Set up the Module
  4. Enable a second ui language where translations for this module are available in (e.g. German)
  5. Create a po file that contains a translation suggestion for that string (see attatchment)
  6. Import that sting amongst all other stings.
  7. See error message in wathcdog porotcol

Result:

Proposed resolution

update the code and merge it into the next release, so UI-Translations at least fixed for end users, althogh that will leave the string ass an untranslateable artifact on the server.

Remaining tasks

review
update code
merge
publish new release

User interface changes

String 2776211 will be translateable in the feature.

API changes

Data model changes

🐛 Bug report
Status

Fixed

Version

3.0

Component

Code

Created by

🇩🇪Germany joachim namyslo Kulmbach 🇩🇪 🇪🇺

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @joachim namyslo
  • 🇮🇪Ireland lostcarpark

    Interesting. I didn't realise that functionality existed.

    On my dev site, the header and link are not displaying, so I think it is a bit broken. It's possible because the header has a format of "rich_html", which doesn't exist on a default D10 install.

    I think putting the link in the header of the view is at best a bit hacky, and the sort of thing you can get away with in a view on a website, but not one that's part of a module.

    I would suggest moving the link out of the view and into the render array returned by the ScheduledPublishListing controller. Obviously this would include moving the <div> tag outside the translation string.

    Is that what you have in mind?

  • 🇩🇪Germany joachim namyslo Kulmbach 🇩🇪 🇪🇺

    Yes, because otherwise the string will remains Englisch in over 100 langusges

  • 🇮🇪Ireland lostcarpark

    MR !16 adds the link into the controller's render array. This means only the link text will be in the translation string.

    I haven't taken it out of the view yet, because it's likely that a new view export will be generated for 📌 Provided view config does not work as expected - Edit and Delete links broken. Needs review .

    I would like to add a test to verify the new link.

  • Status changed to Needs work about 1 year ago
  • 🇮🇪Ireland lostcarpark

    To do: Add test case for checking link present.

  • 🇮🇪Ireland lostcarpark

    I was holding off on changing the view definition as I thought there might be a new export of the view. There might still be, but it could be a while off, and if I can achieve all the needed fixes by just changing the affected elements, that seems preferable.

    I've changed the view definition YML to remove the header, but that only affects new installations, so I've also added an update script that loads the view, replaces the header with an empty array, and saves it.

    I've added a test to verify the bulk update link is present on the view page. The test doesn't care where the link is coming from, as long as it's present.

    I've also added a test of the update script. It verifies that the link test is present in the view definition before the update, and that it's removed after.

    The tests currently only cover the functionality of the bulk update link. I'd like to add more tests of the view functionality, but I feel that's outside the scope of this issue. There's an argument for adding a translation test, but I don't think that's strictly necessary.

    This change fixes the reported issue by moving the bulk update link out of the view, and directly into the page controller. As a result, only the link label text needs to be translated, rather than the whole HTML block. Aside from resolving the issue with importing the HTML elements, it removes the risk of a translator accidentally introducing errors in the HTML when translating the text, and removes for the string to be retranslated if changes are made to the HTML in future.

  • Status changed to Needs review about 1 year ago
  • 🇮🇪Ireland lostcarpark

    CI tests are currently failing for D9 because the test loads a saved database and runs the update script on it, and it fails because it's loading a D10 database against a D9 codebase. I'm reluctant to make too much accommodation for D9 as it's going end of life in a couple of months. Seeing if I can tell CI to skip the test for D9.

  • 🇮🇪Ireland lostcarpark

    Added an exclusion for the update test to the CI for D9 only. Tests are now passing.

  • 🇮🇪Ireland lostcarpark

    I have carried out a test of importing the German translations for the updated code. I updated the .PO file attached to the issue to remove the HTML from the affected translation string, and verified that the link is correctly translated after the import.

    I'm attaching the updated .PO file.

  • 🇺🇸United States cindy_codediaries

    I attempted to translate node of article content type from English to German with moderation state of draft to publish and I received the following warning: Deprecated function: strip_tags(): Passing null to parameter #1 ($string) of type string is deprecated in Drupal\comment\CommentForm->buildEntity() (line 322 of core/modules/comment/src/CommentForm.php).

  • 🇮🇪Ireland lostcarpark

    Hi Cindy,
    Could you outline the steps you followed, please?
    I'm curious what triggered an error in the comment form. Were you posting a comment?
    Thanks for your help.

  • 🇺🇸United States cindy_codediaries

    What triggered these steps was the comment field was empty when I attempted to translate node of article content type. The error message disappeared when I added a comment and published the article.

  • 🇮🇪Ireland lostcarpark

    Thanks for that information.

    That may be a separate bug, but this issue is not related to translating the article.

    The issue here has to do with translation of the user interface, and in particular importing the translation strings.

    To test, you need to do the following:

    1. Make sure the "Interface Translation" module is enabled.
    2. Enable a second language (for the sample files attached, use German).
    3. Navigate to Config->Region and Language->User Interface Translation (/admin/config/regional/translate).
    4. Select the Import tab.
    5. Attempt to import the PO file attached to the original issue, and verify it fails.
    6. Import the PO file attached to #11 and verify it loads successfully.
    7. Navigate to Content, then select the Scheduled Publish tab (/admin/content/scheduled-publish).
    8. Note the position of the "Add bulk scheduled publishing entries" button. (Note: you must have scheduled at least one item for this to be displayed.)
    9. Switch to language to German (you can do this by inserting "/de" between the domain and the page path in the URL).
    10. Verify the button text is translated.

    Hope this helps you to review.

    If you think there's an issue with translating a scheduled article, could you open a separate issue?

  • 🇮🇪Ireland lostcarpark

    I have added a check that the view header hasn't been modified from the default before removing it, in case the site is using a custom header.

    I will add this to the test cases shortly.

  • 🇺🇸United States cindy_codediaries

    I followed the steps listed on comment #15. It appears the .po file in comment #11 when imported loads successfully. When I switch to the German language it translate some of the button text on the page

    • lostcarpark committed b6a9bc48 on 8.x-3.x
      Issue #3384944 by lostcarpark, cindy_codediaries: SID 2776211 contains...
  • 🇮🇪Ireland lostcarpark

    Rebased for other changes. Verified tests passing.

  • Status changed to Fixed 10 months ago
  • 🇮🇪Ireland lostcarpark

    Merge successful.

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

Production build 0.71.5 2024