[WIP] Fix Result Number field not respecting Start Number configuration #2434
Conversation
The Result Number (sequence) field was always starting at 0 instead of using the configured "Start Number" value in View settings. This only affected field display in Views - the {sequence} merge tag worked correctly.
Changes:
- Add configuration reading in get_sequence() method to load start/reverse settings from View
- Refactor get_sequence() into smaller, testable methods
- Add comprehensive test coverage including filters, pagination, and single entry views
- Consolidate scattered sequence tests into single test file
- Fix single entry sequence calculation to respect filters and sort order
Fixes #2431
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
WalkthroughRewrites the Sequence field to support single-entry positioning via captured unpaginated SQL, paginated multi-entry sequencing with per-context caching, honoring field "Start" and "Reverse" settings, and adds helpers and tests; updates changelog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant View as View Renderer
participant Field as Sequence Field
participant Cache as Context Cache
participant GFQ as GF Query Builder / DB
participant Coll as Entry Collection
User->>View: Request page or single-entry
View->>Field: get_sequence(context)
alt Single-entry view (position by full result set)
Field->>GFQ: Hook & capture unpaginated SQL (capture_view_sql_query)
GFQ-->>Coll: Execute full IDs query
Field->>Coll: find_entry_position(entry_id)
Field-->>View: compute number (start ± position, reverse applied)
else Multi-entry view (paginated / streaming)
Field->>Cache: get_or_init_start(context) (calculate_starting_number)
Cache-->>Field: current start value
Field->>Cache: update cached counter (get_next_sequence_number)
Field-->>View: return filtered sequence value (gravityview/field/sequence/value)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
tests/unit-tests/GravityView_Field_Sequence_Test.php (3)
97-105: Avoid assigning non-array values to $_GET
$_GET = 0;can cause unexpected behavior outside this test (e.g., Utils::_GET expects an array-like). Prefer clearing the specific key or resetting to an array.Apply this diff:
- $_GET = 0; + unset( $_GET['pagenum'] ); + // Or if you intend to fully reset test query args: + $_GET = [];
106-113: Nit: unused loop variable creates static analysis noise
foreach ( range( 1, 10 ) as $_ )triggers PHPMD UnusedLocalVariable. Either switch to a for loop or add an inline suppression.Example minimal change:
- foreach ( range( 1, 10 ) as $_ ) { + for ( $i = 0; $i < 10; $i++ ) { \GV\GF_Entry::from_entry( $this->factory->entry->create_and_get( [ 'form_id' => $form['id'], 'status' => 'active', ] ) ); - } + }Alternatively, add a method-level docblock
@SuppressWarnings(PHPMD.UnusedLocalVariable).
579-668: Filtered single-entry test is great; add negative assertion comment / suppression for “unused” entriesEntries intentionally created to be filtered out (
$entry_2,$entry_4) are unused variables from a static analysis perspective. Either reference them in a negative assertion (e.g., verify they yield 0 if resolved) or suppress PHPMD warnings for the method.includes/fields/class-gravityview-field-sequence.php (4)
136-157: Grammar fix in log: “used outside of”Minor but user-facing in logs: “was used without outside of the GravityView entry loop” reads awkwardly.
Apply this diff:
- gravityview()->log->error( '{sequence} Merge Tag was used without outside of the GravityView entry loop.', array( 'data' => $matches ) ); + gravityview()->log->error( '{sequence} Merge Tag was used outside of the GravityView entry loop.', array( 'data' => $matches ) );
223-231: Docblocks with “@SInCE TODO” should be finalized before releaseThere are multiple “@SInCE TODO” placeholders. Please replace with the correct version before shipping.
408-411: Performance consideration: selecting only IDs would reduce memory for large result sets
$wpdb->get_results( implode( ' ', $sql_query ), ARRAY_A )fetches all columns. If possible, adjust the capturedSELECTto only include the entry ID to minimize memory pressure on large Views. Alternatively, add a filter to let integrators switch the projection.
367-397: Improve Resilience of SQL Capture & OverrideCapturing and manipulating the raw SQL generated by Gravity Forms via the
gform_gf_query_sqlfilter works today but relies on internal array keys (paginate,order) and thet1table alias—both of which could change in future GF releases or custom query contexts. To reduce breakage risk and enable rapid rollback:
- Gate the custom
ORDER BYoverride behind a filter so it can be turned off without code changes.- Emit the full captured
$sql_queryto the debug log when debugging is enabled, for faster diagnosis if the shape changes.- Where possible, use the GravityView/GF entry API (e.g. fetch all entry IDs via API parameters) instead of rebuilding raw SQL manually.
Suggested patch to add a filter-based “escape hatch” and leave room for debug logging:
// Remove pagination to get all results unset( $sql_query['paginate'] ); - // For single entry views with a custom start value, sort by ID ASC to get - // consistent sequence numbers based on creation order - // Only override if start value is different from default (1) - if ( $this->is_single_entry_view( $context ) && $context->field->start !== 1 ) { - $sql_query['order'] = 'ORDER BY `t1`.`id` ASC'; - } + // Optionally force an ASC order on single-entry views with a custom start + $force_asc = apply_filters( + 'gk/gravityview/field/sequence/single/force_asc_on_custom_start', + true, + $context, + $sql_query + ); + if ( $force_asc && $this->is_single_entry_view( $context ) && $context->field->start !== 1 ) { + $sql_query['order'] = 'ORDER BY `t1`.`id` ASC'; + } + + /** + * Log the full SQL query shape when debugging is enabled. + * Developers can inspect changes to array keys or table aliases. + */ + if ( defined( 'WP_DEBUG' ) && WP_DEBUG ) { + error_log( '[GravityView] Captured SQL Query: ' . print_r( $sql_query, true ) ); + }
- File:
includes/fields/class-gravityview-field-sequence.php- Method:
capture_view_sql_query()(lines 367–397)Optional: In high-risk environments, consider replacing raw SQL capture entirely with the View’s entry API (
get_entries()plus API parameters to load all IDs) to avoid dependency on internal SQL structures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
includes/fields/class-gravityview-field-sequence.php(1 hunks)readme.txt(1 hunks)tests/unit-tests/GravityView_Field_Sequence_Test.php(1 hunks)tests/unit-tests/GravityView_Field_Test.php(0 hunks)tests/unit-tests/GravityView_Future_Test.php(0 hunks)
💤 Files with no reviewable changes (2)
- tests/unit-tests/GravityView_Future_Test.php
- tests/unit-tests/GravityView_Field_Test.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit-tests/GravityView_Field_Sequence_Test.php (5)
tests/GV_UnitTestCase.php (1)
GV_UnitTestCase(3-80)includes/fields/class-gravityview-fields.php (1)
GravityView_Fields(14-168)includes/fields/class-gravityview-field-sequence.php (1)
get_sequence(234-257)future/includes/class-gv-collection-field.php (1)
by_visible(97-107)future/includes/class-gv-view.php (1)
_flush_cache(859-865)
includes/fields/class-gravityview-field-sequence.php (3)
future/includes/class-gv-view.php (1)
get_anchor_id(1868-1877)future/includes/class-gv-utils.php (2)
Utils(12-193)_GET(21-23)future/includes/class-gv-collection-entry.php (1)
total(104-121)
🪛 PHPMD (2.15.0)
tests/unit-tests/GravityView_Field_Sequence_Test.php
107-107: Avoid unused local variables such as '$_'. (Unused Code Rules)
(UnusedLocalVariable)
182-182: Avoid unused local variables such as '$_'. (Unused Code Rules)
(UnusedLocalVariable)
592-592: Avoid unused local variables such as '$entry_2'. (Unused Code Rules)
(UnusedLocalVariable)
598-598: Avoid unused local variables such as '$entry_3'. (Unused Code Rules)
(UnusedLocalVariable)
604-604: Avoid unused local variables such as '$entry_4'. (Unused Code Rules)
(UnusedLocalVariable)
687-687: Avoid unused local variables such as '$entry_2'. (Unused Code Rules)
(UnusedLocalVariable)
1085-1085: Avoid unused parameters such as '$content'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test_and_package
🔇 Additional comments (16)
readme.txt (1)
37-37: Changelog entry is clear and scoped to the underlying bugNice, this line concisely communicates the user-visible fix for the Result Number field respecting “Start Number” in Views. No action needed.
tests/unit-tests/GravityView_Field_Sequence_Test.php (12)
36-39: Good: centralizing a reusable Sequence field instanceFetching the registered 'sequence' field in setUp() keeps tests DRY and consistent.
144-197: Good: deterministic single-entry tests with cache disabledDisabling both GV and GF caches here is the right move to ensure consistent, deterministic results while reconstructing full result sets.
205-250: Solid coverage: field-configured 'start' respected for multi-entry viewsThis exercises the core regression. Consider adding one extra assertion for a second page (set
$_GET['pagenum']=2) to ensure paging + configured start keeps working.
256-296: Good coverage for negative and zero startsThese tests are valuable edge cases. No changes required.
348-415: Great scenario: reverse + custom start with total/partial pagesGood to see both
entries->all()andentries->total()asserted. One suggestion: add an assertion that the reverse sequence still honors the View sorting settings (if any) to guard against order overrides in the implementation.
500-573: Nice: single-entry with custom start verifies first and last positionsThis validates the “position-based” numbering end-to-end. No action needed.
674-750: Default sort behavior confirmed; consider a parallel test for explicit sort ASCThis test assumes default DESC order. Please add a companion test setting an explicit ASC order on the View to ensure single-entry position math tracks custom sort directions, too.
756-859: Comprehensive: multi-filter + approval + start valueGreat multi-constraint validation. Also good to see the 0 result asserted for out-of-scope entries.
865-902: Invalid start defaults to 1: exact behavior lockedClear and helpful test for resilience against bad configuration values.
909-920: Helpful helper: context reset between testsThis local reset mirrors GV test infra; good hygiene.
927-963: Merge-tag test: covers positive and negative explicit startsGood coverage; no change needed.
970-1067: End-to-end merge-tag rendering coverage is excellentThorough validation across fields, custom classes, and widgets (including the “not working in widgets” expectation). Great job consolidating this into one place.
includes/fields/class-gravityview-field-sequence.php (3)
300-309: Context key choice looks goodUsing the view anchor ID and field UID is a sensible per-context cache key and should be stable within a request.
320-322: Guard for missing request in single-entry detection
$context->request && $context->request->is_entry()is fine, but if$context->requestisn’t set, this short-circuits correctly. No action needed.
536-551: Nice touch: allow filtering the final sequence valueThe
gravityview/field/sequence/valuefilter is a useful extension point for formatting or transformations.
Previously, the Result Number field would incorrectly override a "0" start value due to using empty() check, which treats "0" as empty. This improvement switches to using is_numeric() to properly preserve numeric string values including "0". - Changed condition from `empty() && !== 0` to `!is_numeric()` for better numeric string handling - Added comprehensive test coverage for numeric string start values including "0", negative numbers, and edge cases - Ensures Start Number configuration is respected for all valid numeric values 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Previously, reverse mode used GFAPI::count_entries() which ignored View filters, leading to incorrect sequence numbers. This commit makes several improvements: 1. Prioritize View's entries collection for total count in reverse mode - Respects all View filters, search criteria, and approval status - Removes misleading GFAPI fallback that counted unfiltered entries - Defaults safely to 1 if View collection unavailable 2. Optimize SQL to only select entry IDs for memory efficiency - Reduces memory usage significantly for large result sets - Changes SELECT clause from all columns to just `t1`.`id` - Particularly beneficial for Views with many entries or complex fields These changes ensure reverse sequence numbers are accurate and the implementation is more memory-efficient. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Adds extensive test coverage for the sequence field fixes: - Test that numeric string start values (especially "0") are preserved correctly - Test that reverse mode respects View filters and approval status - Test that reverse mode handles empty/filtered views gracefully - Test SQL optimization to verify only IDs are selected - Test edge cases and fallback scenarios These tests ensure the sequence field behaves correctly with: - Filtered Views - Approval-only Views - Views with no matching entries - Various start values including 0 and negative numbers - Large datasets with memory optimization 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
includes/fields/class-gravityview-field-sequence.php (1)
276-281: Addresses the "Start Number" override bug correctly (numeric checks + config read).Switching to is_numeric() preserves explicit "0" and other numeric strings; loading reverse from configuration is straightforward. This closes the root cause reported in #2431 without side effects.
Also applies to: 286-289
🧹 Nitpick comments (11)
includes/fields/class-gravityview-field-sequence.php (5)
483-489: Prefer $context->request when available to resolve totals under the same request context.Using the global request can diverge from the rendering context when multiple Views exist on a page. Fall back to the global only if the context doesn’t define one.
- $request = gravityview()->request; - if ( $request ) { + $request = isset( $context->request ) ? $context->request : gravityview()->request; + if ( $request ) { $entries_collection = $context->view->get_entries( $request );
320-322: Return a strict boolean from is_single_entry_view for clarity.You currently return the entry object (truthy) or false. Cast to bool to match the method’s contract and improve readability.
- return $context->request && $context->request->is_entry(); + return (bool) ( $context->request && $context->request->is_entry() );
266-271: Replace “@SInCE TODO” placeholders with the release version before merging.Docblocks should reflect the actual version that ships the change to keep API history accurate.
Also applies to: 294-299, 312-319, 324-336, 358-366, 404-412, 418-428, 454-466, 518-529, 546-553
340-416: Sanity checks around captured SQL array shape.
gform_gf_query_sqlgenerally yields a parts array, but defensive checks improve resilience. If a plugin alters the filter and returns a string,implode(' ', $sql_query)would behave unexpectedly.Consider guarding with:
- // Remove pagination to get all results - unset( $sql_query['paginate'] ); + // Normalize and guard against unexpected filter output + if ( ! is_array( $sql_query ) ) { + return array(); + } + unset( $sql_query['paginate'] );
412-416: Potential performance hotspot on large datasets (single-entry path).Fetching all IDs for the full result set is acceptable with the IDs-only optimization, but it’s still O(N). If needed later, consider a rank/position query (window function) when supported, or a bounded seek using the View’s sort to compute position without scanning all rows.
tests/unit-tests/GravityView_Field_Sequence_Test.php (6)
106-113: Avoid unused loop variable to appease static analysis and improve clarity.Switch to a for-loop and drop the unused
$_variable.- foreach ( range( 1, 10 ) as $_ ) { + for ( $i = 1; $i <= 10; $i++ ) { \GV\GF_Entry::from_entry( $this->factory->entry->create_and_get( [ 'form_id' => $form['id'], 'status' => 'active', ] ) ); - } + }
813-836: Remove unused local variables in filtered single-entry setup.
$entry_2,$entry_3, and$entry_4aren’t referenced; create those entries without assigning to variables.- $entry_2 = $this->factory->entry->create_and_get( [ + $this->factory->entry->create_and_get( [ 'form_id' => $form['id'], 'status' => 'active', '1' => 'Exclude', // This will be filtered out ] ); - - $entry_3 = $this->factory->entry->create_and_get( [ + $this->factory->entry->create_and_get( [ 'form_id' => $form['id'], 'status' => 'active', '1' => 'Include', ] ); - - $entry_4 = $this->factory->entry->create_and_get( [ + $this->factory->entry->create_and_get( [ 'form_id' => $form['id'], 'status' => 'active', '1' => 'Exclude', // This will be filtered out ] );Also applies to: 825-830
908-912: Remove unused$entry_2in default-sort single-entry test.- $entry_2 = $this->factory->entry->create_and_get( [ + $this->factory->entry->create_and_get( [ 'form_id' => $form['id'], 'status' => 'active', '1' => 'Hide', // This will be filtered out ] );
1135-1136: Drop unused$wpdbimport in SQL optimization test.The test uses the
queryfilter and doesn’t reference$wpdb.- global $wpdb; - $captured_query = null; + $captured_query = null;
895-971: Add a variant: single-entry + custom start + custom sort order.To prevent regressions, add a test where the View sorts by a field or created date DESC and start is non-default, asserting that the position respects the View’s sort. This will catch the potential ORDER BY override issue flagged in the PHP code review.
1429-1446: Ignore unused$contentparameter via annotation to quiet PHPMD.Signature must match the parent, so suppress the warning instead of changing it.
You can add to the method docblock:
/** * @param string $content * @noinspection PhpUnusedParameterInspection */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
includes/fields/class-gravityview-field-sequence.php(1 hunks)tests/unit-tests/GravityView_Field_Sequence_Test.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
includes/fields/class-gravityview-field-sequence.php (4)
future/includes/class-gv-view.php (1)
get_anchor_id(1868-1877)future/includes/class-gv-utils.php (1)
_GET(21-23)future/includes/class-gv-collection-entry.php (1)
total(104-121)future/includes/class-gv-collection.php (1)
count(97-99)
tests/unit-tests/GravityView_Field_Sequence_Test.php (6)
tests/GV_UnitTestCase.php (1)
GV_UnitTestCase(3-80)tests/unit-tests/GravityView_Future_Test.php (2)
setUp(13-17)_reset_context(26-40)includes/fields/class-gravityview-fields.php (1)
GravityView_Fields(14-168)future/includes/class-gv-field-internal.php (1)
Internal_Field(14-139)future/includes/class-gv-utils.php (2)
_GET(21-23)Utils(12-193)future/includes/class-gv-view.php (1)
_flush_cache(859-865)
🪛 PHPMD (2.15.0)
tests/unit-tests/GravityView_Field_Sequence_Test.php
107-107: Avoid unused local variables such as '$_'. (Unused Code Rules)
(UnusedLocalVariable)
813-813: Avoid unused local variables such as '$entry_2'. (Unused Code Rules)
(UnusedLocalVariable)
819-819: Avoid unused local variables such as '$entry_3'. (Unused Code Rules)
(UnusedLocalVariable)
825-825: Avoid unused local variables such as '$entry_4'. (Unused Code Rules)
(UnusedLocalVariable)
908-908: Avoid unused local variables such as '$entry_2'. (Unused Code Rules)
(UnusedLocalVariable)
1135-1135: Avoid unused local variables such as '$wpdb'. (Unused Code Rules)
(UnusedLocalVariable)
1439-1439: Avoid unused parameters such as '$content'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test_and_package
🔇 Additional comments (1)
tests/unit-tests/GravityView_Field_Sequence_Test.php (1)
349-416: Great coverage for reverse + custom start, including total() vs all().The assertions around
total()and per-page counts validate the reverse-start logic well. Nicely done.
| // For Single Entry context with a custom start value, sort by ID ASC to get | ||
| // consistent sequence numbers based on creation order | ||
| // Only override if start value is different from default (1) | ||
| if ( $this->is_single_entry_view( $context ) && $context->field->start !== 1 ) { | ||
| $sql_query['order'] = 'ORDER BY `t1`.`id` ASC'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t override the View’s sort order for single-entry with custom start; it breaks correctness for custom-sorted Views.
For single-entry Views, forcing ORDER BY \t1`.`id` ASCwheneverstart !== 1` ignores the View’s configured sort (e.g., date DESC, custom field, multiple columns). That will yield incorrect positions and therefore incorrect sequence numbers. Preserve the captured order unless it’s empty.
Apply this diff:
- // For Single Entry context with a custom start value, sort by ID ASC to get
- // consistent sequence numbers based on creation order
- // Only override if start value is different from default (1)
- if ( $this->is_single_entry_view( $context ) && $context->field->start !== 1 ) {
- $sql_query['order'] = 'ORDER BY `t1`.`id` ASC';
- }
+ // Preserve the View's configured sort order. Only provide a default if none exists.
+ if ( $this->is_single_entry_view( $context ) && $context->field->start !== 1 && empty( $sql_query['order'] ) ) {
+ $sql_query['order'] = 'ORDER BY `t1`.`id` ASC';
+ }Follow-up: consider adding a unit test “single_entry_with_custom_start_and_custom_sort” to assert sequences reflect the View’s sort field and direction, not creation order.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For Single Entry context with a custom start value, sort by ID ASC to get | |
| // consistent sequence numbers based on creation order | |
| // Only override if start value is different from default (1) | |
| if ( $this->is_single_entry_view( $context ) && $context->field->start !== 1 ) { | |
| $sql_query['order'] = 'ORDER BY `t1`.`id` ASC'; | |
| } | |
| // Preserve the View's configured sort order. Only provide a default if none exists. | |
| if ( $this->is_single_entry_view( $context ) | |
| && $context->field->start !== 1 | |
| && empty( $sql_query['order'] ) ) { | |
| $sql_query['order'] = 'ORDER BY `t1`.`id` ASC'; | |
| } |
zackkatz
changed the title
@zackkatz is this really supposed to work for the single entry page? I get a database error (see image below) when I add the Result Number field to the Single Entry layout. And it definitely doesn't respect the "First Number in the Sequence" setting. 
|
Summary
What Changed
get_sequence()method to read field configuration from View settingsTest Plan
@Mwalek Please let me know if you have any questions.
{sequence}merge tags continue to work correctlyFixes #2431
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
💾 Build file (766157e).