Skip to content

New 'behavior.inputLimitTest' for checks the setting of php max_input_vars#7581

Closed
Fedik wants to merge 8 commits intojoomla:stagingfrom
Fedik:behavior-inputlimittest
Closed

New 'behavior.inputLimitTest' for checks the setting of php max_input_vars#7581
Fedik wants to merge 8 commits intojoomla:stagingfrom
Fedik:behavior-inputlimittest

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Jul 29, 2015

Here is another way to check max_input_vars with better precision, at least in theory 😄
The script compare amount of form inputs with server limit.

Currently I added it to the form: article, category, module, menu item, global configuration, plugin, user.

test
Simplest way to test is to create the hundred(or two) usergroups and try open one of mentioned above form.

Original idea by @brianteeman #7456
The text for Warning I copied from that pull, with small modification ... @brianteeman please check 😉
I think, in the warning message instead of your website need something more precision , like current form (because script do test per the form) ... but not sure how to make it correct.

@dgrammatiko
Copy link
Contributor

I like it 👍

@brianteeman
Copy link
Contributor

But this will not work with 3pd extensions will it?

On 29 July 2015 at 18:36, Dimitris Grammatiko notifications@github.com
wrote:

I like it [image: 👍]


Reply to this email directly or view it on GitHub
#7581 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... website is close.."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!
I thought about ... and current form is close to that limit or has reached ..., what you think?

@brianteeman
Copy link
Contributor

I prefer the original plugin approach of having both a warning and a fail message.

@Fedik
Copy link
Member Author

Fedik commented Jul 29, 2015

@brianteeman 3pd extension developer need just add JHtml::_('behavior.inputlimittest', 'form-id'); to the form layout, and script will test the form with form-id

@Fedik Fedik changed the title New 'behavior.inputLimitTest' for for checks the setting of php max_input_vars New 'behavior.inputLimitTest' for checks the setting of php max_input_vars Jul 29, 2015
@brianteeman
Copy link
Contributor

I have tested this and can confirm that it works BUT I really dont like that we just have one message that covers approaching the limit and being over the limit which is always a warning box.

People ignore warnings. My 2c is that this really needs to have separate warning and error messages

@brianteeman
Copy link
Contributor

Looking into this a little further after some previous comments we should probably also check suhosin.request.max_vars and 'suhosin.post.max_vars

AND if we are ABOVE the limit then with the approach you are using it would be best if we quit the submit form and redirect to the manager view and displayed the error message. Perhaps with

You can't edit the report until the limit is changed.

"

@Fedik
Copy link
Member Author

Fedik commented Jul 30, 2015

About these two strings, they are big and difference only 2-3 word, so I thought maybe more simple/efficient use the one common string 😉
Additionally I can change the message type error or warning, depend from case.
But if it really important to have 2 different string, I will add them .. just say

About suhosin.request.max_vars suhosin.post.max_vars I will try add them.
But here I need help.
How to compare them in right way, mean in which order(priority) when server have all of these values?
Simplest way it take the smaller value (but not 0) between all these suhosin.request.max_vars suhosin.post.max_vars , max_input_vars ... but not sure how they really work.

... it would be best if we quit the submit form and redirect to the manager view and displayed the error message

if need we can prevent the form submission for specific tasks (save, apply),
@DGT41 also suggested this, so 2:1 ... I will add 😄

what you mean with redirect to the manager view?
means redirect to the content list view?
if so, I would not do redirect, but display another error 😄 ... reason is that user can enter some data, and after we do redirect this data will be lost (because most people do not read big error message fully 😄), so when we display another (but smaller) error user can save his data somewhere in local TXT file (example) and push "Cancel"

@brianteeman
Copy link
Contributor

This is the scenario.

open a new module for editing
Test discovers we are above the vars limit.
Redirects immediately to the module list and displays the error message.

This prevents you from entering data that will be lost.

@Fedik
Copy link
Member Author

Fedik commented Jul 30, 2015

hm hm, I just realised that make redirect even more complicated in realisation,
because after redirect need somehow find a reason why was redirect ... means there no direct connection between page where we made test and the page where we redirect user ...
I do not very like idea use cookie or something like that.

so maybe we just display another error when user push save/apply?

@brianteeman
Copy link
Contributor

The message should get displayed as messages are queued. Something like this code should work

$this->setMessage(JText::sprintf('ERROR_MESSAGE', $inputLimit, $currentLimit), 'error');
$this->setRedirect(JRoute::_('index.php?option=com_component', false));

