Skip to content

Conversation

garak
Copy link
Collaborator

@garak garak commented May 19, 2025

Fix #832

@garak garak requested a review from Copilot May 19, 2025 07:54
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

A concise update to the configuration to allow the "page_limit" setting to accept null values, while ensuring non-null values are valid integers.

  • Changed the "page_limit" node from integerNode to scalarNode.
  • Added a validation block to enforce that non-null values are integers.
  • Commented out normalization code remains, which could be removed if no longer needed.

Comment on lines 58 to 61
#->beforeNormalization()
#->ifNull()
#->then(static fn (mixed $v): int => 5)
#->end()
Copy link
Preview

Copilot AI May 19, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider removing or finalizing the commented-out normalization block if it is no longer needed to improve code clarity.

Suggested change
#->beforeNormalization()
#->ifNull()
#->then(static fn (mixed $v): int => 5)
#->end()

Copilot uses AI. Check for mistakes.

@garak garak force-pushed the fix-nullable-config branch from 7c1b5ab to 56e02f6 Compare May 19, 2025 07:58
@garak garak requested a review from Copilot May 19, 2025 07:59
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses issue #832 by updating the configuration for "page_limit" so that it accepts null values. The changes include switching from an integer node to a scalar node, adding validation for integer or null values, and introducing a normalization step for null values.

->end()
->beforeNormalization()
->ifNull()
->then(static fn (mixed $v): int => 5)
Copy link
Preview

Copilot AI May 19, 2025

Choose a reason for hiding this comment

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

The normalization step converts an explicit null value to 5, which may override a user's intention to provide a null. Consider preserving null if the goal is to allow 'page_limit' to be explicitly set to null.

Copilot uses AI. Check for mistakes.

@garak garak requested a review from Copilot May 19, 2025 08:09
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes issue #832 by allowing the page_limit configuration to be explicitly set to null while still validating integer values.

  • Changed page_limit node from integerNode to scalarNode with defaultNull and custom validation.
  • Added a validation rule to ensure values are either null or an integer.

@garak garak force-pushed the fix-nullable-config branch from 661cf16 to ae78849 Compare May 19, 2025 08:15
@garak garak merged commit 038c50c into KnpLabs:master May 20, 2025
15 checks passed
@garak garak deleted the fix-nullable-config branch May 20, 2025 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid type for path "knp_paginator.page_limit". Expected "int", but got "null".
1 participant