Repository navigation
Remove the last intermediate throws and guard against new ones - #75
Merged
Merged
Conversation
veewee/reflecta#39 adds optional_index() and optional_property(), stacked on #37. Until 1.1.0 is tagged the branch comes in through a VCS repository aliased as 1.1.0, so this cannot be merged before the constraint is back on a release.
The built-in lenses were index() and property(), which throw for every absent element, attribute or property, and the exception was caught again by optional() or runLens. reflecta's optional_index() and optional_property() return null in those cases without throwing, which is what the caught exception turned into before, so they replace the built-in lenses unwrapped whether or not the property is optional. Custom lenses from ProvidesObjectEncoderLens and ProvidesObjectDecoderLens keep the optional() wrapping and the runLens catch.
An untyped value went through Psl's int, float and bool coercion in turn and every miss threw a CoercionException, so a plain string threw three or four times. The decoder now checks the same rules up front: the int and float checks repeat Psl's string coercion, including its trimming of leading zeros that decodes '0-5' as -5. The test compares the result with Psl itself over every short string, so a Psl upgrade that changes those rules shows up as a failure.
php-soap/engine#30 adds TypeCollection::findByNameAndXmlNamespace(). Until it is released, the branch is aliased as 2.21.0 so php-soap/wsdl-reader's ^2.20 constraint still resolves.
Every soap-enc array of a builtin item type, such as xsd:int or xsd:anyType, missed the metadata lookup and threw a MetadataException before falling back to xsd:anyType. The engine's findByNameAndXmlNamespace() returns null instead, so the fallback no longer needs an exception.
veewee/xml's document_encode() checks every element for @attributes, @namespaces, @cdata and @value through Psl unions that throw on null, so the fixture threw four times per encode. Writing the element directly keeps what the test proves: an element-aware simple type encoder writes its own element.
composer throws:report now exits with 1 when an encode or decode throws internally in a test that does not expect an exception. The custom lens tests throw on purpose, so they are listed per test and site in allowed-throws.php with the number of throws they may make; an entry that no longer throws as listed fails the run too, so the list only shrinks.
A line number in allowed-throws.php broke the guard on any unrelated edit above the call. The entry is already scoped to one test with an exact count, so the file and the function it calls are precise enough.
Member
Author
|
@coderabbitai review |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 17
✨ Finishing Touches
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 |
|
Arrays of complex types, arrays of arrays, empty arrays, nil items, an xsi:type per item, apache maps, soap-enc structs, SOAP 1.2 encoded arrays and SOAP 1.2 envelopes all encode and decode without intermediate throws. These scenarios keep it that way.
Member
Author
|
@coderabbitai review |
|
# Conflicts: # .github/workflows/throws.yaml
# Conflicts: # src/Encoder/SimpleType/ScalarTypeEncoder.php # tests/Integration/NoIntermediateThrowsTest.php # tests/Unit/Encoder/SimpleType/ScalarTypeEncoderUntypedDecodeTest.php
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.
Stacked on #74 (#74), which sits on #73 (#73). This PR removes the last intermediate throws from the suite and adds a CI guard that fails as soon as one comes back.
Merging is blocked on one release. php-soap/engine#30 is released as 2.21.0 and this PR now requires
^2.21. veewee/reflecta still comes from a branch through a VCS repository:dev-feature/optional-lenses as 1.1.0becomes^1.1once veewee/reflecta#39 is released, and the VCS repository entry goes away at the same time.Soap-enc array item types
A soap-enc array with a builtin item type, such as
xsd:intorxsd:anyType, looked that type up in the metadata, missed, and caught theMetadataExceptionto fall back toxsd:anyType. php-soap/engine#30 addsTypeCollection::findByNameAndXmlNamespace(), which returns null on a miss, andSoapArrayAccessnow uses it with the same fallback. The oldtry_catchalso wrapped$context->metadata->getTypes(), so an exception while loading the types fell back toxsd:anyTypeas well. Now only a missing type falls back, and a loading error propagates.Guard
composer throws:reportnow exits with 1 when the suite makes an intermediate throw, and the Throws CI job runs it after theno-throwsgroup. It counts the same throws the report from #73 counts: the stack passes through an encode or decode in src/, and the test does not expect an exception.Deliberate throws are listed in
tests/Support/Throws/allowed-throws.php, one entry per test, exception class and the src file plus the function it calls there, each with the number of throws it may make and the reason. There are two entries, both inObjectEncoderCustomLensTest, whose custom lenses throw on purpose to prove that they still get theoptional()andrunLenstreatment. Any throw past the listed count fails the run, and so does an entry that no longer throws as often as listed, which keeps the list from growing stale. There is no general budget.PHPUnit extensions cannot set the exit code, so the extension calls
exit(1)from anApplication\Finishedsubscriber. That runs after PHPUnit has printed its own output, and only when PHPUnit would have exited with 0.To check that the guard catches a regression, I put
optional(index($name))back as the default decoder lens. The report then showed 57ArrayAccessExceptionthrows in 19 tests and exited with 1.The four Psl
AssertExceptionthrows inElementAwareEncoderTestdid not belong on the allowlist. The test registers its own anyType encoder, which called veewee/xml'sdocument_encode(), and its element builder checks@attributes,@namespaces,@cdataand@valuethrough Psl unions that throw on null. The fixture now writes its element with the veewee/xml writer, and the test still checks that an element-aware simple type encoder writes its own element.Throw report
composer throws:reporton the suite of this stack, so including the scenarios #73 added. The suite of main on its own had 303 intermediate throws in 110 tests.UnreflectableExceptionon stdClass hydrationCoercionExceptionandRestrictionExceptionin the untyped scalar chainArrayAccessExceptionfor an absent element or attributeUnreflectableExceptionandErrorfor an absent or uninitialised propertyMetadataExceptionfor a builtin soap-enc item typeAssertExceptionfrom a test encoder that calls veewee/xmlBenchmarks
vendor/bin/phpbench run benchmarks/ --report=aggregatewith Xdebug off, PHP 8.4.13 on macOS, against the baseline stored for #73. The figures for this PR are the mean of three runs.The ExtSoap subjects run ext-soap and none of this package's code. Their +2% to +10% is how far this machine has drifted since the baseline was stored, and #74 measured in the same session shows the same drift. For that reason I report without a phpbench
--assert: with a 5% tolerance it fails on the ExtSoap subjects alone, while every subject that runs this package's code passes it. Against #74 this PR is flat within noise, which is expected because the benchmark fixture has no soap-enc arrays. Decode is 13% to 24% faster than main on the complete fixture and 33% to 34% on the sparse one. Encode stays within 3%.3MB responses
The benchmark fixture has 12 elements, too small to show what #70 reported (6s to 0.6s on a 3MB response). So I built a synthetic list of items with 30 optional elements and 5 optional attributes each and ran it through main, the head of #70 and this PR, with reflecta 1.1.0 and engine 2.21.0 installed in all three. Sparse has 3 elements and 1 attribute present per item (41,000 items, 3.04MB), dense has 28 and 4 (5,400 items, 3.02MB). Seconds, mean of two interleaved rounds of medians, Xdebug off. All three produce identical XML and identical decoded values.
Main already benefits from reflecta 1.1.0 here, because
^1.0resolves to it: with reflecta 1.0.0 the sparse stdClass decode on main took 7.05s, so against that the stack is 10.4x faster. The gain grows with the share of absent fields.Summary by CodeRabbit