Define comment_per_page_options as config [#1830966]

You can configure for each contenttype how many comments shall be displayed per page. The array of options is hardcoded into comment.module. We had several times the use case that we wanted a number of comments that wasn't in the available choices. You can change these options using hook_form_alter() but I think it would be better to transfer this hardcoded array into config.

Comment	File	Size	Author
#3	1830966.png	61.77 KB	penyaskito
#1	drupal-make-comment-per-page-options-config-1830966-1.patch	891 bytes	leschekfm
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

leschekfm CreditAttribution: leschekfm commented 3 November 2012 at 13:01

Issue tags:

+Configuration system

File	Size
drupal-make-comment-per-page-options-config-1830966-1.patch	891 bytes

The attached patch allows to change these options.

Please review

Comment #2

leschekfm CreditAttribution: leschekfm commented 3 November 2012 at 13:01

Status:

Active

» Needs review

Comment #3

penyaskito

he/him

Spanish

🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺

CreditAttribution: penyaskito commented 4 November 2012 at 10:52

Status:

Needs review

» Reviewed & tested by the community

File	Size
1830966.png	61.77 KB

Tested it and looks fine.

Comment #4

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 5 November 2012 at 21:57

Comment #5

catch

he/him

English

CreditAttribution: catch commented 9 November 2012 at 22:33

Status:

Reviewed & tested by the community

» Needs review

I don't understand why hook_form_alter() isn't sufficient here.

Comment #6

leschekfm CreditAttribution: leschekfm commented 11 November 2012 at 08:06

In my opinion these options were randomly chosen by the original developer, because there is no system that would explain why exactly these were chosen. Therefore I think this is hardcoded configuration at the moment and should be converted to the config system.

Another reason to convert this would be that is much more easy to edit only a config file than to create a new module just to override some settings.

This issue could also be bypassed altogether by replacing the select box with a normal text input.

Comment #7

gdd

he/him

English

Portland, OR

CreditAttribution: gdd commented 11 November 2012 at 19:55

I don't think hardcoded config has any place in the config system, it is only user-editable config that makes sense there. I agree with catch that this could just as easily be modified via a form alter. If it became a text input field then configuration would make more sense.

Comment #8

leschekfm CreditAttribution: leschekfm commented 11 November 2012 at 21:12

I don't think hardcoded config has any place in the config system, it is only user-editable config that makes sense there.

That's my point ;) I think this should be user configurable. I would compare this to the options you can define for a list field, which is clearly config. Maybe your concern is that there is no UI for editing the list of comment options at the moment?

If it became a text input field then configuration would make more sense.

Could you explain that? If the input element would be converted to a text field there wouldn't be a need to store any config for the input element as a text field has no predefined list of options.

Comment #9

Xen CreditAttribution: Xen commented 1 July 2013 at 20:01

@leschekfm

Well, it's sorta meta-configuration. If it should make sense to have it as config, there should also be a admin page to configure it. Having an admin setting to set the possible options in another admin setting is clearly overkill. So while I can see why it would be handy, it is a bit of misuse of the configuration system.

I think the argument should really be about whether we should grit our teeth and carry on with using form_alter, change the setting to a textfield (not UX sexy), or create a new fancy widget that combines the dropdown with a textfield (much like the windows combo field), if someone steps up to implement that.