GH-71: Centralise the record-or-throw policy that is actually shared

AGENTS.md

@@ -112,6 +112,14 @@ Guide for LLM-based assistants (Codex/Copilot/ChatGPT, etc.) working in this rep
 * A rejected value is recorded exactly once. Recording it and then continuing produces either a
   duplicate record further up or a native crash - throw instead, and let the single catch site
   record it.
+* The abort-or-record policy lives in `MappingContext::throwOrRecord()`, whose name states the
+  order because the order is the contract. A site that has something usable to hand back - an empty
+  collection, an unconverted value - routes through it. The two that do not both record BEFORE
+  raising, and each says so at its own call site: the shared catch, because it IS the catch the
+  helper's throw reaches; and the collection element loop, because an aborting run would otherwise
+  lose the element's own record. Finishing the centralisation past those two loses a record in each
+  case, silently, since the caller still gets its exception - and on a nested payload a deeper
+  record can survive and leave the report looking complete.
 * Whether a recorded failure also aborts the run is the ENTRY POINT's decision, not the
   configuration's. `map()` raises on the first failure in strict mode; `mapWithReport()` always
   collects, because returning a report is its entire purpose. Strict mode decides only *what*

src/JsonMapper.php

@@ -542,12 +542,11 @@ private function mapCollection(
             // pinned that since long before this change. Only the shape that can satisfy neither
             // reading is rejected.
             //
-            // Thrown rather than routed through handleMappingException(), and so not consulting
-            // shouldAbortOnError() the way the collection factory's guard does. That guard has a
-            // partial answer to offer - an empty collection - so it can record and carry on. This
-            // one has none: returning after recording would let map() fall through to the
-            // single-object lane and build the very element being rejected. The catch that
-            // receives the throw records it exactly once.
+            // Thrown rather than routed through throwOrRecord(). That helper is for a site with a
+            // partial answer to hand back - the collection factory's guard returns an empty
+            // collection - so it can record and carry on. This one has none: returning after
+            // recording would let map() fall through to the single-object lane and build the very
+            // element being rejected. The catch that receives the throw records it exactly once.
             // A LIST whose entries are not mappable is refused for the same reason, and the scalar
             // test alone does not catch it because such a payload IS an array. A list of scalars
             // cannot be a collection of objects, so it fell through to the single-object lane and
@@ -935,12 +934,14 @@ private function handleMappingException(
         MappingException $exception,
         MappingContext $context,
     ): void {
-        $context->recordException($exception);
-
+        // NOT throwOrRecord(): routing it through the helper would leave an aborting run with no
+        // record at all, because this IS the catch site the helper's throw is caught by.
+        //
         // Asked of the context rather than the configuration: strict mode decides what counts as
         // a failure, the entry point decides what happens to one. map() raises on the first in
-        // strict mode; mapWithReport() exists to return a report and so collects them all, which
-        // is what its own recipe demonstrates.
+        // strict mode; mapWithReport() exists to return a report and so collects them all.
+        $context->recordException($exception);
+
         if ($context->shouldAbortOnError()) {
             throw $exception;
         }
@@ -1103,14 +1104,7 @@ function (MappingContext $childContext) use ($json, $type, &$lastException): arr
         // all null types - a shape Symfony's TypeInfo does not produce. It stays because the
         // invariant lives in another method and a future member kind could break it, but it is
         // deliberately not claimed as covered.
-        //
-        // Asked of the context rather than the configuration for the reason given in
-        // handleMappingException(), so that it cannot become the one site that still aborts.
-        if ($context->shouldAbortOnError()) {
-            throw $exception;
-        }
-
-        $context->recordException($exception);
+        $context->throwOrRecord($exception);
 
         return $json;
     }

src/JsonMapper/Collection/CollectionFactory.php

@@ -82,20 +82,9 @@ public function mapIterable(mixed $json, Type $valueType, MappingContext $contex
         };
 
         if (!is_array($source)) {
-            $exception = new CollectionMappingException($context->getPath(), get_debug_type($json));
-
-            // Asked of the context, not the configuration: strict mode decides what counts as a
-            // failure, the entry point decides whether one aborts the run. mapWithReport() turns
-            // aborting off, so this has to yield or the report stops after the first failure.
-            //
-            // Thrown BEFORE recording. When the run aborts, the exception reaches a catch site
-            // that records it, so recording here as well files the same failure twice - visible to
-            // a caller that supplies its own context and inspects it after catching.
-            if ($context->shouldAbortOnError()) {
-                throw $exception;
-            }
-
-            $context->recordException($exception);
+            $context->throwOrRecord(
+                new CollectionMappingException($context->getPath(), get_debug_type($json)),
+            );
 
             // An empty collection, not null. Null is this method's "no collection was asked for"
             // sentinel - the nullable branch above - and every consumer reads it that way: the
@@ -140,10 +129,16 @@ static function (MappingContext $elementContext) use ($exception): void {
                     },
                 );
 
-                // Asked of the context, not the configuration. Rethrowing here means the property
-                // loop above records the very same element failure a second time, so the run has
-                // to be one that aborts - otherwise the caller receives a duplicate and loses the
-                // rejected element's valid siblings along with it.
+                // NOT throwOrRecord(): this site records BEFORE raising, and the helper does the
+                // opposite. Routing it there loses the element's OWN record on an aborting run -
+                // silently, since the caller still gets its exception, and a deeper record written
+                // further inside the element can survive and make the report look complete. (The path is not the obstacle - withPathSegment() restores it in
+                // a finally, so a throw from inside the closure propagates cleanly. What the
+                // segment buys is that the record names the element rather than the collection.)
+                //
+                // Rethrowing here means the property loop above records the very same element
+                // failure a second time, so the run has to be one that aborts - otherwise the
+                // caller receives a duplicate and loses the rejected element's valid siblings.
                 if ($context->shouldAbortOnError()) {
                     throw $exception;
                 }

src/JsonMapper/Context/MappingContext.php

@@ -235,6 +235,30 @@ public function recordException(MappingException $exception): void
         $this->addError($exception->getMessage(), $exception);
     }
 
