"Comments per page" setting cannot be configured to be larger than 300

Plus one to number in d8.
D7 drush vset works

Patch seems sane to me, and due to the fact this is an HTML5 widget there is no need to further information (the D7 patch will probably need a better description to inform the user what is allowed and what is not).

Applied the patch to a fresh D8 HEAD, and it worked as intended.

#type => number indeed makes a lot more sense here.

Committed and pushed to 8.x. Thanks!

Sounds like this needs solving in 7 as well? Would have to use a different approach, though.

I haven't checked that fapi allows all of this, but it's an initial port anyways.

So an alternative patch, that does as is suggested in the issue summary for D7, i.e. it just adds an option to the select box on the form for the currently set variable. This means that this setting doesn't get lost when submitting the form, but doesn't introduce any UI changes.

Note that other modules do actually use the _comment_per_page function, like ctools.

The issue summary also suggests the value will need some validation, the same that the D8 number form element has. Should we at least verify that the value is an integer before displaying it as part of the options?

@dcam Ah yes.

Find attached a patch that adds validation. Sadly we can only enforce is_numeric since is_int would return FALSE for any value set through Drupal's web UI (since they are strings that are just coerced into integers at the appropriate points. But the rest of the line should enforce that it's actually an integer (maybe stored as a string).

Not sure if this needs tests or not, because the D8 version got in fine without tests.

This works fine on my testing here http://s2acf8d9f2e2009f.s3.simplytest.me/node#overlay=admin/structure/ty...

I think it's fine for me to mark this RTBC, but I gave the first hack at backporting this before @Steven Jones went through it with @dcam.

RTBC +1 as far as the functionality goes. When I applied #9 I was able to set a custom value for the comments per page using Drush as long as it was a multiple of 10 and between 10 and 1000.

I am a little concerned, however, that this is basically going to be a hidden feature. Is there a policy about this sort of thing? Does this concern even matter at this point in the 7.x lifecycle?

I am a little concerned, however, that this is basically going to be a hidden feature. Is there a policy about this sort of thing? Does this concern even matter at this point in the 7.x lifecycle?

Hmm...I would argue that all we're doing here is allowing people to know how to change that variable outside of the web UI to then not have it wiped out by the web UI. You can use Drush today to set the variable, and away you go. We're not introducing a new feature really.

Hm, this is a really unusual pattern... Also, isn't it a potential issue for any 'select' element where someone sets the variable to a value that the user interface doesn't allow?

Given that it's easy to fix using hook_form_alter() (without much more effort than using Drush to set a variable in the first place, you can pretty easily add an allowed value to the #options list using hook_form_alter()) I'm not sure we should commit this.

But maybe a separate issue looking at how to handle this scenario in the form API more generally would be a good idea?