Problem/Motivation

In issue #2362321: Check source length limits we've tackled the error of translations coming with length bigger than accepted by field definitions, in the review form.

But translators can have the definition to "Auto accept finished translations" activated. If so, when submitting a translation with a field having length above the allowed, we have an SQL error:

File import failed with the following message: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'field_teste_value'

Note: I got this error when testing with File translator.

Proposed resolution

Add a global field_length_overflow_policy setting controlling what happens on acceptance when a translation exceeds the field length:
- Keep the item for review (needs_review, default): acceptance is refused and a message links to the review form.
- Truncate (truncate): the translation is shortened at a word boundary to fit, with a warning.

Remaining tasks

?

User interface changes

New "Field length overflow policy" select on admin/tmgmt/settings.

API changes

None.

Data model changes

New config key tmgmt.settings:field_length_overflow_policy.
Existing sites get needs_review via tmgmt_update_8012(), which matches the previous outcome (the over-length translation is not saved and the item stays in review) but replaces the uncaught database exception with a clear message.

Issue fork tmgmt-2960458

Command icon 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:

About issue forks

Comments

dxvargas created an issue. See original summary.

dxvargas’s picture

dxvargas commented
Title: Check source length limits » Check source length limits when "Auto accept finished translations" in enabled
dxvargas’s picture

dxvargas commented
Issue summary: View changes
dxvargas’s picture

dxvargas commented
Title: Check source length limits when "Auto accept finished translations" in enabled » Check source length limits when option "Auto accept finished translations" is active
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue tags: -

I had a chat on Slack with @Berdir on this. He said

at a glance, I'd say shortening together with a message that this was done seems like a sensible last resort to avoid the exception

Shortening should be made with truncation at word boundaries

herved made their first commit to this issue’s fork.

herved opened merge request !168

herved opened merge request !169

herved opened merge request !170

herved’s picture

herved commented

Since this is a bug, I wanted to keep the solution simple. However, I think there are at least two behaviors that projects might prefer:
a) Automatically truncate the value and move the job item to the Accepted state (2960458-truncate branch).
b) Block the job item and keep it in Review (2960458-block branch).

I also created a third option that makes this behavior configurable (2960458-configurable branch), with truncation as the default.
Any feedback is welcome.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs work

I've only reviewed the one that gives the site builder the power to decide. In my projects, with continuous jobs, I don't want to review such cases, I want to enforce truncation. But I have no idea how other projects think. Even is introducing a new config, it leaves the decision to the site builder. I would suggest we focus on this MR.

herved changed the visibility of the branch 2960458-block to hidden.

herved changed the visibility of the branch 2960458-truncate to hidden.

herved’s picture

herved commented
Status: Needs work » Needs review
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: Unassigned » claudiu.cristea

Picked up for review

herved’s picture

herved commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: claudiu.cristea » Unassigned
Status: Needs review » Needs work

In https://git.drupalcode.org/issue/tmgmt-2960458/-/pipelines/860363, I've manually started:

  • next minor
  • previous major
  • previous minor

Compared to HEAD's latest pipeline https://git.drupalcode.org/project/tmgmt/-/pipelines/708592 we have an additional "phpunit (next minor)" failure. We should not add additional failures, at least, in PHPUnit jobs.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Reviewed & tested by the community

Ok, so https://git.drupalcode.org/project/tmgmt/-/pipelines/708592 ran 5 months ago. The failures here are because Drupal core evolved. There's nothing to be done here.

Thank you for the work on this

herved closed merge request !169

herved closed merge request !168

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: Unassigned » claudiu.cristea
Status: Reviewed & tested by the community » Needs work

Just discovered that this is not covering properties from fields such as the "alt" field, see #3521659: Check max length of media image alt text. I'm working to extend this rather to work on the other as this has already tests and a clear approach on the "overflow policy"

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: claudiu.cristea » Unassigned
Status: Needs work » Needs review

Because of #21 I'm proposing a change to the initial approach:

TL;DR Don't check the text field max length setting, check the field property schema max length

Instead of checking the max_length field setting of the StringItem fields, and successors, it checks the max length of the field property schema, if any. This has the advantage that covers, along with StringItem, and its successors, any other field property whose data type is 'string'. Should cover the image field properties such as "alt" and "title"; the link field "title" property. It should cover all field string translatable properties which are not main property.

Other changes:

  • Extended the initial test to cover the image & link fields
  • Asserting also that proper log messages were registered

This is ready for a new review

andras_szilagyi’s picture

andras_szilagyi commented
Status: Needs review » Reviewed & tested by the community

looks good

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Reviewed & tested by the community » Fixed

Thank you for contribution. Merged

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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