Repository navigation
Complete the Data package API and fix creation, validation and output defects - #645
Merged
Merged
Conversation
Complete the owner-assigned Laravel Data assessment against spatie/laravel-data main (ce296f2), restoring the upstream APIs Hypervel had dropped and fixing defects found along the way. Restored upstream APIs: - RouteParameterReference, AuthenticatedUserReference and ContainerReference for validation attributes. An unresolvable container binding now throws instead of resolving to null, which upstream lets turn a database constraint into whereNull. - The computed-input ignore option, non-throwing max transformation depth (Eloquent casts still throw), make:data suffix and namespace config (--target-namespace selects one class's namespace), the VarDumper caster mode, and invalid nested partial checks with ignore_invalid_partials. - The upstream custom cast signature, with a view of the declared property values; prepareForPipeline(); configurable rule_inferrers, resolved for each compilation so scoped inferrers follow the current coroutine; and later-payload precedence for multiple payloads. Fixes: - Validation receives the complete normalized input. Confirmed always failed, and rules such as required_if silently passed, because undeclared fields never reached the validator. Class rule and inferrer contexts and beforeValidation hooks now see the same input; validated output still excludes undeclared keys. - Custom data collection withResponse() overrides are called. - PropertyRules::prepend() keeps argument order (upstream Arr::prepend bug). - A prepareData hook receives each selected value under the spelling resolution reads first, so an identity hook changes nothing. - Inferrer-removed keyword rules never resolve their parameters, and surviving rules resolve once. Performance: the general creation path reads each property once, and nested partial checks only run where a selection continues into a property. The owner accepted the remaining measured cost of the restored partial check (about 0.57 us per item for a collection with a nested only()). The README differences are reconciled against main, with the Data docs, porting guide, sync note and benchmark scenarios updated. Validation: tests/Data, PHPStan, harness comparisons against the base revision, and the SDK generator suite against this branch.
Set the queue targets as the checked-through revisions for laravel/wayfinder main, laravel/docs 13.x, inertiajs/inertia-laravel 3.x and spatie/laravel-data main. Data examined no pull requests, so its last reviewed PR stays unset.
The 1,000-item collection and Eloquent scenarios measured 200 operations per sample and validate-5000-nested 20, so single samples ran for 1.5 to 18 seconds and a full default run took about 15 minutes. They now run 20 and 2 operations per sample; the shortest sample is still about 40 ms, long enough that scheduler noise does not dominate, and a full run takes about 2.5 minutes. The faster scenarios keep their operation counts. The README now asks for the owner's confirmation that the machine is idle before running a benchmark, because background load skews the results.
Port the upstream creation coverage from spatie/laravel-data main (ce296f2): CreationTest and its shared fixtures, merged with the existing cast, metadata, PHPDoc, creation, validation and transformation-context tests under upstream names. Excluded features carry REMOVED comments (EnumerableCast, UnserializeCast, deprecated collection classes, withoutOptionalValues() and custom pipes). Fixes, each with a regression test: - Explicit casts on data object and collection properties receive the input before any of its objects are built. Under validation, the input beneath the cast is prepared and validated by the ordinary Fill in the same construction state; named factories and object construction wait until the cast declines, and a matching factory is no validation exemption. Without validation the cast receives the raw value. - Named object factories must return the requested object, as upstream requires; the Hypervel-only mode that continued from another returned value is removed (owner-approved). - Unions with several container types or a data object record the type the raw value selects, per collection item, and Fill, hook reconciliation, validation and casting all read it. A normalized child is no longer mistaken for an accepted array, typed container arms cast their items, and ambiguous containers are rejected. A union value's inferred type rule follows the type that holds it. - Constructor-only inputs receive their raw input by name, governed only by declared rules (owner-approved). - Absent properties outside the constructor keep constructor-assigned values, Optional input counts as absent, and ancestor-promoted properties ignore input. - Models: names from an overridden getAttribute() are read; an explicit null stays a supplied null (owner-approved), while strict-mode missing attributes and unselected columns without a getter are absent. - collect(null, $into) returns an empty non-paginator target; factory and configured item casts apply; untyped containers convert; float items widen integers. - Anonymous-class PHPDoc names resolve in the declaring file's namespace, and item shorthands apply to any container after exact matches. - Input is normalized once: SourceResolver reports unreadable input and the filling step decides between an error and validation. Structure: extension resolution and reuse move from a by-reference memo threaded through the engine into a per-operation CreationExtensions object, cloned from an empty instance because a constructor call per operation measurably slowed the direct path. A class docblock records the engine's invariants. The Fill, reconciliation and construction phases stay together because they share recorded decisions and re-enter each other. The README, data-objects documentation, porting guide and sync note describe the kept union values, model nulls, constructor-only inputs, cast input and finished-object factories. Verified with the Data suite (988 tests), PHPStan, the SDK generator suite (2186 tests) against this branch, and the benchmark harness against aa08c27 on an idle machine: from -2% to +5% across the measured scenarios, about 45 ns per root operation of it from the extensions object.
…ey exposed
Port the next group of spatie/laravel-data tests (main at ce296f2) onto
Hypervel's Data package: the From* attribute tests (through Hypervel's
contextual attributes), CreationFactoryTest, DataTest, InjectPropertyValuesTest,
FillRouteParameterPropertiesDataPipeTest, MagicalCreationTest, MappingTest,
PipelineTest, CollectionAttributeWithAnotationsTest, the Model, Json and
FormRequest normalizer tests, CreationContextFactoryTest, WithDataTest and the
collection annotation reader dataset. Overlapping Hypervel tests are merged
under upstream names, and excluded upstream cases carry REMOVED comments.
Defects the tests exposed, each with a regression test:
- Contextual constructor values skipped conversion, so a
RouteParameter('id') int failed for '/posts/123'. They are now converted like
unvalidated input after the beforeCreation hooks, and resolved once in the
class's build context through the new Container::resolveContextualParameters().
- Container construction, BoundMethod and the routing dispatchers called
application code from strict files, so typed route parameters rejected
numeric strings that Laravel accepts. NativeInvoker now applies PHP's native
weak scalar conversion there; 'abc' still fails with a TypeError.
- Data scalar conversion used custom casts. It now follows PHP's weak typing
through NativeScalar, keeping the 'true'/'false' and array conversions, so
malformed values, including iterable items, fail with a TypeError.
- A collection class's own @extends annotation now gives a property its item
type after DataCollectionOf and property annotations, with template bounds.
- A required data collection infers present instead of required, so an empty
list passes as in Spatie.
- factory() accepts a CreationContext again and copies its options, never its
hooks.
- FailOnUnknownFields checked the raw request body even when a custom
normalizer produced the source, rejecting valid envelope input.
SourceResolver now separates custom normalization from fixed resolution.
Upstream's optional FormRequestNormalizer is included. The default still reads
a form request like any other request, as upstream does.
The README, data-objects documentation and porting guide describe the
paginator source requirement, the missing required property failure, the
immutable CreationContext, data collection presence, scalar conversion and the
optional normalizer.
Verified with the Data suite (1136 tests), PHPStan and php-cs-fixer. Before
the final Data-only changes, the full parallel suite, FacadeDocblocksTest and
the SDK generator suite also ran against this branch.
Port spatie/laravel-data's ValidationTest (main at ce296f2) onto Hypervel's Data package with all 105 cases, including the four upstream skips, whose malformed expectations are corrected. Duplicates in the reference, Exists and Unique tests move under the upstream names, and DataValidationAsserter gains upstream's rule explosion and redirect, error-bag, messages and attributes assertions. Database rules with query callbacks now compare by the query the callbacks build, so the callback tests use the query builder the presence verifier passes and assert real constraints. Defects the tests exposed, each with a regression test: - Lifecycle methods were called through "Class::method" strings, which the container reads as Class@method, so anonymous data classes failed. They are now array callables. - Rule assembly now matches upstream on both the default and the configured-inferrer paths: inferred presence and type rules come first, and a declared attribute replaces the inferred rule of its type. nullable and sometimes are inferred independently, a declared presence rule drops only the inferred sometimes, and a supplied defaulted property is required. - Backed-enum properties infer the Enum rule, a declared enum rule replaces it, and the accumulator compares Enum rules by state so uniform collections keep wildcard rules. - Nested input Fill cannot read, including blank strings that skip non-implicit rules, now fails validation at its own path instead of creating an empty item or throwing a TypeError. An unresolved morph adds EnsurePropertyMorphable and cannot be constructed; root input still throws. - A missing or null required data object compiles its children's rules. Class rules and configured inferrers share one context per node with its concrete input path, and an unobserved node receives an empty payload rather than its parent's. - Validated and rules-only creation read only the mapped input name, as upstream does, so validation and construction read the same field. Unvalidated creation keeps the PHP-name fallback. - Wildcard collection attributes are formatted like explicit ones in error messages; validator hooks can still replace the formatter. The container documentation describes resolveContextualParameters(), the data-objects page the mapped-name rule, and the README the integer rule inferred for int properties. Validated with the Data suite (1250 tests), composer analyse and composer lint:fix.
…tests
Port spatie/laravel-data's remaining validation tests (main at ce296f2):
RulesTest with its full attribute dataset, RuleNormalizerTest,
RuleDenormalizerTest (commented out upstream, active here),
RequiredRuleInferrerTest and DataClassFromValidationPayloadResolverTest.
PasswordTest, ValidationAttributeTest and ValidationPathTest merge the
upstream cases under upstream names, and duplicated Hypervel tests are
consolidated.
Rules declared through #[Rule] were appended as strings, so
#[Rule('required|string')] duplicated the inferred required and string rules,
and rule inferrers could not see them by type. Upstream's RuleNormalizer and
ValidationRuleFactory are restored: the compiler converts a Rule attribute's
rules into typed validation attributes, so they replace inferred rules of the
same type. The factory mapping stays overridable through mapping().
Defects fixed so normalization never changes what a rule means, each with
regression coverage:
- Attribute factories silently dropped parameters they could not hold, such
as integer:strict. StringValidationAttribute, Exists, Unique and Dimensions
now reject parameters they would lose, and such rules stay as written.
- AcceptedIf, DeclinedIf and ExcludeIf turned '1' into 'true', changing what
a string or integer dependent matches, and the date attributes threw a
TypeError for any parseable date. They now keep parsed values as written for
the validator to resolve; the unused parse helpers are removed.
- The Rule attribute accepts every rule form the validator accepts, including
closures, conditional and compilable rules. Native rule objects and their
query callbacks keep their identity.
Upstream's InvokableRule cases are removed with that deprecated contract;
ValidationRule cases replace them. Carbon 3's timezone: replaces tz:.
The data-objects page explains that Rule attributes replace inferred rules.
Validated with the Data suite (1669 tests), composer analyse and
composer lint:fix.
…fects they exposed
Port the transformation and output group of spatie/laravel-data tests (main at
ce296f2) onto Hypervel's Data package: AppendTest, EmptyTest, PartialsTest,
RequestTest, TransformationTest, WrapTest, the transformer tests including
SerializeTransformerTest, TransformationContextFactoryTest, DataContextTest,
InertiaLazyTest, FromContainerPropertyTest, PartialTest (as PartialTreeTest
cases) and the resolver tests, which run through the Hypervel classes that
replace the resolvers or through the transformed output. Overlapping Hypervel
tests are merged under upstream names, and excluded upstream cases carry
REMOVED comments.
This also completes an audit of the earlier exclusions and adapted
expectations. Defects found by the tests and the audit, each with a regression
test:
- Paginated collections and paginator properties now transform to Spatie's
{data, links, meta} shape in toArray(), toJson() and responses. Responses
build links and meta from the original paginator, so a cursor whose ordering
field the output renames or hides no longer throws.
- A data collection property transforms the items of any iterable a lazy
closure returns, and a data iterable at the maximum depth gives [].
- SerializeTransformer and withOptionalValues()/withoutOptionalValues() are
restored; #[Give] accepts a property path.
- Contextual values are validated with their object, resolved once per
prepared node through a names filter on Container::resolveContextualParameters(),
and prepared again when a hook changes a node's morph class.
- A finished value or collection item applies the declared and class rules
that target it, with their custom messages; field-only messages still follow
mapped input names.
- defaultWrap() is restored, and nested data collections in responses keep
their own wrapper as in Spatie.
- A data collection property without an item class no longer throws on first
use.
- Items read from a collection by key or in a loop carry its partials without
consuming them, and repeated reads no longer pile up partial copies.
- An included lazy property that resolves to Optional is omitted.
- Inertia's scroll metadata reads a paginated data collection's paginator.
Malformed partial paths given in code still throw instead of being ignored or
truncated, and TransformationContext stays final; the README records both. The
README, data-objects and container documentation and the porting guide are
updated.
Verified with the Data suite (1918 tests), PHPStan and php-cs-fixer, plus the
Container and Inertia suites and FacadeDocblocksTest earlier in the slice.
… and fix the defects they exposed Port the remaining spatie/laravel-data tests (main at ce296f2) onto Hypervel's Data package: DataPropertyTypeTest, DataMethodTest, DataParameterTest, DataReturnTypeTest (in DataTypeFactoryTest and DataMethodTest), DataAttributesCollectionTest, DataIterableAnnotationReaderTest, both Eloquent cast tests and SerializeableTest, with upstream's migration and model fixtures. Overlapping Hypervel tests are merged under upstream names, and excluded upstream cases carry REMOVED comments. The structure cache, Livewire and TypeScript transformer tests have REMOVED notes in DataServiceProviderTest, because Hypervel has none of those integrations. Defects found by the tests, each with a regression test: - Input no longer reaches protected or private promoted constructor parameters, so from($request) cannot set an object's own state. A required one fails with CannotCreateData, and its class is not instantiated directly from resolved values. - Another collection's annotation, such as @Property DataCollection<Item> on an array property, gives the property its items when nothing closer matches and no other collection annotation gives a different item type. - The package's internal enums are string-backed, as in Spatie #896, so a creation failure's trace with arguments can be JSON-encoded. - Abstract Eloquent data casts store the subtype's alias when one is registered and its class name otherwise, and read either, as Spatie does. An alias was required, so data stored without one could not be read. The stored type must be a subtype of the declared class, checked before its metadata is built, and then a concrete class, checked with that metadata, before from() runs. - Data collection casts accept collections and other Arrayable values, as Spatie does. Collections keep their data objects, since toArray() would give their output form. The unused DataTypeFactory::buildFromString() is removed. Two upstream annotation fakes documented a parameter they did not declare, so it is added and formatting keeps the tag; two clock-dependent date cases are made deterministic. The README and data-objects documentation are updated. Verified with the Data suite (2103 tests), PHPStan and php-cs-fixer, plus the Inertia suite and AfterEachTestSubscriberTest earlier in the slice.
The header read opcache.enable_cli and opcache.jit from the ini settings, which can differ from what is running: OPcache can be loaded but inactive, and the JIT only runs when its mode and buffer allow it. It now reads opcache_get_status(), reporting 0 and disabled when the extension is missing or inactive. Checked with OPcache off, OPcache on with JIT disabled, and OPcache on with tracing JIT.
Reorganize the guide around common tasks, fill applicable coverage gaps against spatie/laravel-data docs at ce296f22861dc1237ce468754cc7f46d3ac34ad5, and use Laravel-style prose and examples. Preserve Hypervel-specific APIs and update README section links. Resolve validation attribute field references through input-name metadata so conditional rules read the same mapped input as validation. Cover nested and root references, collection paths, custom inferrers and hooks, while leaving literal rule strings unchanged. Validation: 2,136 Data tests and 5,954 assertions pass; composer analyse and composer lint:fix pass. Documentation examples and link, anchor, fence and JSON checks pass.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
|
Member
Author
|
@cubic-dev-ai review |
|
Macroscope has since reviewed this pull request. An earlier review was skipped by a cost limit; a review has now completed, so that notice no longer applies. |
@binaryfire I have started the AI code review. It will take a few minutes to complete. |
PR Summary by QodoComplete Data APIs and repair creation, validation, and output
AI Description
Diagram
High-Level Assessment
Files changed (266)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
19 issues found across 275 files
Confidence score: 2/5
- Existing apps with a published
data.phpmiss the new key, andDataConfigreads it without a fallback, so upgrades can fail. Add a fallback or otherwise handle configs published before this change. DataCreatorcan drop earlier nested values when a later source provides the same parent array, which can break validation rules that depend on those values. Preserve nested keys when merging sources.DataIterableAnnotationReaderignores@implements, so iterable collection properties can lose their item type and nested data conversion. Read the matching@implementstag as well as@extends.ContainerReferencetreats a missing or misspelled property asnull, which can silently make database constraints match null rows. Throw when the property cannot be resolved.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/data/src/Normalizers/Normalized/NormalizedModel.php">
<violation number="1" location="src/data/src/Normalizers/Normalized/NormalizedModel.php:73">
P2: A null returned by an overridden `getAttribute()` is treated as absent unless the raw attributes or built-in mutators declare the key. Data creation can therefore retain a property default instead of using the virtual null; use an override-aware presence check before discarding it.</violation>
</file>
<file name="tests/Data/DataCollectionTest.php">
<violation number="1" location="tests/Data/DataCollectionTest.php:309">
P2: `all()` returns the transformed item representation, not the `CollectionPartialData` object, so this assertion fails. Assert the selected array (`[['name' => 'first']]`) instead.</violation>
</file>
<file name="tests/Data/Fixtures/NestedLazyData.php">
<violation number="1" location="tests/Data/Fixtures/NestedLazyData.php:25">
P2: `fromString()` advertises a late-static return but always constructs `NestedLazyData`; calling it on a subclass therefore throws a return-type `TypeError`. Construct with `new static` to honor the declared return type.</violation>
</file>
<file name="src/data/src/Eloquent/AbstractDataEloquentCast.php">
<violation number="1" location="src/data/src/Eloquent/AbstractDataEloquentCast.php:97">
P2: An alias can shadow a class-name envelope: `createMorphEnvelope()` writes the FQCN when that class has no alias, but this lookup remaps it to the alias target, so reads can return a different subtype. Make class-name and alias resolution unambiguous or reject colliding morph-map entries.</violation>
</file>
<file name="src/data/src/Support/Validation/References/RouteParameterReference.php">
<violation number="1" location="src/data/src/Support/Validation/References/RouteParameterReference.php:44">
P2: This throws when a route parameter has a present property whose value is `null`, preventing nullable route fields from being used as rule parameters. Distinguish an absent path from a present `null` before raising this exception.</violation>
</file>
<file name="src/data/src/Support/Annotations/DataIterableAnnotationReader.php">
<violation number="1" location="src/data/src/Support/Annotations/DataIterableAnnotationReader.php:71">
P2: This ignores item types declared with `@implements`, so directly implemented iterable collection properties lose their item type and nested data conversion. Read the matching `@implements` tag as well as `@extends`.</violation>
</file>
<file name="src/inertia/src/ScrollMetadata.php">
<violation number="1" location="src/inertia/src/ScrollMetadata.php:38">
P2: A `JsonResource` wrapping either paginated data collection still takes this arm, so the collection reaches the paginator checks and metadata generation throws. Unwrap the resource first, then normalize the resulting data collection.</violation>
</file>
<file name="src/data/src/Support/Types/PhpDocTypeNameResolver.php">
<violation number="1" location="src/data/src/Support/Types/PhpDocTypeNameResolver.php:124">
P2: Namespace declarations on the same line overwrite one another here, so an anonymous class in an earlier namespace block can resolve its PHPDoc types against a later namespace. Preserve declaration order and enough source-position information to distinguish declarations on the same line.</violation>
</file>
<file name="tests/Data/Fixtures/SimpleDataCollection.php">
<violation number="1" location="tests/Data/Fixtures/SimpleDataCollection.php:16">
P2: `toJson()` discards caller-supplied JSON flags, preventing callers from requesting options such as `JSON_UNESCAPED_UNICODE`; preserve them while adding pretty-printing with `$options | JSON_PRETTY_PRINT`.</violation>
</file>
<file name="tests/Data/Attributes/Validation/RulesTest.php">
<violation number="1" location="tests/Data/Attributes/Validation/RulesTest.php:33">
P2: This freezes CarbonImmutable globally for the rest of the PHPUnit process, so later tests using `now()` observe May 16, 2020. Reset the clock in `tearDown()` after these tests.</violation>
</file>
<file name="src/data/src/Support/Validation/PropertyRules.php">
<violation number="1" location="src/data/src/Support/Validation/PropertyRules.php:65">
P2: `removeType(Required::class)` does not remove other requiring rules because this check recognizes only rule instances. Detect when the class string names a `RequiringRule` implementation as well, so class-based removal matches the method contract.</violation>
</file>
<file name="tests/Data/Casts/BuiltinTypeCastTest.php">
<violation number="1" location="tests/Data/Casts/BuiltinTypeCastTest.php:85">
P2: `NAN` is coerced to an integer by PHP's weak scalar conversion, so this case does not throw `TypeError` and the test fails. Replace it with a value PHP cannot coerce to `int`, such as an object.</violation>
</file>
<file name="src/data/src/Support/Creation/CreationContext.php">
<violation number="1" location="src/data/src/Support/Creation/CreationContext.php:42">
P2: Inserting this parameter here shifts every existing positional argument after `disableMagicalCreation`, so callers passing `ignoredMagicalMethods` positionally now get a type error. Append the new parameter after the existing constructor parameters to preserve compatibility.</violation>
</file>
<file name="src/data/src/Support/Validation/References/ContainerReference.php">
<violation number="1" location="src/data/src/Support/Validation/References/ContainerReference.php:32">
P2: A missing or misspelled property resolves to `null`, so database constraints using this reference can silently filter on null rows instead of failing. Detect an unresolved property and throw before returning it.</violation>
</file>
<file name="src/data/config/data.php">
<violation number="1" location="src/data/config/data.php:47">
P1: Existing apps with a published pre-PR `data.php` do not receive this new key because `mergeConfigFrom()` shallow-merges top-level config. `DataConfig` reads it without a fallback, so upgrading those apps now fails during provider boot; preserve defaults for missing keys or otherwise migrate published configs.</violation>
</file>
<file name="src/data/src/Support/Creation/ConstructionState.php">
<violation number="1" location="src/data/src/Support/Creation/ConstructionState.php:435">
P2: Resetting an item clears only its override, but `selectedType()` then falls back to the collection template; a missing union-valued auto-lazy default can therefore be cast using a sibling's branch. Mask inherited selections when resetting an item.</violation>
</file>
<file name="tests/Data/Eloquent/DataEloquentCastTest.php">
<violation number="1" location="tests/Data/Eloquent/DataEloquentCastTest.php:267">
P2: This assertion compares the `text` column as an exact JSON string, but transformation emits inherited `variant` before subclass `a`, so the test fails despite correct storage. Expect `variant` before `a`.</violation>
</file>
<file name="src/data/src/Support/Creation/DataCreator.php">
<violation number="1" location="src/data/src/Support/Creation/DataCreator.php:2968">
P2: This replaces a scalar ancestor with an empty array while writing a nested property, silently changing malformed input such as `profile: 'bad'` into an object-shaped payload. Skip the nested write when an ancestor is not an array so validation or construction can reject the original shape.
(Based on your team's feedback about nested payload paths.)</violation>
<violation number="2" location="src/data/src/Support/Creation/DataCreator.php:2994">
P2: This shallow merge drops earlier nested keys when a later source supplies the same parent array, so undeclared values used by validation rules, such as confirmation or conditional fields, disappear. Merge nested arrays recursively while preserving later-source precedence.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
| * Rule inferrers adjust the validation rules of every data property after | ||
| * the package's fixed rule inference. Only add your own inferrers here. | ||
| */ | ||
| 'rule_inferrers' => [], |
There was a problem hiding this comment.
P1: Existing apps with a published pre-PR data.php do not receive this new key because mergeConfigFrom() shallow-merges top-level config. DataConfig reads it without a fallback, so upgrading those apps now fails during provider boot; preserve defaults for missing keys or otherwise migrate published configs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/config/data.php, line 47:
<comment>Existing apps with a published pre-PR `data.php` do not receive this new key because `mergeConfigFrom()` shallow-merges top-level config. `DataConfig` reads it without a fallback, so upgrading those apps now fails during provider boot; preserve defaults for missing keys or otherwise migrate published configs.</comment>
<file context>
@@ -31,11 +40,20 @@
+ * Rule inferrers adjust the validation rules of every data property after
+ * the package's fixed rule inference. Only add your own inferrers here.
+ */
+ 'rule_inferrers' => [],
+
/*
</file context>
Member
Author
There was a problem hiding this comment.
Not changing this. mergeConfigFrom() merges the package defaults into the published config at the top level, so a published data.php without rule_inferrers still gets the default for it. Only nested arrays under a key the app already defines are replaced as a whole, and rule_inferrers is a top-level key.
| } | ||
|
|
||
| // Without a getter, unselected columns and unknown names also read as null, but they are absent rather than supplied. | ||
| if ($value === null |
There was a problem hiding this comment.
P2: A null returned by an overridden getAttribute() is treated as absent unless the raw attributes or built-in mutators declare the key. Data creation can therefore retain a property default instead of using the virtual null; use an override-aware presence check before discarding it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Normalizers/Normalized/NormalizedModel.php, line 73:
<comment>A null returned by an overridden `getAttribute()` is treated as absent unless the raw attributes or built-in mutators declare the key. Data creation can therefore retain a property default instead of using the virtual null; use an override-aware presence check before discarding it.</comment>
<file context>
@@ -54,10 +55,28 @@ protected function fetchNewProperty(string $name, DataProperty $dataProperty): m
+ }
+
+ // Without a getter, unselected columns and unknown names also read as null, but they are absent rather than supplied.
+ if ($value === null
+ && ! array_key_exists($name, $this->model->getAttributes())
+ && ! $this->model->hasAnyGetMutator($name)
</file context>
Member
Author
There was a problem hiding this comment.
Not changing this. A null for a name that isn't a model attribute and has no accessor looks the same as a column that wasn't selected, and unselected columns are treated as missing so the property keeps its default. Virtual attributes are supported through accessors, and those are detected even when they return null.
| $this->assertSame($item, $collection[0]); | ||
| $this->assertSame($item, $collection[0]); | ||
| $this->assertSame([$item], iterator_to_array($collection)); | ||
| $this->assertSame([$item], $collection->all()); |
There was a problem hiding this comment.
P2: all() returns the transformed item representation, not the CollectionPartialData object, so this assertion fails. Assert the selected array ([['name' => 'first']]) instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/Data/DataCollectionTest.php, line 309:
<comment>`all()` returns the transformed item representation, not the `CollectionPartialData` object, so this assertion fails. Assert the selected array (`[['name' => 'first']]`) instead.</comment>
<file context>
@@ -78,22 +276,66 @@ public function testConstructorAndOffsetSetBypassAnOverriddenPublicFromMethod():
+ $this->assertSame($item, $collection[0]);
+ $this->assertSame($item, $collection[0]);
+ $this->assertSame([$item], iterator_to_array($collection));
+ $this->assertSame([$item], $collection->all());
+
+ $this->assertCount(1, $item->getPartialsDefinition()->resolve($item)['only']);
</file context>
Suggested change
| $this->assertSame([$item], $collection->all()); | |
| $this->assertSame([['name' => 'first']], $collection->all()); |
Member
Author
There was a problem hiding this comment.
This test passes. all() returns the stored items, which are the data objects themselves, not their transformed arrays.
| */ | ||
| public static function fromString(string $string): static | ||
| { | ||
| return new self(Lazy::create(fn (): SimpleData => SimpleData::from($string))); |
There was a problem hiding this comment.
P2: fromString() advertises a late-static return but always constructs NestedLazyData; calling it on a subclass therefore throws a return-type TypeError. Construct with new static to honor the declared return type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/Data/Fixtures/NestedLazyData.php, line 25:
<comment>`fromString()` advertises a late-static return but always constructs `NestedLazyData`; calling it on a subclass therefore throws a return-type `TypeError`. Construct with `new static` to honor the declared return type.</comment>
<file context>
@@ -0,0 +1,27 @@
+ */
+ public static function fromString(string $string): static
+ {
+ return new self(Lazy::create(fn (): SimpleData => SimpleData::from($string)));
+ }
+}
</file context>
Suggested change
| return new self(Lazy::create(fn (): SimpleData => SimpleData::from($string))); | |
| return new static(Lazy::create(fn (): SimpleData => SimpleData::from($string))); |
Member
Author
There was a problem hiding this comment.
Not changing this fixture. Nothing extends NestedLazyData or calls fromString() through a subclass, so the static return type only ever sees NestedLazyData itself.
| $node['mappings'] = []; | ||
| $node['children'] = []; | ||
| unset($node['autoLazy'], $node['paginatorSource']); | ||
| unset($node['selectedTypes'], $node['autoLazy'], $node['paginatorSource'], $node['namedFactory'], $node['contextualPrepared']); |
There was a problem hiding this comment.
P2: Resetting an item clears only its override, but selectedType() then falls back to the collection template; a missing union-valued auto-lazy default can therefore be cast using a sibling's branch. Mask inherited selections when resetting an item.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Support/Creation/ConstructionState.php, line 435:
<comment>Resetting an item clears only its override, but `selectedType()` then falls back to the collection template; a missing union-valued auto-lazy default can therefore be cast using a sibling's branch. Mask inherited selections when resetting an item.</comment>
<file context>
@@ -329,7 +432,7 @@ public function resetNodeStructure(): void
$node['mappings'] = [];
$node['children'] = [];
- unset($node['autoLazy'], $node['paginatorSource']);
+ unset($node['selectedTypes'], $node['autoLazy'], $node['paginatorSource'], $node['namedFactory'], $node['contextualPrepared']);
if ($this->pathContainsItem()) {
</file context>
Member
Author
There was a problem hiding this comment.
Not changing this. Every supplied value records its own selection before it's cast. A missing value with an auto-lazy default returns before any cast, and a reset item records its selections again when it's filled, so no value is cast with a sibling's type.
| ])->id; | ||
|
|
||
| $this->assertDatabaseHas($modelClass::class, [ | ||
| 'data' => json_encode(['a' => 'foo', 'variant' => 'foo']), |
There was a problem hiding this comment.
P2: This assertion compares the text column as an exact JSON string, but transformation emits inherited variant before subclass a, so the test fails despite correct storage. Expect variant before a.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/Data/Eloquent/DataEloquentCastTest.php, line 267:
<comment>This assertion compares the `text` column as an exact JSON string, but transformation emits inherited `variant` before subclass `a`, so the test fails despite correct storage. Expect `variant` before `a`.</comment>
<file context>
@@ -52,39 +70,323 @@ protected function defineEnvironment(Application $app): void
+ ])->id;
+
+ $this->assertDatabaseHas($modelClass::class, [
+ 'data' => json_encode(['a' => 'foo', 'variant' => 'foo']),
+ ]);
+
</file context>
Suggested change
| 'data' => json_encode(['a' => 'foo', 'variant' => 'foo']), | |
| 'data' => json_encode(['variant' => 'foo', 'a' => 'foo']), |
Member
Author
There was a problem hiding this comment.
This test passes, so the stored JSON already has the key order the assertion expects.
| $target = &$input; | ||
|
|
||
| foreach ($property->inputPath($this->propertyInputKey($property, $context)) as $segment) { | ||
| if (! is_array($target)) { |
There was a problem hiding this comment.
P2: This replaces a scalar ancestor with an empty array while writing a nested property, silently changing malformed input such as profile: 'bad' into an object-shaped payload. Skip the nested write when an ancestor is not an array so validation or construction can reject the original shape.
(Based on your team's feedback about nested payload paths.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Support/Creation/DataCreator.php, line 2968:
<comment>This replaces a scalar ancestor with an empty array while writing a nested property, silently changing malformed input such as `profile: 'bad'` into an object-shaped payload. Skip the nested write when an ancestor is not an array so validation or construction can reject the original shape.
(Based on your team's feedback about nested payload paths.) </comment>
<file context>
@@ -2503,7 +2905,101 @@ protected function propertyInputKey(
+ $target = &$input;
+
+ foreach ($property->inputPath($this->propertyInputKey($property, $context)) as $segment) {
+ if (! is_array($target)) {
+ $target = [];
+ }
</file context>
Member
Author
There was a problem hiding this comment.
Not changing this. It only happens with several payloads, a prepareData hook, and a dotted input name whose parent a later payload gives as a scalar. In that case the hook sees the value that resolution selected, which is the value the object receives.
|
|
||
| foreach ($sources as $source) { | ||
| if (is_array($source)) { | ||
| $input = $input === [] ? $source : array_replace($input, $source); |
There was a problem hiding this comment.
P2: This shallow merge drops earlier nested keys when a later source supplies the same parent array, so undeclared values used by validation rules, such as confirmation or conditional fields, disappear. Merge nested arrays recursively while preserving later-source precedence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Support/Creation/DataCreator.php, line 2994:
<comment>This shallow merge drops earlier nested keys when a later source supplies the same parent array, so undeclared values used by validation rules, such as confirmation or conditional fields, disappear. Merge nested arrays recursively while preserving later-source precedence.</comment>
<file context>
@@ -2503,7 +2905,101 @@ protected function propertyInputKey(
+
+ foreach ($sources as $source) {
+ if (is_array($source)) {
+ $input = $input === [] ? $source : array_replace($input, $source);
+ }
+ }
</file context>
Suggested change
| $input = $input === [] ? $source : array_replace($input, $source); | |
| $input = $input === [] ? $source : array_replace_recursive($input, $source); |
Member
Author
There was a problem hiding this comment.
Not changing this. Payloads combine per top-level key, with later payloads taking precedence, as documented. A deep merge would change what a later payload's nested value means: it would no longer replace the earlier one.
Code Review by Qodo
1. Custom-context dates fail to parse
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Rule strings were kept whole whenever they contained "regex:" anywhere, so a string such as "required|not_regex:/^a/" reached the rule factory as one unknown rule. The normalizer and denormalizer now keep a string whole only when it starts with regex: or not_regex:, since that pattern may contain a pipe. Other strings split on pipes like Laravel's rule parser. A child path's own terminal wildcard now counts as continuing the partial selection, so "artist.*" next to "*" still checks the nested names under artist. Merging two partial trees keeps the nested property lists of both trees instead of recomputing them from the merged result. Contextual parameter resolution skips variadic parameters. An override is passed to the constructor as a single argument, so a resolved list for a variadic parameter would arrive as one nested array; the container already spreads the contextual value when it builds the class. resolveContextualParameters() is now part of the container contract, and the Data instantiator depends on the contract again. When a property declares several collection item annotations, the fallback item type is chosen by comparing the resolved types. An imported name and a fully qualified name for the same item class now agree, so array properties keep their Data item type. Smaller changes: - The test migration uses an unsigned big integer for fake_model_id, matching the referenced key. - The removeType() docblock explains how requiring rules are matched. - The container reference comment explains why an unresolvable binding fails. - The Data creation hooks documentation states that values returned by the hooks are treated as prepared input. - The container documentation notes that variadic parameters are resolved when the class is built. - docs/todo.md records the planned removal of pipe-separated rule strings in favor of rule arrays. Validation: formatting, static analysis, the Data and Container test suites, the facade docblock test and the route dependency resolver test pass.
Member
Author
|
@macroscope-app review |
|
Manual reviews triggered for commit All prior checks · these links stay valid even if you push more commits. |
Member
Author
|
@cubic-dev-ai review |
|
Review started; results will be posted in the check runs. |
@binaryfire I have started the AI code review. It will take a few minutes to complete. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a broad Data-package feature and engine refactor affecting request creation, validation, casting, transformation, persistence, routing, and container behavior across hundreds of files. An unresolved concrete defect in regex rule handling, along with additional creation and compatibility concerns, leaves material runtime risk requiring human review. Not approved because:
Notes:
Enable approvability here. You can add or adjust custom eligibility rules. Learn more. |
There was a problem hiding this comment.
6 issues found across 277 files
Confidence score: 3/5
DataInstantiator.php: Valid input can fail withpropertyMissingwhen a child constructor skips its declaring parent constructor. Accept the supplied value for that inherited promoted property.DataCreator.php: Hook-introduced inputs can fail when they need an operation’s custom normalizer, and hook replacements can be lost before casting. Apply the normalizers and update construction state with the replacement.docs/todo.md: Removing string-rule support would break the documented public API for#[Rule('required|string')]. Preserve string-rule compatibility at public boundaries and normalize internally.DataCreator.php: Nested date casts can silently fall back to global settings instead of the creation context’s formats and timezone. Copy both date settings into the nested factory.
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/todo.md">
<violation number="1" location="docs/todo.md:73">
P2: This would remove a documented public API, not just change internal normalization: `#[Rule('required|string')]` is explicitly supported. Preserve string-rule compatibility at public boundaries and normalize internally, or specify a versioned deprecation and migration.</violation>
</file>
<file name="src/container/src/Container.php">
<violation number="1" location="src/container/src/Container.php:1775">
P2: An unknown class produces an empty recipe here, so this method silently returns `[]` instead of the `BindingResolutionException` raised by `build()` for the same target. Check `classExists` before iterating so invalid names do not look like classes without contextual parameters.</violation>
</file>
<file name="src/data/src/Support/Creation/DataInstantiator.php">
<violation number="1" location="src/data/src/Support/Creation/DataInstantiator.php:109">
P1: An inherited promoted property is skipped even when the child constructor did not call its declaring parent constructor, so valid input then fails with `propertyMissing`. Accept the supplied value when the property is still uninitialized, while preserving values assigned by the parent constructor.</violation>
</file>
<file name="src/data/src/Support/Creation/DataCreator.php">
<violation number="1" location="src/data/src/Support/Creation/DataCreator.php:114">
P2: Copy `$creationContext->dateFormats` and `$creationContext->dateTimezone` when creating the nested factory; otherwise date casting falls back to global configuration.</violation>
<violation number="2" location="src/data/src/Support/Creation/DataCreator.php:1068">
P2: Apply the operation's configured normalizers before fixed resolution here, matching `fillGeneralNode()`; hook-introduced nested inputs that only a custom normalizer can read otherwise fail creation.</violation>
<violation number="3" location="src/data/src/Support/Creation/DataCreator.php:1806">
P2: Write the replacement to construction state even when it is not finished; otherwise the cast receives the stale or missing value instead of the hook's replacement.</violation>
</file>
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Re-trigger cubic
| $value = $properties[$property->name] ?? null; | ||
|
|
||
| // A property promoted by an ancestor constructor belongs to the constructor chain, so input never replaces it. | ||
| if (! $property->isPromoted |
There was a problem hiding this comment.
P1: An inherited promoted property is skipped even when the child constructor did not call its declaring parent constructor, so valid input then fails with propertyMissing. Accept the supplied value when the property is still uninitialized, while preserving values assigned by the parent constructor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Support/Creation/DataInstantiator.php, line 109:
<comment>An inherited promoted property is skipped even when the child constructor did not call its declaring parent constructor, so valid input then fails with `propertyMissing`. Accept the supplied value when the property is still uninitialized, while preserving values assigned by the parent constructor.</comment>
<file context>
@@ -85,15 +103,30 @@ public function instantiate(DataClass $dataClass, array $properties): BaseData
+ $value = $properties[$property->name] ?? null;
+
+ // A property promoted by an ancestor constructor belongs to the constructor chain, so input never replaces it.
+ if (! $property->isPromoted
+ && array_key_exists($property->name, $properties)
+ && ! $value instanceof Optional
</file context>
Suggested change
| if (! $property->isPromoted | |
| if ((! $property->isPromoted || ! $property->reflection->isInitialized($data)) |
Member
Author
There was a problem hiding this comment.
Not changing this. Promoted properties belong to the constructor that declares them, so input is never assigned to them directly, as in Spatie's DataFromArrayResolver. If a child constructor doesn't call the parent's, the parent's promoted properties stay unset, as they would in plain PHP. Assigning them from outside the class would also fail for readonly properties.
|
|
||
| ## Validation | ||
|
|
||
| - Remove pipe-separated validation rule strings in favor of arrays of rules across both `hypervel/validation` and `hypervel/data`, including Data's rule normalizer, denormalizer and `#[Rule]` handling. Update callers, tests and documentation together; preserve pipes inside individual regex patterns. |
There was a problem hiding this comment.
P2: This would remove a documented public API, not just change internal normalization: #[Rule('required|string')] is explicitly supported. Preserve string-rule compatibility at public boundaries and normalize internally, or specify a versioned deprecation and migration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/todo.md, line 73:
<comment>This would remove a documented public API, not just change internal normalization: `#[Rule('required|string')]` is explicitly supported. Preserve string-rule compatibility at public boundaries and normalize internally, or specify a versioned deprecation and migration.</comment>
<file context>
@@ -70,6 +70,7 @@
## Validation
+- Remove pipe-separated validation rule strings in favor of arrays of rules across both `hypervel/validation` and `hypervel/data`, including Data's rule normalizer, denormalizer and `#[Rule]` handling. Update callers, tests and documentation together; preserve pipes inside individual regex patterns.
- Adopt Brick Math 0.20's bounded parsing API for untrusted numeric validation inputs once the supported dependency graph permits it. Ramsey UUID currently limits Brick Math to 0.18, which has neither bounded parsing nor 0.20's parser-backtracking fix, so an intermediate upgrade would not address this issue. Preserve the configurable exponent-range behavior and cover delegated and compiled rules, oversized mantissas and exponents, malformed input, and `multiple_of`; do not add a local parsing workaround or disguise an incompatible Brick version with a Composer alias.
</file context>
Suggested change
| - Remove pipe-separated validation rule strings in favor of arrays of rules across both `hypervel/validation` and `hypervel/data`, including Data's rule normalizer, denormalizer and `#[Rule]` handling. Update callers, tests and documentation together; preserve pipes inside individual regex patterns. | |
| - Normalize pipe-separated validation rules to arrays internally across `hypervel/validation` and `hypervel/data`, including Data's rule normalizer, denormalizer and `#[Rule]` handling. Preserve public string-rule compatibility and pipes inside regex patterns; update callers, tests and documentation together. |
Member
Author
There was a problem hiding this comment.
Not changing this. The entry records a planned breaking change, and this PR doesn't make it. When it's made, callers, tests and documentation change with it, as the entry says.
| */ | ||
| public function resolveContextualParameters(string $concrete, ?array $names = null): array | ||
| { | ||
| $recipe = $this->getBuildRecipe($concrete); |
There was a problem hiding this comment.
P2: An unknown class produces an empty recipe here, so this method silently returns [] instead of the BindingResolutionException raised by build() for the same target. Check classExists before iterating so invalid names do not look like classes without contextual parameters.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/container/src/Container.php, line 1775:
<comment>An unknown class produces an empty recipe here, so this method silently returns `[]` instead of the `BindingResolutionException` raised by `build()` for the same target. Check `classExists` before iterating so invalid names do not look like classes without contextual parameters.</comment>
<file context>
@@ -1756,6 +1756,55 @@ public function buildWith(Closure|string $concrete, array $parameters = []): mix
+ */
+ public function resolveContextualParameters(string $concrete, ?array $names = null): array
+ {
+ $recipe = $this->getBuildRecipe($concrete);
+ $resolutionState = $this->getOrCreateResolutionState();
+ $resolutionState->buildStack[] = $concrete;
</file context>
Suggested change
| $recipe = $this->getBuildRecipe($concrete); | |
| $recipe = $this->getBuildRecipe($concrete); | |
| if (! $recipe->classExists) { | |
| throw new BindingResolutionException("Target class [{$concrete}] does not exist."); | |
| } |
Member
Author
There was a problem hiding this comment.
Not changing this. The values are resolved for a class that is then built with buildWith(), and buildWith() throws BindingResolutionException for a class that doesn't exist. A check here would report the same error one call earlier.
Comment on lines
+1806
to
+1808
| if ($property->isFinishedValue($value)) { | ||
| $state->writeFinishedPropertyValue($inputPath, $value); | ||
| } |
There was a problem hiding this comment.
P2: Write the replacement to construction state even when it is not finished; otherwise the cast receives the stale or missing value instead of the hook's replacement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Support/Creation/DataCreator.php, line 1806:
<comment>Write the replacement to construction state even when it is not finished; otherwise the cast receives the stale or missing value instead of the hook's replacement.</comment>
<file context>
@@ -1539,6 +1799,17 @@ protected function reconcileProperty(
+ // Without validation, the cast receives the hook's value exactly as given.
+ $state->clearChildStructure($property->name);
+
+ if ($property->isFinishedValue($value)) {
+ $state->writeFinishedPropertyValue($inputPath, $value);
+ }
</file context>
Suggested change
| if ($property->isFinishedValue($value)) { | |
| $state->writeFinishedPropertyValue($inputPath, $value); | |
| } | |
| if ($property->isFinishedValue($value)) { | |
| $state->writeFinishedPropertyValue($inputPath, $value); | |
| } else { | |
| $state->writePropertyValue($inputPath, $value); | |
| } |
Member
Author
There was a problem hiding this comment.
Not changing this. applyPayloadHooks() replaces the operation's payload with the hook's return value before properties are reconciled, so the cast reads the replacement from there. writeFinishedPropertyValue() only marks a value as already built. I checked this with a cast that records its input, and it receives the afterValidation() replacement.
| ->withOptionalValues($creationContext->useOptionalValues) | ||
| ->ignoreMagicalMethod(...$creationContext->ignoredMagicalMethods) | ||
| ->withCastCollection($creationContext->casts) | ||
| ->withNormalizers(...$creationContext->normalizers); |
There was a problem hiding this comment.
P2: Copy $creationContext->dateFormats and $creationContext->dateTimezone when creating the nested factory; otherwise date casting falls back to global configuration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Support/Creation/DataCreator.php, line 114:
<comment>Copy `$creationContext->dateFormats` and `$creationContext->dateTimezone` when creating the nested factory; otherwise date casting falls back to global configuration.</comment>
<file context>
@@ -77,18 +88,32 @@ public function __construct(
+ ->withOptionalValues($creationContext->useOptionalValues)
+ ->ignoreMagicalMethod(...$creationContext->ignoredMagicalMethods)
+ ->withCastCollection($creationContext->casts)
+ ->withNormalizers(...$creationContext->normalizers);
+ }
+
</file context>
Member
Author
There was a problem hiding this comment.
Not changing this. A CreationContext is only built by CreationContextFactory::get(), which always takes dateFormats and dateTimezone from the Data config. An operation can't set its own date settings, so a nested factory built from the same config gets the same values.
| return $this->invokeNamedObjectFactory($dataClass, ...$match); | ||
| } | ||
|
|
||
| $source = SourceResolver::resolve($payload); |
There was a problem hiding this comment.
P2: Apply the operation's configured normalizers before fixed resolution here, matching fillGeneralNode(); hook-introduced nested inputs that only a custom normalizer can read otherwise fail creation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/data/src/Support/Creation/DataCreator.php, line 1068:
<comment>Apply the operation's configured normalizers before fixed resolution here, matching `fillGeneralNode()`; hook-introduced nested inputs that only a custom normalizer can read otherwise fail creation.</comment>
<file context>
@@ -950,44 +1041,61 @@ protected function tryCreateDirectArrayNode(
+ return $this->invokeNamedObjectFactory($dataClass, ...$match);
+ }
+
+ $source = SourceResolver::resolve($payload);
+
+ if ($source === null) {
</file context>
Suggested change
| $source = SourceResolver::resolve($payload); | |
| $normalizers = $extensions->normalizers($dataClass, $state->context); | |
| $source = SourceResolver::normalize($payload, $normalizers) ?? SourceResolver::resolve($payload); |
Member
Author
There was a problem hiding this comment.
Not changing this. Values returned by creation hooks are treated as prepared input, so normalizers, prepareForPipeline() and prepareData hooks don't run on them, as the Data guide documents. A hook that adds nested input returns it in the shape the data class reads.
The previous change kept a rule string whole only when it started with "regex:" or "not_regex:". Laravel's rule parser also treats "notregex" as a regex rule and compares rule names without regard to case, so "notregex:/a|b/" was split on its pipe, and "Regex:/a|b/" had been split before that change too. The normalizer and denormalizer now compare the lowercased first rule name against the names Laravel uses. Numeric partial path segments are stored as integer keys, which lets "meta.0" select an array item. Code that assumed string keys threw a TypeError instead: - merging two partial trees passed an integer key to child(); - the nested partial check passed an integer name to the missing property exception; - the request query string resolver passed an integer field to findProperty(), so a request such as "?only=0" caused a server error. child() and findProperty() accept integer keys, the nested property list holds strings, and the tree docblocks describe integer keys. A numeric segment where a data property is expected follows the usual rules for unknown properties, and the query string resolver treats it as an unknown name. Validation: formatting, static analysis and the Data test suite pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This brings Hypervel's Data package in line with the current
spatie/laravel-dataAPI wherever it fits Hypervel, and fixes a long list of creation, validation and output defects. Spatie's test suite is now part of the Data tests, merged with the existing Hypervel coverage, and every defect it exposed is fixed with a regression test. The Data guide is rewritten around common tasks.Two small framework changes support this. Container and routing calls now convert scalar arguments the way Laravel does, so a typed route parameter accepts
'123'forint $id. The container can also resolve a class's contextual attribute values without building it, which lets the Data package convert them before construction.Restored APIs
Several Spatie APIs that Hypervel had left out are now supported:
RouteParameterReference,AuthenticatedUserReferenceandContainerReferencefor validation attributes. An unresolvable container binding throws instead of resolving tonull, which could otherwise turn a database rule's constraint intowhereNull.#[Rule]normalization. A#[Rule]attribute's rules become typed validation attributes, so#[Rule('required|string')]replaces the inferredrequiredandstringrules instead of duplicating them, and rule inferrers can see them by type. Rule strings split on|as Laravel splits them, except a string that starts with a regex rule, which stays whole because its pattern may contain|.rule_inferrers,ignore_invalid_partials,throw_when_max_transformation_depth_reached,var_dumper_caster_modeand computed-property exception options, andmake:datanamespace and suffix defaults. Rule inferrers are resolved for each compilation, so a scoped inferrer follows the current coroutine.prepareForPipeline(),SerializeTransformer,withOptionalValues()andwithoutOptionalValues(),defaultWrap(),factory()accepting aCreationContext, and the optionalFormRequestNormalizer.{data, links, meta}shape intoArray(),toJson()and responses. Responses build the links and meta from the original paginator, so a cursor whose ordering field the output renames or hides no longer throws.Creation
confirmedalways failed and rules such asrequired_ifsilently passed. Validated output still excludes undeclared keys.'123'becomes123for anintproperty, while'abc'fails with aTypeError. Contextual values such as#[RouteParameter('id')] int $idare converted the same way.from($request)cannot set an object's internal state.getAttribute()are read, and columns that were not selected are treated as missing.FailOnUnknownFieldschecks the normalized source, so a custom normalizer's envelope input is no longer rejected.collect(null, $into)returns an empty target, and a collection class's own@extendsannotation gives a property its item type.Validation
Enumrule.RequiredWith('lastName')or#[Rule('required_with:lastName')], now resolve it to that property's input name, including nested, root and collection paths. Rule strings returned fromrules()are passed to the validator as written.TypeError. A missing ornullrequired data object now compiles its children's rules.integer:strict, keep the rule as written;AcceptedIf,DeclinedIfandExcludeIfno longer turn'1'into'true'; and the date attributes no longer throw aTypeErrorfor a parseable date.Output
Optionalis omitted, and nested data collections in responses keep their own wrapper.TypeErrorwhen nested partials are checked or merged, or when they come from a request's query string. Array indexes remain supported, and a numeric segment where a data property is expected follows the usual rules for unknown properties.Eloquent casts
Abstract data casts store the subtype's alias when one is registered and its class name otherwise, and read either. An alias was required before, so data stored without one could not be read. The stored type must be a concrete subtype of the declared class, checked before the class is created. Collection casts accept collections and other
Arrayablevalues, and the package's internal enums are string-backed so a creation failure's trace can be JSON-encoded.Framework changes
BoundMethod, the controller and callable dispatchers, andController::callAction()call application code through a small invoker that leaves outstrict_types, so PHP applies its normal weak scalar conversion as it does in Laravel.Container::resolveContextualParameters(), now part of the container contract, resolves a class's contextual attribute values, optionally limited to named parameters, so a package can adjust them and pass them tobuildWith(). Variadic parameters are left for the build to resolve, because an override is passed as a single argument.#[Give]accepts a property path, likeRouteParameterandCurrentUser.Design and performance
The package keeps its fixed creation engine rather than Spatie's configurable pipeline. Validation and construction share one prepared input and one set of type decisions, metadata is cached for the worker lifetime, and per-operation state lives in objects created for that operation, so concurrent requests in a worker never share it.
Performance was measured with the Data benchmark harness, with OPcache on and JIT off. The general creation path now reads each property once. Restoring Spatie's check for invalid nested partials costs about 0.57 µs per item for a collection with a nested
only(); the check only runs where a selection continues into a property. The later creation, validation and output fixes add about 1.5–1.9 µs per object on the general creation path (about 9%), 0.8–2.1 µs per item for general-path collections, about 6 µs (14%) for contextual constructor injection, about 3% to validation, and about 0.15 µs to the direct and flat factory paths. Nested creation, eager and lazy collections, large validation, transformation, responses, Eloquent casts and relation loading stay within run-to-run variation. A review of the hot paths found no redundant work to remove. The harness's expensive scenarios now use shorter samples, so a full run takes minutes rather than a quarter of an hour, and its header reports the OPcache and JIT status that is actually running.Intentional differences
The package README lists the remaining differences from Spatie and why. The notable ones:
nullattributes staynull.200for every method; set201inwithResponse()when something was created.pipeline()overrides and custom data pipes are not supported. Named factories,prepareForPipeline()and factory hooks provide customization without allowing the built-in creation phases to be reordered.Documentation
The data objects guide is reorganized around common tasks, fills the gaps against Spatie's documentation, and covers the behavior above. The porting guide lists the behavior differences to review in ported code, and the container guide documents
resolveContextualParameters()and#[Give]property paths.docs/upstream-sync/sync.yamlrecords the checked-through revisions for Wayfinder, the Laravel docs, Inertia and Laravel Data.Verification
The Data suite, PHPStan and formatting pass on the final head. The Container and Inertia suites,
FacadeDocblocksTestand the full parallel suite also ran during development. The documentation examples were run, and its links, anchors and code fences were checked.Note
Complete Data package creation, validation, and transformation pipeline
ConstructionStateand resolved through the newCreationExtensionsregistry in DataCreator.phpRuleNormalizer/RuleDenormalizer, field references resolve through mapped input names, configured rule inferrers can add or remove rules, and preserved values restore from validator dataCannotPerformPartialOnDataFieldContainer::resolveContextualParameters,NativeInvokerweak-typed construction, andGiveattribute property-path extractionmake:datanamespace/suffix defaultsConstructionState(Castimplementations must update);DataTypeFactory::buildFromString,InvalidDataDeclaration::nonPublicPromotedProperty,CannotFindDataClass::forTypeable, and severalCannotCastDatafactories were removed; scalar coercion now follows PHP weak typing so non-numeric strings raiseTypeErrorMacroscope summarized 036bb01.