@brianteeman
Copy link
Contributor

Also just realised that because this is in the view you will need to update the hathor template overrides as well

@Fedik
Copy link
Member Author

Fedik commented Jul 30, 2015

@brianteeman but we do all thing on the client side, where no PHP 😉

@brianteeman
Copy link
Contributor

Then I would suggest it is not the right approach ;)

(very many php applications do it in their own applications)

@Fedik
Copy link
Member Author

Fedik commented Jul 30, 2015

well, I will try add some changes for prevent form submission, then we'll see 😉

@brianteeman
Copy link
Contributor

Have a look at this code from prestashop - should help
https://github.com/PrestaShop/blocklayered/blob/master/blocklayered.php#L1668

@Fedik
Copy link
Member Author

Fedik commented Jul 30, 2015

thanks!

@Bakual
Copy link
Contributor

Bakual commented Jul 30, 2015

This is the scenario.
open a new module for editing
Test discovers we are above the vars limit.
Redirects immediately to the module list and displays the error message.
This prevents you from entering data that will be lost.

I wouldn't force a redirect. It would prevent sending the form, yes. But it would also prevent just reading what is there.
Showing a warning/error is enough. The user will see that for sure. If you really want to prevent the user from saving, go and disable the save buttons. But don't disable the whole form.

Personally, I would have preferred it to be in the submitform, submitbutton or formvalidate code somewhere, since that would work for any form from any extensions without adding specific code. But it would only work after the user entered the data and tried to save. Which is indeed not optimal. However it would allow to prevent sending the data and actually loose it.

@Fedik
Copy link
Member Author

Fedik commented Jul 30, 2015

ok, so last changes:

  • change the message type: error if limit reached and warning if limit is near. But string the same 😄
  • prevent the form submission for specific tasks if the limit is reached
  • test for suhosin max_vars limits

@ggppdk
Copy link
Contributor

ggppdk commented Sep 17, 2015

Hello

i am also using code to detect max_input_vars limitation (and suhosin limitations)

and if found then serialize the form and submit it via AJAX (it works if form does not have file fields)

