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
Comment | File | Size | Author |
---|---|---|---|
#42 | check_source_length-d7-2362321-42.patch | 5.1 KB | AritoMelo |
| |||
#41 | interdiff_37-41.txt | 718 bytes | dxvargas |
#41 | check_source_length-d7-2362321-41.patch | 2.1 KB | dxvargas |
| |||
#37 | check_source_length-d7-2362321-37.patch | 1.84 KB | dxvargas |
| |||
#30 | check_source_length-2362321-30.patch | 10.12 KB | edurenye |
Comments
Comment #1
BerdirIs 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.
Comment #2
miro_dietikerWell then it's a 8.x release target... :-)
Comment #3
miro_dietikerIn 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.
Comment #4
miro_dietikerMoving to the current working context.
Comment #5
miro_dietikerMajor because it leads to fatal errors in real cases.
Comment #6
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedI could not reproduce this error, I need more info.
Comment #7
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedFinally I could reproduce the error, but seems to be a problem of the core, node needs to validate tags length.
Comment #8
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedI 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.
Comment #9
miro_dietiker@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.
Comment #10
miro_dietikerFor 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.
Comment #11
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedComment #12
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedComment #13
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedFixed and added test.
Comment #15
miro_dietikerHmm, JobItemForm::form does flattening on getData()
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
We should be nice and tell how many are too much.
Comment #16
BerdirThat'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.
Comment #17
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedNow I use the flattened data, aded unit tests for source and not using defaultSettings anymore.
The error also now shows the actual size.
Comment #19
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedHead is broken.
Comment #23
juanse254 CreditAttribution: juanse254 at MD Systems GmbH commentedThis looks good now and the tests are passing.
Comment #25
BerdirDon'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.
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...?
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).
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.
Validation like this would possibly be a lot easier if we'd use #element_validate.
Comment #26
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedI 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.
Comment #27
miro_dietikerAnother quick review.
Not a real sentence. ;-)
Not really a $label.
Comment #28
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedDone.
Comment #29
BerdirIt still needs to be condition. max_length should only be set if there is a non-zero setting value.
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.
Comment #30
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedRebased 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.
Comment #32
BerdirThis 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.
Comment #33
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedCreated a followup for comment 26: #2619770: acceptTranslation in JobItem should return false if can not accept the translation, not an exception
Comment #34
Dan Kolbas CreditAttribution: Dan Kolbas commentedI 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.
Comment #35
Berdir@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.
Comment #36
Dan Kolbas CreditAttribution: Dan Kolbas commentedAhh, 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
Comment #37
dxvargas CreditAttribution: dxvargas commentedI'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.
Comment #38
albapb CreditAttribution: albapb commentedI tested patch #37 and it solved the problem.
Comment #39
enriquelacoma CreditAttribution: enriquelacoma commentedI tested patch #37 and worked as intended
Comment #40
BerdirThanks 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
Comment #41
dxvargas CreditAttribution: dxvargas commentedI'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.
Comment #42
AritoMelo CreditAttribution: AritoMelo as a volunteer and commentedI am submitting this patch with follow the previous one and add more checks:
Change log
Comment #43
Delphine Lepers CreditAttribution: Delphine Lepers at Arhs for European Commission and European Union Institutions, Agencies and Bodies commentedI have successfully used this patch in my websites.
Comment #44
seanm89 CreditAttribution: seanm89 commentedUsing 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