Hello.

In my case I need 5 comments per page. But I can't submit form because of this field have validation - number with minimum value = 10 and with step = 10.
It's really not clear to me why this "min" restriction was imposed. I also think that this will not be a pleasant thing for many users who want to change the number of comments per page for example to 15.

Steps to reproduce:

  1. Go to /admin/structure/types/manage/{CONTENT_TYPE}/fields/node.{CONTENT_TYPE}.comment
  2. In field "Comments per page" enter any number less than 10 or not divisible by 10
CommentFileSizeAuthor
#3 drupal-core-2906298-3.patch694 bytesqzmenko
#2 drupal-core-2906298-2.patch674 bytesqzmenko

Comments

qzmenko created an issue. See original summary.

qzmenko’s picture

qzmenko
Primary language Russian
Location Novosibirsk
commented
Status: Active » Needs review
StatusFileSize
new drupal-core-2906298-2.patch674 bytes

So, I suggest removing these restrictions. I could not find the reason why they were added.

qzmenko’s picture

qzmenko
Primary language Russian
Location Novosibirsk
commented
StatusFileSize
new drupal-core-2906298-3.patch694 bytes

It's necessary to leave the validation to a minimum value of 1. To prevent a value of 0 or less.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

qzmenko’s picture

qzmenko
Primary language Russian
Location Novosibirsk
commented

Any reviews?

runeasgar’s picture

runeasgar commented

Reviewing this now.

runeasgar’s picture

runeasgar commented
Status: Needs review » Reviewed & tested by the community
  1. Starting by confirming the undesirable behavior.
  2. Navigated to /admin/structure/types/manage/article/fields/node.article.comment.
  3. Confirmed. Placing 4, 5, or 6 into the "Comments per page" field results in this inline popup error: "Value must be greater than or equal to 10."
  4. Applying patch: $ curl -l https://www.drupal.org/files/issues/drupal-core-2906298-3.patch | git apply -v.
  5. Applied cleanly.
  6. drush cr, refresh.
  7. Testing putting 0 in the field: popup error, "Value must be greater than or equal to 1."
  8. Testing putting 1 in the field: success.
  9. Testing the actual functionality, by creating an article and adding 2 comments (seems like a good thing to test): success.
  10. Testing putting 5 in the field and adding 4 more comments (can never be too careful!): success

Looks good to me. Seeing as how this is not marked as a "bug", I'm presuming it doesn't need tests. Also, since it's not really a UI change, assuming it doesn't need a screenshot (and not sure what I'd screenshot, anyway).

Marking as RTBC. Thanks, @qzmenko !

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: drupal-core-2906298-3.patch, failed testing. View results

qzmenko’s picture

qzmenko
Primary language Russian
Location Novosibirsk
commented
Status: Needs work » Reviewed & tested by the community

I returned RTBC from #7, because after the re-testing it successfully passed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Discussed with @yoroy and we agreed that this is okay to go in. I would not recommend a setting of 1 or even 5 but that's not up to me - this is something that a site should be able to choose for whatever reason. Also since this is just changing FAPI structure no test is necessary.

Crediting @runeasgar for the thorough review in #7.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 7476b37 and pushed to 8.6.x. Thanks!

  • alexpott committed 7476b37 on 8.6.x 
    Issue #2906298 by qzmenko, runeasgar: Comments per page number...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.