Skip to content

Commit 976e49a

Browse files
magicsundaymagicsunday
authored
GH-74: Split the 164-line mapSingleObject() along its phases (#120)
* GH-74: Split mapSingleObject() along its four phases The method ran a full pipeline in one body, its phases already marked by comment blocks. It is now an orchestration that reads as the pipeline it is - collect converted values, apply the unknown collector, report missing properties, hydrate - with each phase in its own method. The state the phases hand along - the converted values, the names actually mapped, the diverted unknown keys - travels by return rather than by shared locals, so each method's inputs and outputs are its signature. That is also why collectConvertedValues() returns the unknown keys separately for applyUnknownCollector() rather than merging inline: the merge is a distinct phase, not a tail of the collection loop. 180 lines of body became a 31-line orchestration plus four named methods. Behaviour-neutral: the whole suite was green before this commit's own formatting pass. #72 and #73 had already removed the configuration round trip and the per-element reflection this method used to carry, so what remained split cleanly along the existing seams. * GH-74: Fold the collector merge into the collect phase The simplicity review pushed back on applyUnknownCollector() as its own method, and the argument held: it re-read metadata->collectorProperty that collectConvertedValues() already has as a local, and it forced that method to return a third tuple element whose only purpose was to be handed straight back for merging. The divert and the merge are two halves of one concern - a key with no declared property is diverted during the loop and the gathered keys merged after it - so they belong in one method. collectConvertedValues() now does both and returns a two-tuple. The orchestration drops to three phases, and the double read is gone. The issue suggested applyUnknownCollector() as a phase name, but 'e.g.' - and a single-caller method re-reading its own input is what KISS ranks below keeping the concern whole. Also trimmed per the review: the orchestration's lead comment no longer narrates the refactor's history, and a docblock sentence that restated an inline comment three lines down is gone.
1 parent 5a67529 commit 976e49a

1 file changed

Lines changed: 103 additions & 39 deletions

File tree

‎src/JsonMapper.php‎

Lines changed: 103 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
use MagicSunday\JsonMapper\Exception\ReadonlyPropertyException;
2828
use MagicSunday\JsonMapper\Exception\TypeMismatchException;
2929
use MagicSunday\JsonMapper\Exception\UnknownPropertyException;
30+
use MagicSunday\JsonMapper\Metadata\ClassMetadata;
3031
use MagicSunday\JsonMapper\Metadata\ClassMetadataFactory;
3132
use MagicSunday\JsonMapper\Report\MappingReport;
3233
use MagicSunday\JsonMapper\Report\MappingResult;
@@ -600,26 +601,54 @@ private function mapSingleObject(
600601
string $resolvedClassName,
601602
MappingContext $context,
602603
): object {
603-
$source = $this->toIterableArray($json);
604+
// Orchestrates the phases below; the state they hand along travels by return rather than
605+
// by shared locals.
606+
$metadata = $this->classMetadataFactory->forClass($resolvedClassName);
604607

605-
// One derivation for the class, reused for every element of a collection. All of this is
606-
// fixed by the declaration, and used to be re-reflected per mapSingleObject() call.
607-
$metadata = $this->classMetadataFactory->forClass($resolvedClassName);
608+
[$convertedValues, $mappedProperties] = $this->collectConvertedValues(
609+
$this->toIterableArray($json),
610+
$resolvedClassName,
611+
$metadata,
612+
$context,
613+
);
614+
615+
if ($context->isStrictMode()) {
616+
$this->reportMissingProperties($resolvedClassName, $metadata, $mappedProperties, $context);
617+
}
618+
619+
return $this->hydrate($resolvedClassName, $metadata, $convertedValues, $context);
620+
}
621+
622+
/**
623+
* Converts every payload value once, keyed by the property it maps to.
624+
*
625+
* Whether a value ends up a constructor argument or is assigned afterwards, it goes through
626+
* the exact same conversion, replace-property, replace-null and error-handling pipeline.
627+
*
628+
* A key matching no declared property is not converted but diverted to the nominated collector,
629+
* and the gathered keys are merged into it once the loop completes - the two halves of the one
630+
* unknown-key concern, kept together.
631+
*
632+
* @param array<array-key, mixed> $source Payload as an associative array.
633+
* @param class-string $resolvedClassName Class the values are mapped onto.
634+
* @param ClassMetadata $metadata The class's derived shape.
635+
* @param MappingContext $context Active mapping context.
636+
*
637+
* @return array{0: array<string, mixed>, 1: list<string>} Converted values by property, and the
638+
* names actually mapped.
639+
*/
640+
private function collectConvertedValues(
641+
array $source,
642+
string $resolvedClassName,
643+
ClassMetadata $metadata,
644+
MappingContext $context,
645+
): array {
608646
$properties = $metadata->properties;
609647
$replacePropertyMap = $metadata->replaceMap;
648+
$collectorProperty = $metadata->collectorProperty;
610649
$mappedProperties = [];
611-
612-
// A class may nominate one property (via the UnknownPropertyCollector attribute) as the sink
613-
// for every source key that matches no declared property. Such keys are gathered here, by
614-
// normalized name and raw value, and handed to that property after the main pass instead of
615-
// being ignored or reported.
616-
$collectorProperty = $metadata->collectorProperty;
617-
$collectedUnknown = [];
618-
619-
// Convert every payload value once, collecting the results by property name. Whether a
620-
// value ends up as a constructor argument or is assigned afterwards, it goes through the
621-
// exact same conversion, replace-property, replace-null and error-handling pipeline.
622-
$convertedValues = [];
650+
$collectedUnknown = [];
651+
$convertedValues = [];
623652

624653
foreach ($source as $propertyName => $propertyValue) {
625654
$normalizedProperty = $this->normalizePropertyName($propertyName, $replacePropertyMap);
@@ -707,13 +736,13 @@ private function mapSingleObject(
707736
});
708737
}
709738

710-
// Hand the gathered unknown keys to the nominated collector as the raw associative array of
711-
// normalized name to unconverted value, bypassing the per-value conversion pipeline (its
712-
// element type is deliberately open). Left untouched when nothing was gathered, so the
713-
// property keeps its constructor default. The consumer interprets the raw map itself. Any
714-
// explicitly mapped value for the same property is merged in rather than overwritten, so a
715-
// payload that carries both the collector key and unknown keys loses neither. array_replace
716-
// (not array_merge) preserves numeric keys instead of re-indexing them.
739+
// Merge the diverted unknown keys into the collector, the tail of the same concern that
740+
// diverted them above. They go in as the raw associative array of normalized name to
741+
// unconverted value, bypassing the per-value pipeline (the collector's element type is
742+
// deliberately open). Left untouched when nothing was gathered, so the property keeps its
743+
// constructor default. Any explicitly mapped value for the same property is merged in
744+
// rather than overwritten, so a payload carrying both the collector key and unknown keys
745+
// loses neither. array_replace (not array_merge) preserves numeric keys.
717746
if (($collectorProperty !== null) && ($collectedUnknown !== [])) {
718747
$mappedProperties[] = $collectorProperty;
719748
$existingValue = $convertedValues[$collectorProperty] ?? [];
@@ -724,24 +753,59 @@ private function mapSingleObject(
724753
);
725754
}
726755

727-
if ($context->isStrictMode()) {
728-
foreach ($this->determineMissingProperties($resolvedClassName, $properties, $mappedProperties) as $missingProperty) {
729-
$context->withPathSegment($missingProperty, function (MappingContext $propertyContext) use (
730-
$resolvedClassName,
731-
$missingProperty,
732-
): void {
733-
$this->handleMappingException(
734-
new MissingPropertyException($propertyContext->getPath(), $missingProperty, $resolvedClassName),
735-
$propertyContext,
736-
);
737-
});
738-
}
756+
return [$convertedValues, $mappedProperties];
757+
}
758+
759+
/**
760+
* Records a failure for every required property the payload did not supply.
761+
*
762+
* @param class-string $resolvedClassName Class being mapped.
763+
* @param ClassMetadata $metadata The class's derived shape.
764+
* @param list<string> $mappedProperties Names the payload actually supplied.
765+
* @param MappingContext $context Active mapping context.
766+
*
767+
* @return void
768+
*/
769+
private function reportMissingProperties(
770+
string $resolvedClassName,
771+
ClassMetadata $metadata,
772+
array $mappedProperties,
773+
MappingContext $context,
774+
): void {
775+
foreach ($this->determineMissingProperties($resolvedClassName, $metadata->properties, $mappedProperties) as $missingProperty) {
776+
$context->withPathSegment($missingProperty, function (MappingContext $propertyContext) use (
777+
$resolvedClassName,
778+
$missingProperty,
779+
): void {
780+
$this->handleMappingException(
781+
new MissingPropertyException($propertyContext->getPath(), $missingProperty, $resolvedClassName),
782+
$propertyContext,
783+
);
784+
});
739785
}
786+
}
740787

741-
// Build the object through its constructor when it declares promoted or required
742-
// parameters (an immutable value object cannot be populated afterwards); otherwise fall
743-
// back to an argument-less instantiation. Either way, any collected value that is not a
744-
// constructor argument is assigned afterwards, so mixed classes lose nothing.
788+
/**
789+
* Builds the object and assigns the converted values it did not consume as constructor arguments.
790+
*
791+
* The object is built through its constructor when it declares promoted or required parameters
792+
* (an immutable value object cannot be populated afterwards); otherwise through an argument-less
793+
* instantiation. Either way, any collected value that is not a constructor argument is assigned
794+
* afterwards, so mixed classes lose nothing.
795+
*
796+
* @param class-string $resolvedClassName Class to build.
797+
* @param ClassMetadata $metadata The class's derived shape.
798+
* @param array<string, mixed> $convertedValues Values to hydrate with.
799+
* @param MappingContext $context Active mapping context.
800+
*
801+
* @return object The built and populated object.
802+
*/
803+
private function hydrate(
804+
string $resolvedClassName,
805+
ClassMetadata $metadata,
806+
array $convertedValues,
807+
MappingContext $context,
808+
): object {
745809
$constructor = $metadata->constructor;
746810
$consumed = [];
747811

0 commit comments

Comments
 (0)