+    /**
+     * Raises the failure when the run aborts on the first one, and records it otherwise.
+     *
+     * The name states the order because the order is the whole contract: a site that must record
+     * even while aborting cannot use this, and two do. Each explains itself at its own call site.
+     *
+     * @param MappingException $exception Failure to raise or record
+     *
+     * @return void
+     *
+     * @throws MappingException When the entry point aborts on the first failure
+     */
+    public function throwOrRecord(MappingException $exception): void
+    {
+        // When the run aborts, the exception reaches a catch site that records it, so recording
+        // here as well files the same failure twice - visible to a caller that supplies its own
+        // context and inspects it after catching.
+        if ($this->shouldAbortOnError()) {
+            throw $exception;
+        }
+
+        $this->recordException($exception);
+    }
+
     /**
      * Returns collected mapping errors.
      *

src/JsonMapper/Value/Strategy/BuiltinValueConversionStrategy.php

@@ -249,7 +249,7 @@ private function guardCompatibility(mixed $value, BuiltinType $type, MappingCont
             // leaves the caller to store a null the declared type forbids. On a property that
             // means a value contradicting its own docblock; in a collection it means the offending
             // element is kept instead of dropped. The caller records the throw exactly once, which
-            // is why this branch does not consult shouldAbortOnError() the way the mismatch below
+            // is why this branch does not go through throwOrRecord() the way the mismatch below
             // does - it never records, so it cannot double-record.
             throw new TypeMismatchException($context->getPath(), $identifier->value, 'null');
         }
@@ -258,14 +258,9 @@ private function guardCompatibility(mixed $value, BuiltinType $type, MappingCont
             return;
         }
 
-        $exception = new TypeMismatchException($context->getPath(), $identifier->value, get_debug_type($value));
-
-        // Thrown before recording, for the reason given in the null branch above.
-        if ($context->shouldAbortOnError()) {
-            throw $exception;
-        }
-
-        $context->recordException($exception);
+        $context->throwOrRecord(
+            new TypeMismatchException($context->getPath(), $identifier->value, get_debug_type($value)),
+        );
     }
 
     /**

tests/JsonMapper/Context/MappingContextTest.php

@@ -12,6 +12,7 @@
 namespace MagicSunday\Test\JsonMapper\Context;
 
 use MagicSunday\JsonMapper\Context\MappingContext;
+use MagicSunday\JsonMapper\Exception\TypeMismatchException;
 use PHPUnit\Framework\Attributes\Test;
 use PHPUnit\Framework\TestCase;
 
@@ -101,4 +102,44 @@ public function itDefersTheAbortDecisionToStrictModeUntilOverridden(): void
             MappingContext::OPTION_ABORT_ON_ERROR => true,
         ]))->shouldAbortOnError(), 'The explicit option overrides lenient mode too.');
     }
+
+    #[Test]
+    public function itRecordsAFailureWhenTheRunCollects(): void
+    {
+        $context = new MappingContext(['root']);
+
+        $context->throwOrRecord(new TypeMismatchException('$.value', 'int', 'string'));
+
+        self::assertSame(1, $context->getErrorCount());
+    }
+
+    #[Test]
+    public function itRaisesAFailureWhenTheRunAborts(): void
+    {
+        $context = new MappingContext(['root'], [MappingContext::OPTION_ABORT_ON_ERROR => true]);
+
+        try {
+            $context->throwOrRecord(new TypeMismatchException('$.value', 'int', 'string'));
+
+            self::fail('An aborting run must raise.');
+        } catch (TypeMismatchException) {
+            // Expected - what matters is that nothing was recorded on the way out.
+        }
+
+        // Nothing recorded: the catch site that receives the throw records it, so recording here
+        // too would file the same failure twice. That is the whole reason the helper raises first.
+        self::assertSame(0, $context->getErrorCount(), 'The raising path records nothing.');
+    }
+
+    #[Test]
+    public function itFollowsStrictModeWhenTheAbortOptionIsAbsent(): void
+    {
+        // The helper asks shouldAbortOnError(), so it inherits that accessor's fallback rather
+        // than repeating the decision - which is the duplication it exists to remove.
+        $context = new MappingContext(['root'], [MappingContext::OPTION_STRICT_MODE => true]);
+
+        $this->expectException(TypeMismatchException::class);
+
+        $context->throwOrRecord(new TypeMismatchException('$.value', 'int', 'string'));
+    }
 }