Repository navigation
Land the ElementLookup and null handling stack on main - #89
Merged
Merged
Conversation
…ildren DocumentToLookupArrayReader puts child elements and attributes in one flat array keyed by local name, so an attribute overwrites or stands in for a child with the same name, xsi:type and xmlns declarations included. ElementLookup keeps them apart: element() only returns child elements, attribute() reads by namespace and local name, text() follows the existing '_' rule, and toArray() builds the legacy array with the child winning on a collision and protocol attributes left out. attribute() falls back to the only other attribute with that local name when the exact namespace is absent. That keeps xml:lang, other ref attributes and servers that qualify attributes differently from their WSDL decoding, while id next to a:id still resolves to the right one. The child wins rule, the protocol attribute filter and two of the tests come from #72. Nothing uses it yet. Co-authored-by: NickSdot <catch.github.com.nicksdot@nicksdot.dev>
The built-in decoder lenses read child elements, attributes and the '_' text from ElementLookup instead of the flat lookup array. An attribute no longer overwrites or stands in for a child element with the same name, xsi:type and namespace declarations included, and an attribute property only reads the attribute in its own namespace, with the single namesake fallback of ElementLookup::attribute(). ObjectEncoder::from builds one lookup per element. DocumentToLookupArrayReader now returns ElementLookup::toArray(), so its callers get the same fix: the child wins on a collision and xsi, soap-enc and xmlns attributes are left out. Custom decoder lenses still receive that array through an adapter that ObjectAccess builds once per property. The next commit hands them the lookup itself. Because the adapter is the innermost frame in src/ now, the allowed throw of the throwing lens fixture moves from ObjectEncoder to ObjectAccess. The tests port #72. Its two lookup array tests are in ElementLookupTest since the previous commit, asserting attribute() where #72 asserted '@{ns}name' keys, which this design does not have. Co-authored-by: NickSdot <catch.github.com.nicksdot@nicksdot.dev>
ElementLookup::attribute() no longer falls back to the only other attribute with the same local name. The fallback cost about half of the sparse decode regression of the previous commit, because every absent optional attribute walked the attributes of its element, and it only helped servers that send an attribute in another namespace than their WSDL declares. An attribute in the wrong namespace now decodes to null. Attributes declared through ref, like xml:lang, need their own namespace from the metadata to be found. That comes with the wsdl-reader change that reports them as qualified, required in a later commit, which also brings the xml:lang decode test.
ProvidesObjectDecoderLens lenses now read from the ElementLookup of the parent element instead of the flat lookup array, so they can ask for a child element or for an attribute in its namespace without the two being mixed. This breaks third party decoder lenses: read $lookup->element('name') or $lookup->attribute('name') where the lens read $data['name'].
AnyElementEncoder, the only implementer in src, takes the children from ElementLookup::children(). The adapter of the previous commits is gone, so the allowed throw of the throwing lens fixture is attributed to ObjectEncoder again. The fixtures read the lookup like a third party lens would.
The wsdl-reader branch fix/qualified-attribute-refs reports attributes declared through ref as qualified, because a global attribute in XSD is always qualified, and it adds the implicit xml prefix to the WSDL namespaces. Until it is released the branch comes in through a VCS repository aliased as 0.34.0. This commit gets replaced by a ~0.34 constraint after the release, so the branch cannot be merged before that. Schema038Test references the global attribute tns:int. ext-soap writes it as int="123" without namespace, the encoder now writes tns:int="123" as the schema says. I changed the expectation instead of keeping ext-soap parity, since a server that validates against the schema rejects the unqualified form. Decoding reads it back by its namespace.
With the wsdl-reader branch, an attribute declared through ref is qualified and carries the namespace of the schema that declares it, so the encoder writes xml:lang, o:flag and tns:code, and the decoder reads them back by that namespace. The compatibility tests cover a ref to xml:lang, a ref to an attribute of another schema and a ref to a global attribute of the same schema. They fail on wsdl-reader 0.33, which writes all three without namespace. ext-soap writes xml:lang the same way, but writes the foreign and the same-namespace ref without namespace. Schema088Test and Schema089Test follow the schema instead, as Schema038Test already does. AttributeBuilder no longer declares xmlns:xml next to xml:lang. The xml prefix is bound by the XML spec, and XMLWriter only leaves the declaration out when it gets no namespace URI for the prefix.
ObjectEncoder decodes from ElementLookup now, so the profile script times that step too. DocumentToLookupArrayReader stays in the profile because it is built on top of the lookup.
Nothing in the package reads the flat lookup array any more: built-in and custom decoder lenses read ElementLookup, and AnyElementEncoder reads its children. ElementLookup is now only a read view over one element, with the child index as the one thing it stores.
firstElementChild and nextElementSibling skip text and comment nodes without a predicate call: about 1.0us instead of 2.4us for an element with 30 children (PHP 8.4, hrtime probe).
A null value on an optional element that is not nillable goes through ErrorHandlingEncoder and OptionalElementEncoder only to come back as an empty string. ObjectAccess now records per property whether that is the case, and ObjectEncoder skips the iso for those nulls. The empty write still closes the start tag of the parent, so the first skipped element of an object keeps it to write <a></a> as before. Custom encoders that handle null themselves are not wrapped in the optional element encoder and keep their output.
Every expectation also holds on the base branch, so the skip leaves the output of these cases as it was.
Xmlns::xsi() builds a new object on every call. OptionalElementEncoder now resolves it once per iso, and DocumentToLookupArrayReader compares against a class constant.
Locator\Node\value() reads substitutedNodeValue and coerces it to a string, which it already is on a Dom\Element. Reading the property skips the function call and the Psl coercion.
ObjectAccess no longer depends on ErrorHandlingEncoder wrapping OptionalElementEncoder directly. An encoder that implements WritesNothingForNull answers for itself, ErrorHandlingEncoder forwards the question, and any other encoder keeps its own null handling.
An object whose elements are all null now encodes as <a/> instead of <a></a>. Both are the same empty element; the short form is what an object without children already gets elsewhere.
…per-decode-reads # Conflicts: # src/Xml/Reader/DocumentToLookupArrayReader.php
Two booleans per property, one for absent nillable elements and one for elements that write nothing, described a single decision from two sides. The encoder now answers it once through NullValueAware with a NullValueHandling case, and ObjectAccess keeps one map of those answers. A custom OptionalAware encoder that is not wrapped in OptionalElementEncoder no longer has its absent optional nillable elements left out, unless it implements NullValueAware itself, because only the encoder can tell what it writes for null.
Decode through ElementLookup so attributes and child elements stay apart
Every property of a decoded object went through Lens::readonly, its closure and the try/catch in runLens, only to call element(), attribute() or text() on the lookup. ObjectAccess now records per property how the built in lens reads it, and ObjectEncoder::from makes that call itself. Those three calls cannot throw, so the try/catch had nothing to catch for them. Custom ProvidesObjectDecoderLens lenses keep going through runLens. The built in lenses stay in decoderLenses and are built from the same record, so they read exactly what the direct call reads.
… too Missing elements, attributes in another namespace and nil or mixed text all read null, and those are the reads where a lens and the direct call could differ.
…ements Skip the encoder for null optional elements that write nothing
Cheaper xsi:nil checks and value reads on decode
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 28 minutes. View limit details
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 |
Read built in decoder properties from ElementLookup directly
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.
Brings the reviewed stack onto main. #80, #83 and #85 were merged into their stack base branches instead of main, and #84 merges into
absent-optional-elementsas well. This PR carries them to main in one merge.ElementLookup, require wsdl-reader 0.34NullValueHandling)All four were reviewed on their own PRs. The one conflict, in
ObjectEncoder::tobetween #83'sonNulland #84'sencodeReaders, is resolved on #84's branch.