Repository navigation
Decode through ElementLookup so attributes and child elements stay apart - #80
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).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
|
NickSdot
reviewed
Oct 15, 2026
| "php-soap/wsdl": "^1.19", | ||
| "php-soap/xml": "^1.10", | ||
| "php-soap/wsdl-reader": "~0.32" | ||
| "php-soap/wsdl-reader": "dev-fix/qualified-attribute-refs as 0.34.0" |
Member
Author
There was a problem hiding this comment.
Yes, will tag and merge soon
Contributor
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
2 participants
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 #79, so it targets that branch until it is merged.
Attributes mixed with child elements
Decoding an object read every child element and every attribute of the element into one array keyed by local name. An attribute with the same name as a child overwrote it, and an attribute could stand in for a child that was not there. That includes
xsi:typeand namespace declarations: a propertytypecould decode to the xsi:type of its parent. @NickSdot found this and fixed it in #72.ElementLookup
This PR rebuilds #72. Decoding now reads from
Soap\Encoding\Xml\Node\ElementLookup, built once per element.element($name)only returns child elements,attribute($localName, $namespace)reads one attribute in one namespace,text()gives the text content of an element without children, andchildren()gives every child element for the any property. The built in decoder lenses read from it, so a child, an attribute and the text never stand in for each other.An attribute is looked up in the namespace the WSDL gives it: its target namespace when it is qualified, no namespace when it is not. A miss is null. There is no fallback to another attribute with the same local name. I tried one and it made sparse decoding up to 5% slower, while it only helps servers that send attributes in a different namespace than their WSDL says, and I know of none.
DocumentToLookupArrayReaderand its flat lookup array are gone. Nothing in the package reads that array any more, soElementLookuponly has to be a read view over one element.Differences from #72
@{namespace}namekeys. The lenses read attributes from the lookup object withattribute(), so the array has no reader left and I removed it.ElementLookupinstead of the array. That is the breaking part.AnyElementEncoderis the only implementer in this package and readschildren().refare read and written in their own namespace, see below. Separates XML attribute and child element lookup #72 looked them up without namespace and lostxml:lang.NickSdot's tests are ported, with him as co-author on those commits. The two that asserted
@{...}keys now assertattribute().Attributes declared through ref
A ref points to a global attribute declaration, and a global attribute is always qualified with the namespace of the schema that declares it. The metadata reported those attributes as unqualified, so the encoder wrote
lang="nl"where it should writexml:lang="nl", and wroteo:flagandtns:codewithout prefix. With the wsdl-reader change that marks ref attributes as qualified, the encoder writes all three in their namespace and the decoder reads them back by that namespace. Forxml:langthe encoder no longer declares thexmlprefix, which the XML spec already binds.ext-soap writes
xml:langthe same way, but writes refs to other global attributes without namespace. Here I followed the schema instead of ext-soap, soSchema038Testchanged its expectation fromint="123"totns:int="123".Dependencies
This needs the qualified attribute refs from php-soap/wsdl-reader#66 and #67, released in wsdl-reader 0.34.0, so composer.json requires
~0.34.Release notes
ProvidesObjectDecoderLenslenses now receiveSoap\Encoding\Xml\Node\ElementLookupinstead of the lookup array: read$lookup->element('name')or$lookup->attribute('name', $namespace)where you read$data['name'].refare now written in the namespace of their declaration, for exampletns:code="c"instead ofcode="c".DocumentToLookupArrayReaderis removed. Build anElementLookup::fromElement($element)and readelement(),children(),attribute()andtext()from it.Speed
Base is #79 (absent optional elements). Medians over three interleaved rounds on the same machine, with ext-soap as the noise reference. The 3MB workloads decode about 3MB of XML into stdClass or classmapped objects: sparse has 41000 items with 3 of 30 elements and 1 of 5 attributes present, dense has 5400 items with 28 elements and 4 attributes.
The phpbench rows were measured before the last two commits, which removed the legacy reader and changed the child walk; the 3MB rows include them. Sparse decoding pays a few percent for the lookup object and for the lens closures, which cost more per property than an array index on the old lookup array. Building the lookup itself is cheaper than building the old array, because attributes are no longer copied into it and the children are walked through
firstElementChildandnextElementSibling. The other rows are within the run to run spread.