Skip to content

Comments

PHP max_input_vars test plugin#7456

Closed
brianteeman wants to merge 30 commits intojoomla:stagingfrom
brianteeman:maxvarsplugin
Closed

PHP max_input_vars test plugin#7456
brianteeman wants to merge 30 commits intojoomla:stagingfrom
brianteeman:maxvarsplugin

Conversation

@brianteeman
Copy link
Contributor

Since php 5.3.9 there is a variable max_input_vars with a default value of 1000.

_Ever had a large site suddenly stopped saving changes - this is why!!_

Unfortunately on sites with lots of usergroups, menu items, modules and/or categories this will create issues when saving or making changes. Sadly it is a silent error as it is only a notice in php so depending on your site and server error configuration you might never know about it. All you do know is that you press save and nothing happens.

This plugin works in the same way as the old EOSNOTIFY plugin as a quickicon plugin and is only fired on the admin home screen.

It takes a count of the usergroups, menu items, modules and categories and displays either an error message if the total is greater than the php value for max_input_vars or a warning message if the total is greater than 80% of the available variables.

This can only be an estimate of the variables in use. The only other way would be to count the ones in use on every page and this would be an unnecessary overhead.

To test - Apply the patch, discover the plugin and enable it and then all I can think of is to add echo $varcount and echo $maxinputvars to your admin template and check that $varcount is less than 80$ of $maxinputvars.

Warning Screen

warning

Error Screen

error

thanks to @zero-24 and @rdeutz who helped me to create this plugin (my first)

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Jul 17, 2015
@brianteeman
Copy link
Contributor Author

Another method to test would be to massively reduce the setting for max_input_vars on your site from say 1000 to 100

With the sample testing data this will easily trigger the error message and with a clean no sample data install you should see the warning.

Another option would be to use com_overload from @nikosdion to create a crazy amount of data

Copy link
Contributor

Choose a reason for hiding this comment

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

@brianteeman can I propose a quick bail out for the front end

if ( JFactory::getApplication()->isSite() )
{
    return;
}

Also maybe is possible to extend this even further so it runs only when com_panel is the component? (I am guessing the message is displayed there…)

Copy link
Contributor

Choose a reason for hiding this comment

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

The whole quickicon group is only loaded on the cpanel view (from the quickicon module). So it isn't really needed to bail out otherwise.

@roland-d
Copy link
Contributor

Pages with too many items, would be great to ajaxify them. This solves this problem as well.

@brianteeman
Copy link
Contributor Author

@roland-d how on this earth would that solve the problem - but its off topic anyway

@roland-d
Copy link
Contributor

@brianteeman It solves it by not having to post the huge number of variables. I shouldn't have posted it here as you said, it's off-topic.

@Bakual
Copy link
Contributor

Bakual commented Jul 18, 2015

Let me be really clear as people are not reading so I have to repeat

This can only be an estimate of the variables in use. The only other way would be to count the ones in use on every page and this would be an unnecessary overhead.

Don't assume people don't read. That's not nice.

I wondered why you got to that estimate, as I can't think of a form where this would apply.
Let's assume we have 300 items in each of the tables in question. With your calculation it would show a big error as the count would be 1200. However in practice it would be max 600+something in the module form. Still far away from the limit. Even if there is a form with all three groups present (which I'm not aware) it would still work.
Imho, it doesn't make sense to show warnings if there is no issue at all.

It doesn't mean we have to count for each page, but we should maybe think about the cases and check if the problematic combinations yield an issue.

So instead of checking the total, check eg $menuitems + $usergroups.

@cyrez
Copy link
Contributor

cyrez commented Jul 18, 2015

@brianteeman thank you opening this PR! 👍
This could apply to config for any extensions having a lot of settings.

There's suhosin to be checked too.
What's about something using client-side to control each form before submit like this one for WP : https://github.com/academe/wp-max-submit-protect/blob/master/wp-max-submit-protect.php ?
The script files are MIT licenced, and it seems to be easily integrated into a Joomla plugin.

@btoplak
Copy link

btoplak commented Jul 18, 2015

This is good idea to inform users about possible issue.

Later on the plugin could go further to prevent submitting a form which would loose some data, like the WP plugin mentioned by @JoomliC

All should take into account that this limit is there for a reason. Originally it was made to mitigate some DoS attack vector (CVE-2011-4885). So, don't push it too high. Maybe you could include such remark in plugin's notice?

