Skip to content

points: preserve partially null ordinates - #37

Merged
Alexandre-T merged 2 commits into
mainfrom
36-allow-partially-null-point-ordinates
Oct 28, 2026
Merged

Alexandre-T merged 2 commits into
mainfrom
36-allow-partially-null-point-ordinates

Conversation

@Alexandre-T

@Alexandre-T Alexandre-T commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

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

  • All acceptance criteria are implemented.
  • All 72 null/supplied combinations are covered explicitly across four dimensions and both families, through constructors, indexed factories and immutable replacement.
  • Accessors, coordinate arrays, EMPTY, zero values, dimensions, families, SRIDs and preservation of original values are verified.
  • Shared LineString, MultiPoint, Polygon and CompoundCurve paths are covered; obsolete rejection tests are replaced. CompoundCurve tests cover LineString and CircularString components in all four layouts and both families, including array access, reference copies and EMPTY point members.
  • No dependency on or automatic invocation of spatial-validator is introduced.

Validation

  • Relevant tests pass.
  • Repository quality checks pass.
  • Regression coverage was added.

Commands run

composer test-local
composer quality
composer validate --strict
git diff --check

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

  • No normative spatial validity rule is changed.

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

  • Updated point construction, immutable coordinates, aggregate tuple descriptions and explicit validation guidance in docs/.

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.

Allow nullable coordinate values across constructors, factories and immutable replacements while retaining declared dimensions and EMPTY semantics.

Refs #36
@codecov-commenter

codecov-commenter commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.40%. Comparing base (04465cc) to head (084a6aa).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor Author

CHANGES REQUIRED

Reviewed commit: 2cb4af65ee8451cf5b784a4bcd0de81ad079992a, against Story #36, parent Epic #23, the validation-removal boundary in #33, repository AGENTS.md, and the shared governance pre-merge Definition of Done.

BLOCKING — residual null-ordinate rejection in CompoundCurve

Location: lib/Types/AbstractCompoundCurve.php:163–164.

componentCoordinates() still rejects every ordinate that is neither an integer nor a float. It therefore rejects PHP null in a component point, even though the component can now preserve that ordinate and the updated PHPDoc and documentation explicitly allow numeric or null tuples.

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 null alongside numeric ordinates in this shared helper while retaining tuple/member typing checks, and add explicit regression coverage for compound components containing partially null points, including preservation through array access and immutable copying. Verify the shared path for both families and the relevant coordinate layouts.

Other review results

The point constructors, indexed hydration, point factories, Value\Coordinates, and withCoordinates() otherwise implement the requested null/supplied representation contract, including zero, declared dimensions, SRID/family preservation, immutability and all-null EMPTY behavior. The obsolete rejection tests are appropriately replaced, and the explicit point-combination tests cover all four layouts and both families.

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

  • composer test-local: passed, 653 tests / 5,404 assertions.
  • PHP CS Fixer with the repository configuration, --dry-run --sequential: passed, no changes needed.
  • Configured PHPStan level 9, PHPMD for source and tests, and PHP_CodeSniffer: passed.
  • composer validate --strict and git diff --check origin/main...HEAD: passed.
  • GitHub PHP 8.4, PHP 8.5 and GitGuardian checks: all successful on the reviewed commit.

The aggregate local composer quality command initially hit a sandbox TCP-listener restriction in parallel CS Fixer; its configured tools were then verified individually, with CS Fixer in sequential inspection mode. PDepend emits PHP 8.5 deprecations without failing its checks. The independent runtime reproduction above exposes a gap not covered by the passing suite.

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 Alexandre-T left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 Alexandre-T left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 PHP null alongside 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(), legacy withSrid(), 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.

@Alexandre-T
Alexandre-T merged commit d0d711d into main Oct 28, 2026
3 checks passed
@Alexandre-T
Alexandre-T deleted the 36-allow-partially-null-point-ordinates branch October 28, 2026 14:52
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.

Story: Allow partially null point ordinates

2 participants