Prevent to save too large module content > 65535

[alikon]

Pull Request for Issue #24631 .

Summary of Changes

added a check to prevent to save too large module content > 65535

Testing Instructions

• Create a module containing a field type of editor.
• Using this module, enter any content which exceeds 65535 characters.

Expected result

an error message is displayed that you are not permitted to save too large content

Actual result

no check

[mbabker]

This type of validation belongs in the table class, not the model.

If character limitations are going to be checked, then this should be applied consistently across core and not only on this field IMO. Then again, from what I remember at least MySQL throws warnings about right truncating columns when the values are too big. Is this not happening consistently in Joomla, and if so why not!? It seems like that should be an Exception that someone can catch and handle more gracefully than this type of check.

[alikon]

you are right as always, btw, should be a little bit better now....not perfect but....
check moved on table class

[ghost]

@ricomonkeon please test.

[ghost]

@alikon is it okay for you if i copy the testing instructions from #24631?

[alikon]

@franz-wohlkoenig feel free to do it

[ricomonkeon]

Just tested it. I'm out of my depth in this part of Joomla, but should it not be $data[content] instead of $this->content ? (the $this object does not seem to contain the submitted data)
Also, note that if you add an editor field type in your module, then it does not use the content field, but saves it as part of a json string in the params field with the rest of the data. Only customContent seems to use the content field.
The failure in $data[content] isn't actually as bad - it's the broken json string in params which causes a fatal error.

[alikon]

well i've only experienced with content .... btw added the check on params too
please re-test

[ricomonkeon]

$this->content and $this->params always seem to return a length of zero.

It feels like -
if ((strlen($this->content) > 65535) || (strlen($this->params) > 65535))
Should be something like -
if ((strlen($data[content]) > 65535) || (strlen(json_encode($data[params])) > 65535))
The latter works on my test, but I'm not sure how tidy my code is.

[alikon]

can you post the zip of your module please

[mbabker]

t feels like -
if ((strlen($this->content) > 65535) || (strlen($this->params) > 65535))
Should be something like -
if ((strlen($data[content]) > 65535) || (strlen(json_encode($data[params])) > 65535))

Make sure you're editing the right file. In the table's check() method, the properties are bound to the class before it is called so $this->foo should be what you're using in that context.

If you're having to check $data['foo'] then you might be editing the wrong file.

[ricomonkeon]

Apologies. I was indeed editing the wrong file.
The bug is now fixed - good work!

[ghost]

@ricomonkeon please mark your test in Issue Tracker as successfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results

[HLeithner]

2 questions.

1. Why do you limit the params mediumtext column to 65k chars? it should be 16m
2. Why do we limit to text, we as joomla has no gain from this, especially if we don't have another column that can be used for meta data.

params would be such column but it seams that it isn't used by devs for there needs, why don't they use it and would a new column dedicated for easy use better?

[HLeithner]

and btw. this is a real bad user experience if you edit something and the system tells you I can't save it without any help how to solve it. Especially when extensions are abusing this field and fill it with json data that isn't seen or can't be modified by the user.

[alikon]

both field content and params are declared as text
https://github.com/joomla/joomla-cms/blob/staging/installation/sql/mysql/joomla.sql#L1498
https://github.com/joomla/joomla-cms/blob/staging/installation/sql/mysql/joomla.sql#L1509

so 65,535 characters should be the max length allowed iirc

the goal of this pr is really simple: only to prevent data truncation, it's only a band-aid
not a schema redesign nor a complete review of Exception flows as correctly Michael pointed out #24635 (comment)

[HLeithner]

sorry about the mediumtext, the first website I checked had it as mediumtext and I don't know why.

ok then I understand thx.

[ricomonkeon]

I have tested this item ✅ successfully on 406da01

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24635.}

[viocassel]

I have tested this item ✅ successfully on 93e3505

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24635.}

[ghost]

@HLeithner set this on RTC or retest of @ricomonkeon needed?

[zero-24]

No need to retest. We just need to get appveyor running. Let me reboot him

[zero-24]

https://ci.appveyor.com/project/release-joomla/joomla-cms/builds/23987426

[ghost]

Status "Ready To Commit".

[anirudh7]

But why text limitations? I have 60+ Speakers attending the conference, and I want to use as a module in multiple paces.