@btoplak
Copy link

btoplak commented Jul 18, 2015

P.S. @JoomliC mentioned Suhosin.

suhosin.post.max_vars
suhosin.request.max_vars
suhosin.get.max_vars

Those variables have the same function as max_input_vars, so if they are set the problem would be the same. I'd suggest you check them too and take the lowest value as a threshold.

@brianteeman
Copy link
Contributor Author

OK tried to help users with this. This is a real world problem . yiuve spoken clearly and I wontnwaste my time again

@btoplak
Copy link

btoplak commented Jul 18, 2015

It was a good idea.

@brianteeman
Copy link
Contributor Author

Here is an example of what happens on a site with many modules and only a few menu items if you hit the limit of max_vars_input. Nice how Joomla says everything is saved and working perfectly and how nothing is actually saved at all.

maxvars2

@brianteeman brianteeman reopened this Jul 21, 2015
@Fedik
Copy link
Member

Fedik commented Jul 27, 2015

just got idea how to count this thing with better precision for an each form, and without SQL involve 😄
it can be a simple JavaScript code:

if(form.length >= maxVars){
  Joomla.renderMessages({'warning':[Joomla.JText._('PLG_QUICKICON_MAXVARS_WARN'])})
}

just not sure where the right place for add this code, maybe onContentPrepareForm?
and how safe to expose the max_input_vars value on the client side? hm, should be safe enough, if do it only for admin

@dgrammatiko
Copy link
Contributor

@Fedik why not a function on core.js and a call from submit (or something like that) ?

@Fedik
Copy link
Member

Fedik commented Jul 27, 2015

@DGT41 then this means that User will see the warning only when push "submit" (save, cancel .. so on)
not sure that it is good idea 😉
And we still need to have maxVars (max_input_vars) value on the client side , somehow (we still do not have a simple way to add custom options #3072 😄 )

I see two way:

  1. Add this code by content plugin, in onContentPrepareForm event
  2. Add this code directly to the form layout, in each component in edit.php ... by new JHtml::behavior method

but not sure, maybe there a better way

@dgrammatiko
Copy link
Contributor

@Fedik maybe just manipulating the save, the save and close and the save and new buttons to inject this script (or actually prevent the usage of those buttons, disabled) with conjunction of the plugin that will raise the warning, is easier to implement?

@Fedik
Copy link
Member

Fedik commented Jul 27, 2015

@DGT41 I thought enough just show the warning,
I not sure that we need to prevent anything, form will not be saved anyway (in most cases),
but yes, we can do this, if need 😉

ok, so I vote for "make new behavior", name something like JHtml::_('behavior.maxvartest');, so we can add it in any form that we need, and extension developers also can use it in their own extensions.

@Fedik
Copy link
Member

Fedik commented Jul 27, 2015

hm, @DGT41 I think better warn User about possible problem before he/she fill the form, than make "surprise" after he/she spend some time to fill the form

@dgrammatiko
Copy link
Contributor

@Fedik I am not gonna support my my own idea, because there is a way better option here: ajaxify these permissions. @phproberto already showcased this: https://www.youtube.com/watch?v=vNZZhi7WB-c

@brianteeman
Copy link
Contributor Author

thats only part of the issue with max_input_vars and until @phproberto
contributes that code it doesnt exist

On 27 July 2015 at 16:17, Dimitris Grammatiko notifications@github.com
wrote:

@Fedik https://github.com/Fedik I am not gonna support my my own idea,
because there is a way better option here: ajaxify these permissions.
@phproberto https://github.com/phproberto already showcased this:
https://www.youtube.com/watch?v=vNZZhi7WB-c


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

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

@Fedik
Copy link
Member

Fedik commented Jul 27, 2015

@DGT41 well, sure that would be a very good solution 😉
but by looking through the window of reality I see very low chance get something like that in short term 😄

@Fedik
Copy link
Member

Fedik commented Jul 29, 2015

so there is JavaScript way #7581 to test max_input_vars limit

@brianteeman
Copy link
Contributor Author

Closed due to lack of interest

@brianteeman brianteeman deleted the maxvarsplugin branch October 29, 2015 00:29
@YaegerDesign
Copy link

This little plugin would have helped save us several hours of troubleshooting and a client several years of (potentially) lost data!


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

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.

10 participants