Repository navigation
points: preserve partially null ordinates - #37
Conversation
Allow nullable coordinate values across constructors, factories and immutable replacements while retaining declared dimensions and EMPTY semantics. Refs #36
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
============================================
+ Coverage 98.90% 99.40% +0.50%
+ Complexity 727 703 -24
============================================
Files 133 132 -1
Lines 1550 1525 -25
============================================
- Hits 1533 1516 -17
+ Misses 17 9 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
CHANGES REQUIREDReviewed commit: BLOCKING — residual null-ordinate rejection in CompoundCurveLocation:
Reproduced independently: use LongitudeOne\SpatialTypes\Types\Dimension2\Geometry\CompoundCurve;
use LongitudeOne\SpatialTypes\Types\Dimension2\Geometry\LineString;
$line = new LineString([[0, null], [1, 2]], 4326);
assert([[0, null], [1, 2]] === $line->toArray());
new CompoundCurve([$line], 4326);
// InvalidValueException:
// A compound curve component must expose numeric ordinates.This violates #36's acceptance criterion requiring removal of all coordinate-completeness checks, including shared paths, and its source-review Definition of Done. The same helper is called during construction, array access and immutable spatial-reference copies. Updating only its return annotation does not change the rejection. Required correction: allow PHP Other review resultsThe point constructors, indexed hydration, point factories, No dependency change, automatic validator invocation, unrelated migration, or additional unresolved API/architecture gate was found. Representation acceptance is correctly distinguished from geometric conformity; this review does not replace the normative conformance work in #26. Independent validation
The aggregate local The pre-merge Definition of Done is not satisfied until the blocking shared-path rejection and its regression coverage are corrected. No merge or Done transition performed. Publication note: GitHub rejected a separate review submission because this account already has a pending review. This independent review is therefore published as a PR conversation comment; the existing pending review is preserved. |
Alexandre-T
left a comment
There was a problem hiding this comment.
The exception message has to be updated.
| { | ||
| if (!is_float($coordinate) && !is_int($coordinate)) { | ||
| if (null !== $coordinate && !is_float($coordinate) && !is_int($coordinate)) { | ||
| throw new InvalidValueException(sprintf('The %s coordinate must be a number.', $name)); |
There was a problem hiding this comment.
The %s coordinate must be a number.
Accept null coordinates throughout compound-curve construction and reference copies, retain technical rejection checks, and clarify nullable coordinate diagnostics. Refs #36
Alexandre-T
left a comment
There was a problem hiding this comment.
READY FOR MERGE
Independently re-reviewed commit 084a6aa6551e89fffab99a166f7ac0cea1af7725 against Story #36, the changes since 2cb4af6, current PR comments and review threads, repository instructions, and the shared governance pre-merge Definition of Done.
This verdict supersedes my previous CHANGES REQUIRED review for the current revision.
Findings resolved
- CompoundCurve null-ordinate rejection:
componentCoordinates()now preserves PHPnullalongside integers and floats. My original failing construction now succeeds, and its immutable reference copy preserves the full nested arrays and original SRID. - Regression coverage: explicit tests exercise LineString and CircularString components in XY, XYZ, XYM and XYZM for both Geometry and Geography, including zero, null positions, concrete types, dimensions, families,
toArray(),withSpatialReference(), legacywithSrid(), independent copies and an EMPTY point member. Boolean and numeric-string component ordinates remain rejected. - Coordinate diagnostic: the hydrator now reports “number or null” for unsupported Z/M input. Both diagnostics have regression coverage. The earlier inline diagnostic comment is addressed in code, although its outdated GitHub thread remains open.
The overall change satisfies the acceptance criteria: all point null/supplied combinations are representable, partial points remain non-EMPTY, all-null points retain EMPTY semantics, declared layouts and structural typing are preserved, and the affected shared paths contain no residual completeness rejection identified by this review. Documentation matches the resulting behavior.
No blocking finding, additional unresolved gate, dependency change or scope expansion remains. This is the representation contract approved in #36, not a claim of geometric conformity; the separate normative review in #26 remains outside this PR's completion criteria.
Independent validation on this revision
composer test-local: 665 tests, 5,587 assertions — passed.- Repository-configured PHP CS Fixer (
--dry-run --sequential): passed, no formatting changes required. - Repository-configured PHPStan level 9, PHPMD for source and tests, and PHP_CodeSniffer: all passed.
git diff --check origin/main...HEAD: passed; working tree clean.- GitHub PHP 8.4, PHP 8.5 and GitGuardian checks: all successful on the reviewed SHA.
The configured quality tools were run individually for this independent review; sequential CS Fixer avoids the local sandbox TCP-listener limitation encountered previously. PDepend still emits PHP 8.5 deprecations without failing the checks.
All applicable pre-merge Definition of Done requirements are satisfied. Merge remains Alexandre's decision; this review neither merges the PR nor declares Story #36 Done.
Closes #36
Summary
Points now preserve every combination of supplied and PHP-null ordinates in XY, XYZ, XYM and XYZM, for both Geometry and Geography. For example, an XY point with X = 0 and Y = null is representable and is not EMPTY. Only all-null points are EMPTY.
Remove coordinate-completeness rejection from point constructors, shared initialization, coordinate values, indexed-array hydration and internal point factories. Preserve structural typing, tuple indices, declared dimensions, family and spatial reference. Preserve numeric or null ordinates through CompoundCurve components and immutable reference copies. Keep technical rejection of unsupported ordinate types, and make coordinate diagnostics explicitly mention null. Update nullable array contracts through shared aggregate paths and document the representation/validation boundary.
Acceptance criteria
Validation
Commands run
PHP 8.5.11: 665 tests, 5,587 assertions, 99.44% line coverage. Quality includes PHP CS Fixer, PHPStan level 9, PHPMD for source/tests, and PHP_CodeSniffer. The installed PDepend tooling emits PHP 8.5 deprecation notices; quality exits successfully. Remote PHP 8.4/8.5 CI remains a separate check.
Source review confirms the former completeness checks and the obsolete internal AbstractPointFactory are removed.
Standards
This implements the representation contract explicitly defined in #36. Accepting partially supplied coordinates does not establish conformity with ISO/IEC 13249-3 or OGC requirements. Coordinate-completeness validation remains an explicit consumer operation in spatial-validator; the normative review in #26 can continue against this contract.
Architecture
Compatibility
Point constructor parameters were already nullable. Public Value\Coordinates construction, accessors and replacement methods now support null values on declared axes; getZ()/getM() return null for a declared axis with a null value and still reject axes outside the dimension. Existing complete-point behavior and all-null EMPTY semantics are preserved. Consumers may now encounter partial nulls when they supply them and must explicitly validate completeness if needed. No unrelated breaking change is introduced.
Documentation
Developer handoff
Indexed arrays still require each declared index; an explicit null differs from an absent index. Coordinate values keep all declared entries, including nulls; EMPTY point arrays remain [] and getCoordinates() remains null.
No unresolved implementation question or gate was encountered. Independent re-review and required CI on the current revision are pending; this PR does not complete or merge the Story.