i am using this in various forms, modules and in component configuration (com_config)
(i don't think you would like my code, but feel free to look at it if you want)

about this plugin, it is very good idea

just i want to note that using form.length to estimate the form fields does not exclude the

  • disabled form fields, (e.g. in com_config !)

also

  • radio sets should be counted once
  • and only the checked checkboxes should be counted

this way the form's real size is over estimated and can be quite wrong, and it will prevent submiting and saving forms that can be saved

@ggppdk
Copy link
Contributor

ggppdk commented Sep 17, 2015

if you would use jQuery this should give a better count (it will over-estimate by 5% or less)
var submit_count = jQuery(form).find('textarea:enabled, select:enabled, input[type="radio"]:enabled:checked, input[type="checkbox"]:enabled:checked, input:not(:button):not(:radio):not(:checkbox):enabled').length;

@infograf768
Copy link
Member

@ggppdk

We just implemented an alert and prevent edit an ini file if more lines than max_input_vars in com_localise.
joomla-projects/com_localise#287
The alert displays in the Translations view.

I am interested in your Ajax method.
Could you propose a PR there?

@ggppdk
Copy link
Contributor

ggppdk commented Mar 22, 2016

Hi, i believe i will have spare time at end of this week
and i can make a PR by monday (or otherwise by next monday 4 of April), if anyone else wants to do this then please do, just say so that i will not work on it

this can be optionally enabled (e.g. form must add a CSS class to the <form class="jsubmit_doserialize ...") and if JS code detects need it will fallback to serialized submit

Note there is related code with this

  • to submit a form without page reload using AJAX
    this can be an optional button "Fast apply" for forms that wish to add this button (or by adding a class to the we can alter the default 'apply' button)
    (again this works if you don't have file upload field)
  • system messages are rendered and send back to client as json (to be able to include more into the response)

@ggppdk
Copy link
Contributor

ggppdk commented Mar 22, 2016

@infograf768
If i understood you want to propose PR to ?
joomla-projects/com_localise

@brianteeman
Copy link
Contributor

@ggppdk the PR would be great for core - then it wont be needed in com_localise either


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7581.

@ggppdk
Copy link
Contributor

ggppdk commented Apr 14, 2016

@brianteeman
i have just restudied my code and re-read my old comments

Combination of
jQuery.serialize() / PHP parse_str()

  • is subject to the max_input_vars constraint, aka it can not be used to overcome it
  • but also is problematic with empty array values (e.g. submiting ACL rules array)

http://php.net/manual/en/function.parse-str.php

so i am using
JSON.stringify( jform.serializeArray() ) + custom PHP code to deserialize it

  • this also works with empty array values
    (this is needed to restore the POST data exactly as if they were posted without serialization)

I did not have a case to fail so far, but the code is 2 years old

and i would like to search if some solution is now available, that is better than mine
give me some time, i will not neglect this

@brianteeman
Copy link
Contributor

@ggppdk thanks - looking forward to it. This is becoming more of an issue as extensions become more complex and sites become larger


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7581.

@ggppdk
Copy link
Contributor

ggppdk commented Apr 14, 2016

In regards to my comment above, about ACL permissions

  • i do see now, that the permissions are now updated via AJAX call
    and they are no longer submitted ... because they get disabled onSubmit Event

thus some forms become much smaller

i see the merged pull request too ... i had seen before but i forgot of it

@ggppdk
Copy link
Contributor

ggppdk commented Apr 15, 2016

Ok i have written new code, that is much cleaner code and tested it with recursive array_diff on my old method

now i need to find the best place to add / make the check if

  • decompression has been run once (check if the compressed / serialiazed POST data variable exists or not)
  1. Is best place inside JInput ?
  2. also for best compatibility we must add same check inside JRequest (that is IF it possible for J3.x that JRequest will can be used before JInput constructor is ever used ?)

For proper compatibility we will need to also set the variable back into both $_REQUEST and $_POST (without any filtering of course because this is supposed not to have been filtered, that is a job done by validation)

Please look at the constructor of JInput

    public function __construct($source = null, array $options = array())
    {
        if (isset($options['filter']))
        {
            $this->filter = $options['filter'];
        }
        else
        {
            $this->filter = JFilterInput::getInstance();
        }

        if (is_null($source))
        {
            $this->data = &$_REQUEST;
        }
        else
        {
            $this->data = $source;
        }

        // Set the options for the class.
        $this->options = $options;
    }

should we check and do the decompresision (once) at just before line ?:

            $this->data = &$_REQUEST;

@brianteeman
Copy link
Contributor

Sorry I have no idea

Best thing to do is to try one option and submit as a pull request. (it
will then be on the first page and people will see it ;) )

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@ggppdk
Copy link
Contributor

ggppdk commented Apr 21, 2016

About JS serialization code, which also does other things too, like disable the serialized fields and provide way to restore form if submit is done via AJAX ...

  • i am looking for best compatiblity for placing it

(placement for my custom code is different, since i full override joomla validation, and do custom, that is not the case here, so placing it must be done differently)

Also concern is to work with 3rd party extensions too, for Joomla forms there should not be any issue
(that is if form fields count is over max_input_vars / suhosin limits and thus decision is made to run serialization)

I see that for best compatibility now forms are submited

  • either via click on submit button (optionally with "validate" class)
  • or via method Joomla.submitform , which is also triggering a click on a submit button

thus we can not call the serialization code from Joomla.submitform()

need to run it by listening to on-submit event and also make sure that it runs after at least the 2 validation functions

  • isValid (validate.js)
  • validateForm (html5fallback.js)

** OF course if form validation fails no serialization is done (YET) **

i think to do serialization only if html5fallback.js is loaded, if not loaded then not do anything !

  • so it will work for Joomla forms and for 3rd party that load: html5fallback.js

any ideas are welcome

anyway you can wait for new PR and suggest moving the code elswhere

@Fedik Fedik closed this May 21, 2016
@brianteeman
Copy link
Contributor

@ggppdk are you going to submit a PR - this is becoming more and more important

@ggppdk
Copy link
Contributor

ggppdk commented Aug 29, 2017

I am sorry for not submiting such a PR already

Yes i can do it,
I am using it for more than 1 and half years now in all type of forms that have
-- many input fields
menu items, modules, my component forms, and even loading it in joomla article form too

i will make a PR please give a week, i ll find time to do it

please note that JS code (client) is using jQuery
but the good is that server side code does not use eval() but it still works properly to decompress all cases, including empty multi-level array input fields

@ggppdk
Copy link
Contributor

ggppdk commented Aug 29, 2017

@brianteeman

i have opened an open issue here:
#17757

so that this work is not neglected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants