[Alexandre-T]

Pull Request

Closes #45

Summary

Consumers can now edit MultiPoint, MultiLineString, MultiPolygon and SpatialCollection in place in XY, XYZ, XYM and XYZM. Supplied mutable members keep their identity, so edits and propagated context changes remain visible through every owner referencing those instances.

All four kinds expose addElement(), insertElement(), removeElement(), setElement(), setElements(), getElement() and getElements(). Multi-object coordinate input creates mutable descendants; heterogeneous collections require mutable spatial objects. Complete membership replacement validates technical inputs before changing membership. A recursive component guard rejects immutable descendants without conversion and atomically rejects direct or indirect containment cycles while preserving shared acyclic descendants.

Acceptance criteria

• All acceptance criteria are implemented.
• Sixteen final concrete classes reside directly in their dimension namespaces; family is mutable instance state and defaults to Geometry.
• Membership uses strict zero-based addressing, retains shared instances and supports atomic complete replacement.
• Each context setter supports recursive propagation by default and receiver-only changes with false, preserving coordinates, type and layout.
• Mutable contracts expose no copy operations or dimension setters; EMPTY and geometrically invalid representable values remain supported.

Validation

• Relevant tests pass.
• Repository quality checks pass.
• 66 new tests cover all layouts and member kinds, ownership, nested collections, propagation settings, chaining, immutable rejection, technical failures and addressing boundaries.

Commands run

vendor/bin/phpunit --no-coverage tests/Unit/Types/MutableMultiObjectTest.php tests/Unit/Types/MutableMultiObjectLayoutTest.php tests/Unit/Types/MutableMultiObjectContextTest.php tests/Unit/Types/MutableMultiObjectFailureTest.php
composer test-local
composer quality
vendor/bin/phpunit --no-coverage tests/Unit/Types/MutableSpatialCollectionCycleTest.php
git diff --check

Final full suite: 986 tests, 9,018 assertions, using PHP 8.5.11 with PCOV. Line coverage: 99.49%. Quality includes PHP CS Fixer, PHPStan level 9, PHPMD for source/tests and PHP_CodeSniffer. PHPMD/PDepend emit existing dependency deprecation notices under PHP 8.5; the aggregate command exits successfully. Local tools required permission for their loopback worker processes; no quality rules or suppressions changed.

Standards

• Applicable normative requirements were verified.

Normative references

ISO/IEC CD 13249-3:201x(E), clause 9.1.1, defines collection membership and well-formedness, including common spatial references. This change implements the approved representation/explicit-validation boundary: it retains technically representable inputs and does not claim that mismatched contexts or invalid geometries are normatively valid. No repair, reprojection, automatic geometric validation or new normative interpretation is introduced.

Architecture

• The change follows an already approved architectural decision.

Architecture decision

Follows #38, #39 and #45. A shared internal mutable collection base handles membership and context; type-specific bases hydrate the existing mutable point, line-string and polygon implementations. Immutable implementations remain separate.

Compatibility

• Public API changes were explicitly approved.

The formerly placeholder mutable collection interfaces now expose their planned editing contracts. Custom implementations of these interfaces must implement the added methods. Existing immutable classes and copy APIs retain their behavior. This PR does not add aliases, dependencies, layout conversion or factory changes; factory work belongs to #46.

Documentation

• Documentation was updated where required.

Updated the public type reference, class hierarchy and ownership documentation with accepted inputs, signatures, failure behavior and shared-identity examples.

Developer handoff

Important implementation decisions

The common element API applies to all four aggregate kinds. Single-member mutation takes mutable objects; constructors and complete multi-object replacement additionally accept coordinate arrays. Supplied members preserve their own metadata until an explicit context update. SpatialCollection::toArray() preserves arbitrary nested member boundaries. Membership must be acyclic: constructors and append/insertion/replacement paths raise InvalidValueException before committing a direct, indirect or pre-existing cycle. This is a technical object-graph restriction, independent of geometric validity. Fourteen regressions cover these paths, all layouts, failure atomicity and shared acyclic graphs; thirteen failed before the correction.

Remaining risks or questions

Independent re-review of commit 1f0d975c73556d2f68e77aaadfbf3ac64bbd6cc6 concluded READY FOR MERGE. PHP 8.4, PHP 8.5 and GitGuardian checks passed on that revision. No blocking finding or unresolved gate remains. The PR is open and awaits Alexandre's merge decision.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.50%. Comparing base (d0d711d) to head (1f0d975).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main      #53      +/-   ##
============================================
+ Coverage     99.40%   99.50%   +0.09%     
- Complexity      703     1027     +324     
============================================
  Files           132      192      +60     
  Lines          1525     2206     +681     
============================================
+ Hits           1516     2195     +679     
- Misses            9       11       +2     

☔ 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.

[Alexandre-T]

CHANGES REQUIRED

Reviewed commit: 7e7287e662c5c17cedfe5a95e0d1516665d7f51b.

BLOCKING P1: accepted cyclic membership crashes PHP during context propagation

Location: lib/Types/AbstractMutableCollection.php:150-170, with membership accepted at lines 56-58 and 135-141; MutableComponentGuard does not reject cycles.

The new collection API accepts direct and indirect object cycles, but its recursive context setters have no cycle protection. Both of these examples were independently reproduced in separate PHP processes and terminated with exit code 139:

use LongitudeOne\Core\Enum\SpatialModelEnum;
use LongitudeOne\SpatialTypes\Types\Dimension2\SpatialCollection;

$collection = new SpatialCollection();
$collection->addElement($collection); // Accepted.
$collection->setSrid(4326);           // PHP process crashes.

$a = new SpatialCollection();
$b = new SpatialCollection([$a]);
$a->setElements([$b]);                      // Accepted indirect cycle.
$a->setFamily(SpatialModelEnum::GEOGRAPHY); // PHP process crashes.

The guard's visited set prevents repeated inspection, but allows the cycle to become membership. setFamily() and setSpatialReference() then recursively call the same instances without termination; setSrid() delegates to the latter. This violates Story #45's successful, chainable recursive-context-mutation contract for membership accepted by the public API and the pre-merge requirement of no known blocking correctness defect.

Expected correction: prevent these crashes consistently across append, insertion, single replacement and complete replacement. I recommend atomically rejecting direct and indirect containment cycles as technically unsupported input while preserving shared members in acyclic graphs. Add explicit regression tests covering those paths, unchanged membership/context after rejection, and legitimate shared acyclic descendants; document the resulting contract. This is an object-recursion problem, distinct from geometric validity checking.

Independent validation and scope

• composer test-local: passed, 972 tests and 8,933 assertions.
• composer quality: passed; no source changes. Existing PHPMD/PDepend dependency deprecations are emitted on PHP 8.5.
• git diff --check HEAD^ HEAD: passed; working tree clean.
• Remote checks on the reviewed SHA: PHP 8.4, PHP 8.5 and GitGuardian all successful.
• Reviewed #45, parent #38, shared-contract Story #39, dependency completion, API/ownership separation, all four layouts, shared identity, failure atomicity, propagation settings and documentation. The interface additions follow the explicitly planned mutable-contract work in #39/#45; custom implementations must implement the added operations as disclosed in the PR.
• Inspected the local ISO/IEC CD 13249-3:201x(E) source, clause 9.1.1, including descriptions 10 and 11. The documented distinction between representable data and explicit geometric validation is preserved.

The applicable pre-merge Definition of Done is not satisfied until the blocking issue is corrected and independently re-reviewed. This review does not merge the PR or mark the Story Done.

[Alexandre-T]

READY FOR MERGE

Independently re-reviewed commit 1f0d975c73556d2f68e77aaadfbf3ac64bbd6cc6, including the complete implementation reviewed previously and the cycle-safety correction.

The previous blocking P1 finding is resolved. The component guard now distinguishes active ancestors from completed nodes, rejects the receiving aggregate as an ancestor, and runs before membership is committed. Direct, indirect and pre-existing containment cycles are rejected with InvalidValueException. Repeated members and shared acyclic descendants remain supported. The four aggregate hydrators use the receiver-aware guard, and the documentation describes this technical restriction separately from geometric validation.

I inspected the 14 added regressions covering append, insertion, single and complete replacement, unchanged membership/context after rejection, custom cyclic input, all four layouts, and a shared acyclic diamond. Both original crash scenarios now throw the documented exception without changing membership; subsequent context mutation returns normally.

Independent validation on this revision:

• composer test-local: passed, 986 tests and 9,018 assertions; reported line coverage 99.49%.
• composer quality: passed. The sandbox initially blocked the formatter's loopback worker with EPERM; the same aggregate command passed after the authorized rerun. Existing PHPMD/PDepend dependency deprecations remain informational.
• git diff --check HEAD~2 HEAD: passed; working tree clean.
• Remote CI: PHP 8.4, PHP 8.5 and GitGuardian successful on the reviewed SHA.

The Story #45 acceptance criteria and applicable pre-merge Definition of Done are satisfied. The review covers supported collection kinds/layouts, public mutable contracts, shared ownership and recursive propagation, failure atomicity, immutable-input rejection, EMPTY/incomplete representable values, documentation, compatibility disclosures and the approved #38/#39 representation boundary. No blocking finding or unresolved gate remains.

This supersedes my earlier CHANGES REQUIRED verdict. The PR is ready for Alexandre's merge decision; this review does not merge the PR or declare the Story Done.

[Alexandre-T]

READY FOR MERGE — additional independent verification requested by Alexandre

Reviewed SHA: 1f0d975c73556d2f68e77aaadfbf3ac64bbd6cc6, confirmed as both the current PR head and the locally executed revision.

I personally executed the exact four-collection scenario, independently of the Developer's tests:

$c = new SpatialCollection([], 303);
$b = new SpatialCollection([$c], 202);
$a = new SpatialCollection([$b], 101);
$d = new SpatialCollection([$a], 404);
$c->addElement($d); // Would close D -> A -> B -> C -> D.

Result: the attempted addition throws InvalidValueException with Mutable aggregate members must not form reference cycles. No PHP crash occurs. Explicit runtime checks verified that C remains empty, A still contains the identical B, B the identical C, and D the identical A; all four original SRIDs (101/202/303/404) and Geometry families remain unchanged after rejection.

I then called $d->setSrid(4326) and $d->setFamily(SpatialModelEnum::GEOGRAPHY) on the remaining D -> A -> B -> C graph. Both operations return D, terminate normally, update all four original instances, and preserve their membership and identity links.

The standalone verification ran in a bounded PHP process (max_execution_time=2, memory_limit=64M) and exited successfully with code 0. No implementation or repository file was changed. This additional case confirms the existing READY FOR MERGE verdict; no merge or Done transition is performed.