Problem/Motivation

Recently, accepting of items resulted in a fatal error.
Reason:

PDOException: SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'meta_description_metatags_quick

Sure, it is wrong that metatags does not check the boundaries and die fatal here.
It sounds easy to say, this is a metatags quick issue to not check the incoming length.
But the origin of the issue is more general.

Proposed resolution

During review already, users should realise that there is a technical limit and what they review is not fitting.
Also, translators should get a comment about the hard length limit.

UX needs to be improved here.

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Component: User interface » Core

Is this a normal field? Then it's not a metatags issue, it's a core issue.

What we can do is extract the length based on the field settings, and then add that to the data item structure, as #max_length or so. then we can verify that and translators can use it if they want to.

miro_dietiker’s picture

Issue tags: +8.x release target

Well then it's a 8.x release target... :-)

miro_dietiker’s picture

In Drupal 8, we can call validation first and abort with the provided error messages.
Still, in case of the metatags_quick module, it's no regular field and the information is not always available beforehand.
(This still requires that the module properly validates... and metatags_quick does not yet.)
In Drupal 7, this is not possible.

miro_dietiker’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev

Moving to the current working context.

miro_dietiker’s picture

Priority: Normal » Major

Major because it leads to fatal errors in real cases.

edurenye’s picture

I could not reproduce this error, I need more info.

edurenye’s picture

Finally I could reproduce the error, but seems to be a problem of the core, node needs to validate tags length.

edurenye’s picture

I created a issue for this #2569549: Add length validation for tags in NodeForm, but I'm not sure if we still will need to do somthing here, or still I'm not sure that the issue that I found is the same than this one.

miro_dietiker’s picture

@edurenye Before we can validate, TMGMT needs to have the data. The idea is that a source plugin checks the definition (say an entity field definition) for a limitation in length. It then annotates that through #max_length to the data provided. Each source plugin needs to define on its own if there is a length limit of data items that are added to a job.
TMGMT then can natively support looking at this length limit and validate when a translation is saved.

And some translators also pass information to the (human) translator about background of an item. We should tell them about the length limitation. Dunno anymore how much these comments are provided by the translator only... but we might want to provide such a default human translator comment for the item in the TranslatorBase.
From looking at the code, providing a default human comment considering the length limitation seems to be a followup.

miro_dietiker’s picture

For now we don't want to output the max length in the UI to avoid noise.

Passing it into a comment for translators should go into a followup.

edurenye’s picture

Assigned: Unassigned » edurenye
edurenye’s picture

edurenye’s picture

Fixed and added test.

The last submitted patch, 13: check_source_length-2362321-13-test_only.patch, failed testing.

miro_dietiker’s picture

Status: Needs review » Needs work
Related issues: +#1891862: content health checks
  1. +++ b/src/Form/JobItemForm.php
    @@ -172,6 +172,25 @@ class JobItemForm extends TmgmtFormBase {
    +  private function getTranslatableFields(JobItemInterface $item) {
    ...
    +    foreach ($item->getData() as $key => $value) {
    
    @@ -205,12 +225,30 @@ class JobItemForm extends TmgmtFormBase {
    +    $translatable_fields = $this->getTranslatableFields($item);
    ...
    +      if (array_key_exists($parent_key, $translatable_fields) && ($form_state->getTriggeringElement()['#value'] != 'Validate HTML tags')) {
    

    Hmm, JobItemForm::form does flattening on getData()

        $data = $item->getData();
        foreach (Element::children($data) as $key) {
          $form['review'][$key] = $this->reviewFormElement($form_state, \Drupal::service('tmgmt.data')->flatten($data[$key], $key), $item, $zebra, $key);
    

    You are duplicating logic here and i think you don't properly consider deep nesting cases. And yes, before they worked cleanly.
    We should be very clear about flattening / unflattening and always access things properly. Eduard also already asked why we don't have Data interfaces and yeah they should consequently also support both domains..

    Possibly reviewFormElement should bind data to the $form item so you can access it when iterating correspondingly..?

    It looks to me like if we push validation forward, then we need to significantly improve in that domain otherwise we might end up with very high code complexity and everywhere special cases about nesting and flattening... (that are still limited and do not support all possible cases...) we already had this multiple times with simple body conversion to editors with format, references, ... and again this looks like a rebirth #2594841: Tests for deep nested validation for JobItem reviews

  2. +++ b/src/Form/JobItemForm.php
    @@ -205,12 +225,30 @@ class JobItemForm extends TmgmtFormBase {
    +            $this->t('The field has more than :size characters.', array(':size' => $translatable_fields[$parent_key][0]['value']['#max_length'])));
    

    We should be nice and tell how many are too much.

Berdir’s picture

+++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
@@ -124,6 +125,11 @@ class ContentEntitySource extends SourcePluginBase {
+            if ($field_item instanceof StringItem) {
+              $data[$key][$index][$property_key]['#max_length'] = $field_item->defaultStorageSettings()['max_length'];
+            }

That's the wrong method. You want getStorageDefinition()->getSetting('max_length'), this is just the default, as the method says.

Based on that, we should also have an actual integration test with content entity source. Extend the unit test by specifying the max-length setting on a field we create there.

edurenye’s picture

Now I use the flattened data, aded unit tests for source and not using defaultSettings anymore.
The error also now shows the actual size.

Status: Needs review » Needs work

The last submitted patch, 17: check_source_length-2362321-17.patch, failed testing.

edurenye’s picture

Status: Needs work » Needs review

Head is broken.

The last submitted patch, 13: check_source_length-2362321-13-test_only.patch, failed testing.

The last submitted patch, 13: check_source_length-2362321-13.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: check_source_length-2362321-17.patch, failed testing.

juanse254’s picture

Status: Needs work » Needs review

This looks good now and the tests are passing.

The last submitted patch, 13: check_source_length-2362321-13-test_only.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
    @@ -124,6 +125,11 @@ class ContentEntitySource extends SourcePluginBase {
    +          if ($translate) {
    +            if ($field_item instanceof StringItem) {
    +              $data[$key][$index][$property_key]['#max_length'] = $field_item->getFieldDefinition()->getFieldStorageDefinition()->getSetting('max_length');
    +            }
    

    Don't limit this to a specific class. TextItem for example has a max_length too. Just check if there is a max_length and if so, add it.

    As discussed, we have a problem with the fact that the setting is on the field level but we add it on the property level.

    There is no really clean solution, one thing we could think about is checking the schema() instead.

  2. +++ b/sources/content/src/Tests/ContentEntitySourceUnitTest.php
    @@ -364,7 +377,7 @@ class ContentEntitySourceUnitTest extends EntityUnitTestBase {
       protected function drupalCreateContentType($settings = array()) {
         $name = strtolower($this->randomMachineName(8));
    -    $values = array(
    +    $values = array_merge($settings, [
           'type' => $name,
    

    Valid fix but I can't see where you'd use this? Also, normally, $settings + array( is used for this, or simply rename values to settings and use $settings += [..

    But again, you don't actually use this here...?

  3. +++ b/sources/content/src/Tests/ContentEntitySourceUnitTest.php
    @@ -483,4 +496,70 @@ class ContentEntitySourceUnitTest extends EntityUnitTestBase {
    +    // The acceptTranslation will fail as the translation from testTranslator
    +    // will have 3 extra characters.
    +    try {
    +      $item->acceptTranslation();
    +      $this->fail('Translation was accepted');
    +    }
    +    catch (\Exception $e) {
    +      $this->pass('Could not accept the translation');
    +    }
    

    What exactly are we testing here? I guess what's actually exploding is the sql query trying to store the value? That's not a very useful thing to test.

    Doing real validation is hard, but I'd rather just leave out this part of the test unless we have our own specific validation for this, with a specific exception and helpful message (that we'd then assert).

  4. +++ b/src/Form/JobItemForm.php
    @@ -205,12 +205,32 @@ class JobItemForm extends TmgmtFormBase {
    +    foreach ($this->getTranslatableFields($form) as $parent_key => $value) {
    +      $key = $parent_key . '|0|value';
    +      if ($form_state->getTriggeringElement()['#value'] != 'Validate HTML tags') {
    +        // If has HTML tags will be an array.
    

    this wasn't added here, but a) why is that check within the loop and b) #value is a translatable string, that will fail if that is translated.

  5. +++ b/src/Form/JobItemForm.php
    @@ -205,12 +205,32 @@ class JobItemForm extends TmgmtFormBase {
    +        // Validate that is not longer than the max length.
    +        if (isset($value[$key]['translation']['#max_length'])
    +          && ($value[$key]['translation']['#max_length'] < strlen($label))) {
    +          $form_state->setError($form['review'][$parent_key][$key]['translation'],
    +            $this->t('The field has :size characters while the limit is :limit.', [
    +              ':size' => strlen($label),
    

    Validation like this would possibly be a lot easier if we'd use #element_validate.

edurenye’s picture

I modified it because in one try I tried to send settings to that method and realized of that. Should I move it to a follow up?
I'm testing that the translation is not accepted because it longer than the limit, the fact that acceptTranslation don't check it and throw an exception, so maybe we might open a followup to add a validation there also.

miro_dietiker’s picture

Status: Needs review » Needs work

Another quick review.

  1. +++ b/src/Form/JobItemForm.php
    @@ -205,18 +206,46 @@ class JobItemForm extends TmgmtFormBase {
    +   * Validate that is not longer than the max length.
    

    Not a real sentence. ;-)

  2. +++ b/src/Tests/TMGMTUiTest.php
    @@ -496,25 +496,52 @@ class TMGMTUiTest extends TMGMTTestBase {
    +    $label = '<p><em><strong>Source text bold and Italic</strong></em></p>';
    ...
    +          '#text' => $label,
    ...
    +            '#text' => $label,
    ...
    +      'title|0|value[translation]' => $label,
    ...
    +      ':size' => strlen($label),
    

    Not really a $label.

edurenye’s picture

Done.

Berdir’s picture

Status: Needs review » Needs work
+++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
@@ -126,9 +126,7 @@ class ContentEntitySource extends SourcePluginBase {
-            if ($field_item instanceof StringItem) {
-              $data[$key][$index][$property_key]['#max_length'] = $field_item->getFieldDefinition()->getFieldStorageDefinition()->getSetting('max_length');
-            }
+            $data[$key][$index][$property_key]['#max_length'] = $field_item->getFieldDefinition()->getFieldStorageDefinition()->getSetting('max_length');

It still needs to be condition. max_length should only be set if there is a non-zero setting value.

I'm testing that the translation is not accepted because it longer than the limit, the fact that acceptTranslation don't check it and throw an exception, so maybe we might open a followup to add a validation there also.

I think we already have an issue to use entity validation, which could then add test coverage for this. The test coverage that you added is pretty much meaningless, so I think we should not include it here.

edurenye’s picture

Rebased and deleted the unit tests.
Also modified the validation to support deep nesting, due to the new tests added in this issue #2594781: Do not validate on save review, not sure if is fully supported but this has to be analized in this issue #2594841: Tests for deep nested validation for JobItem reviews, in case is fully supported then I think we have to add more test coverage there for all the different cases.

  • Berdir committed a654c3c on 8.x-1.x authored by edurenye
    Issue #2362321 by edurenye: Check source length limits
    
Berdir’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev
Assigned: edurenye » Unassigned
Status: Needs review » Patch (to be ported)
+++ b/src/Form/JobItemForm.php
@@ -219,18 +223,45 @@ class JobItemForm extends TmgmtFormBase {
+        $this->t('The field has :size characters while the limit is :limit.', [
+          ':size' => strlen($element['#value']),
+          ':limit' => $element['#max_length'],

This should use @ placeholders, : is reserved for URL's. Fixed on commit.

Yeah, the whole form validation stuff doesn't exactly get prettier, but agreed, we can improve that in the plenty of related issues on that topic.

edurenye’s picture

Dan Kolbas’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev

I am changing this to 8.x-1.x-dev it is currently marked as a fix for drupal 7, but those files don't exist in D7 module. So I looked at the D8 module and they do exists. I assume that means this ticket is labeled wrong.

Granted this issue still exists in D7 latest release branch. Unless someone knows of another patch.

Berdir’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev

@Dan Kolbas: Yes, Version 7.x-1.x + Status Patch (to be ported) means exactly that. It was fixed for Drupal 8, but that fix needs to be backported to Drupal 7.

Changing the version back.

Dan Kolbas’s picture

Ahh, okay that makes sense.

So I added a patch for the D7 instance in trying to address the issue. Feel free to use or criticize it. : )

https://www.drupal.org/project/tmgmt/issues/2929207

dxvargas’s picture

I'm now submitting a patch for D7.

I have tried to follow the way D8 version validates both empty translations and length.
Length is checked by adding maxlength property to the form fields definition. Patch #30 uses save() method, but in current dev version this is done in buildTranslation() method.
Empty translation is checked by adding a #validate callback to the "accept" action. This way, validation occurs only when accepting the translation while still allowing to just save it.

@Dan Kolbas , your patch in https://www.drupal.org/project/tmgmt/issues/2929207 didn't work for me and also the approach didn't seem to follow the D8 concerns. So I've opted to start a new patch here.

albapb’s picture

I tested patch #37 and it solved the problem.

enriquelacoma’s picture

I tested patch #37 and worked as intended

Berdir’s picture

Status: Patch (to be ported) » Needs work
Issue tags: -8.x release target

Thanks for working on this. A test would be great to make sure we won't break this in the future. Should be possible to port the relevant test additions more or less directly to 7.x

dxvargas’s picture

I'm now submitting a new patch which covers the case where length is defined in columns, which may happen in multi value fields.

I've tested this patch with a Link field after applying patch #9 in issue 2489134.

AritoMelo’s picture

I am submitting this patch with follow the previous one and add more checks:
Change log

  1. Fixed the bug for body field
  2. Implemented the logic to treat text_with_sumary and long_text field type, which was the the bug cause.
  3. Implemented a logic for link test.
  4. Break the whole logic into two function.
  5. Small improvements and typo.
Delphine Lepers’s picture

Status: Needs work » Reviewed & tested by the community

I have successfully used this patch in my websites.

seanm89’s picture

Using the latest version of TMGMT for Drupal 8 and i still get a fatal error when going over maximum source limit for a "Text(Formatted)" field when translating via Drupal User