fix(physical-exam): preserve persisted diagnosis ordering on save

[pradhankukiran]

Short description of what this changes or resolves:

Fixes #12029.
Fixes #12030.

The Physical Exam diagnosis editor rendered existing row order values from the row loop counter instead of the persisted ordering column. Opening the editor and saving without changes could rewrite custom orderings like 10, 20, 30 to 1, 2, 3.

Changes proposed in this pull request:

• Build one existing-plus-blank diagnosis row list before rendering.
• Render existing and blank rows through one foreach instead of two shared-counter loops.
• Preserve persisted ordering values for existing rows while keeping 5 blank rows numbered after them.
• Move the diagnosis save loop into a small helper so the regression test can exercise the same save path as the page.
• Keep row markup consistent, including maxlength attributes and escaped values.
• Add a regression test that renders the editor, feeds the rendered values through the save helper, and verifies persisted ordering survives the save round trip.
• Remove the drained PHPStan baseline entries and lower the variable.undefined cap.

Validation:

• php -l interface/forms/physical_exam/edit_diagnoses.php
• php -l interface/forms/physical_exam/diagnosis_helpers.php
• php -l tests/Tests/Services/Forms/PhysicalExam/EditDiagnosesTest.php
• composer phpcs -- --standard=phpcs.xml.dist interface/forms/physical_exam/edit_diagnoses.php interface/forms/physical_exam/diagnosis_helpers.php tests/Tests/Services/Forms/PhysicalExam/EditDiagnosesTest.php
• vendor/bin/phpstan analyze --memory-limit=4G --configuration=phpstan.neon.dist interface/forms/physical_exam/edit_diagnoses.php interface/forms/physical_exam/diagnosis_helpers.php tests/Tests/Services/Forms/PhysicalExam/EditDiagnosesTest.php
• vendor/bin/phpunit -c phpunit.xml --testsuite services --filter EditDiagnosesTest
• composer phpunit-isolated -- --filter FatalBaselineCapsIsolatedTest

Was an AI assistant used? Yes

Codex assisted with issue review, implementation, and validation.

[codecov]

Codecov Report

❌ Patch coverage is 91.96429% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.92%. Comparing base (bc08701) to head (f158ae8).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
interface/forms/physical_exam/edit_diagnoses.php	75.00%	6 Missing ⚠️
.../Services/Forms/PhysicalExam/EditDiagnosesTest.php	97.40%	2 Missing ⚠️
...nterface/forms/physical_exam/diagnosis_helpers.php	90.90%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12133      +/-   ##
============================================
- Coverage     26.13%   23.92%   -2.22%     
- Complexity    84131    84146      +15     
============================================
  Files          3935     3937       +2     
  Lines        417566   417408     -158     
============================================
- Hits         109130    99856    -9274     
- Misses       308436   317552    +9116     

Flag	Coverage Δ
apache	15.49% <91.96%> (-0.06%)	⬇️
api	3.73% <0.00%> (-0.07%)	⬇️
api-tests	3.73% <0.00%> (-0.09%)	⬇️
e2e	9.08% <0.00%> (-0.32%)	⬇️
e2e-tests	9.08% <0.00%> (-0.31%)	⬇️
email	0.18% <0.00%> (-0.01%)	⬇️
http	?
inferno	?
isolated-php8.2	7.60% <0.00%> (-0.01%)	⬇️
isolated-php8.3	7.60% <0.00%> (-0.01%)	⬇️
isolated-php8.4	7.60% <0.00%> (-0.01%)	⬇️
isolated-php8.5	7.60% <0.00%> (-0.01%)	⬇️
isolated-php8.6	7.60% <0.00%> (-0.01%)	⬇️
mariadb10.11.18	?
mariadb10.6.27	?
mariadb11.4.12	?
mariadb11.8.8	15.49% <91.96%> (-0.29%)	⬇️
mariadb12.2.2	?
mysql8.0.46	?
mysql8.4.9	?
mysql9.3.0	?
nginx	?
php8.2	15.49% <91.96%> (-0.26%)	⬇️
php8.3	?
php8.4	?
php8.5	?
phpunit	?
services	5.24% <91.96%> (+0.05%)	⬆️
upgrade	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kojiromike]

The ordering fix is straightforward and correct: existing rows now read ordering from the persisted row instead of being overwritten with the loop counter, and the loop unification preserves render output for the existing-row case while keeping the five trailing blanks numbered sequentially.

A couple of items worth resolving before merge:

• The PR title is refactor(physical-exam): ..., but this PR closes #12030, which is a behavior bug (re-saving rewrites custom ordering). Conventional Commits would expect fix( here — a refactor by definition doesn't change behavior, and a release note generated from this title would not mention the ordering bug at all. Suggest retitling to something like fix(physical-exam): preserve persisted diagnosis ordering on save and noting the rendering unification in the body.
• No tests are added or updated. The pre-existing tree has no coverage of edit_diagnoses.php at all (rendering or save handler), so this PR isn't making things worse, but the regression that #12030 reports — open editor, save without changes, ordering values get rewritten — is exactly the kind of round-trip that a small integration test against the save handler + a re-render would catch and lock in. Worth at least acknowledging in the PR description whether you considered adding coverage, given the regression is silent and would only surface after the next save.

One inline question on the new render block.

[kojiromike]

What case is the is_scalar($orderingValue) guard defending against? sqlFetchArray on SELECT ordering, diagnosis returns string|null per column (the ?? above already handles null), and the schema for form_physical_exam_diagnoses.ordering is a small integer column, so $orderingValue is string|int by the time we reach this line. If this is here purely to silence PHPStan rather than to guard a real runtime case, a (string) ($drow['ordering'] ?? $i) would say the same thing more directly. Same question on line 117 for diagnosis.

[pradhankukiran]

Addressed in d4ecc27. I replaced the render-time is_scalar() guards with direct casts and added a narrow PHPStan type assertion where the sqlFetchArray() rows are collected, so the runtime path stays direct while static analysis still knows the selected column types.

[pradhankukiran]

Thanks for the review. Addressed in d4ecc27: retitled the PR to fix(physical-exam): preserve persisted diagnosis ordering on save, simplified the render casts per the inline comment, and added a services-suite regression test that seeds physical exam diagnoses and verifies the editor renders persisted ordering values. I also updated the PR description with the new coverage/validation notes.

[kojiromike]

This cap is causing the PHP 8.6 - Isolated Tests job to fail: the test asserts assertSame($cap, $count), and on PHP 8.6 the actual entry count for variable.undefined.php is now 545, not 546. The diff removed one $ignoreErrors[] entry (the physical_exam/edit_diagnoses.php one with count => 4), and the test counts entries by preg_match_all('/\$ignoreErrors\[\] = \[/') — so the entry count drops by exactly one regardless of how many occurrences that entry covered.

Options:

1. Drop the cap to 545 to match what PHP 8.6 reports. If older PHP versions still report 546, this PR would need to wait until the corresponding baseline regeneration on those versions also drops the entry.
2. Re-run composer phpstan-baseline on PHP 8.6 specifically and use whatever value all required matrix versions agree on.

Whichever path, this needs to land before the PR can merge — assertSame will not accept count < cap.

[pradhankukiran]

Done. I rebased the PR branch onto current upstream/master so the local baseline matches the merge result, kept variable.undefined.php at 545, and reran composer phpunit-isolated -- --filter FatalBaselineCapsIsolatedTest successfully (OK (20 tests, 20 assertions)).

[kojiromike]

Minor: since $drow['ordering'] is typed as int|string|null and is then cast to (string), the ?? $i fallback fires only when the column is null. The persisted schema stores ordering as int NOT NULL DEFAULT 0, so for any row read from form_physical_exam_diagnoses the value should be non-null and the fallback is dead. Worth either dropping the ?? $i (it can't trigger for existing rows, and blank rows are synthesized with null precisely so this fallback fires) or adding a brief comment that the fallback only applies to the synthetic blank rows from array_pad. As-is, a reader has to trace $diagnosisRows back up to see why a non-null DB column needs a null guard.

[pradhankukiran]

Done. Added a brief comment above the fallback clarifying that it is for the synthetic blank rows created by array_pad, not for persisted diagnosis rows.

[kojiromike]

This is good render-side coverage, but #12029 is reported as a save-time data-loss bug ("open editor, save without changes, ordering rewritten to 1..N"). The save loop lives at lines 47-65 of edit_diagnoses.php and reads $_POST['form_ordering'] / $_POST['form_diagnosis']. A test that POSTs the form values back (the same values the editor just rendered) and re-reads form_physical_exam_diagnoses would close the loop the issue actually describes. As written, this test would also pass if the save handler silently dropped the ordering column — the render is correct, but the round-trip is what the bug was about.

[pradhankukiran]

Done. The regression test now renders the editor, extracts the rendered form_diagnosis and form_ordering values, sends those through the same save helper used by the POST handler, and re-reads the table to verify the persisted 10/20/30 ordering survives the save round trip. The focused services test passes locally (OK (1 test, 6 assertions)).

[kojiromike]

This new file is missing declare(strict_types=1). The repo's CLAUDE.md requires it on every new PHP file ("Every new PHP file starts with declare(strict_types=1)"), and the PR's own test file already has it. It matters here specifically: save() casts both $ordering and $diagnosis with (string) and writes $ordering into an int column. Without strict types, the surrounding loose coercion is exactly the silent-data-corruption class the rule guards against.

Suggested change

	<?php
	<?php
	declare(strict_types=1);