Migrate DOM-classes to PHP 8.4's new DOM-API

[tvdijen]

Closes #85

[codecov]

Codecov Report

❌ Patch coverage is 95.52239% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.92%. Comparing base (d99f764) to head (bbcec53).

Additional details and impacted files

@@                Coverage Diff                @@
##             release-3.x      #84      +/-   ##
=================================================
- Coverage          92.93%   92.92%   -0.02%     
- Complexity           747      750       +3     
=================================================
  Files                125      125              
  Lines               2875     2884       +9     
=================================================
+ Hits                2672     2680       +8     
- Misses               203      204       +1     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tvdijen]

This feels a little awkward, but just like the QName-issues in xml-wsdl I had to set the xmlns:ds namespace-attribute on the parent.

[ioigoume]

@tvdijen in what sense it feels awkward? Because you mutate the parent in a method that is responsible for the signature? Two suggestions for this one either add the ns attribute to the parent the way you do it now, but i would propose this implementation:

    public function toXML(?Dom\Element $parent = null): Dom\Element
    {
        $e = $this->instantiateParentElement($parent);

        if ($parent !== null && $parent->lookupNamespaceURI('ds') !== C::NS_XDSIG) {
            $parent->setAttributeNS(C::NS_XMLNS, 'xmlns:ds', C::NS_XDSIG);
        }


        if ($this->getId() !== null) {
            $e->setAttribute('Id', strval($this->getId()));
        }

        $this->getSignedInfo()->toXML($e);
        $this->getSignatureValue()->toXML($e);

        $this->getKeyInfo()?->toXML($e);

        foreach ($this->getObjects() as $o) {
            $o->toXML($e);
        }

        return $e;
    }

or if you want to avoid mutating the parent then this should be like this:

 public function toXML(?Dom\Element $parent = null): Dom\Element
    {
        $e = $this->instantiateParentElement($parent);

        // Bind the ds prefix on this element (the Signature subtree root).
        $e->setAttributeNS(C::NS_XMLNS, 'xmlns:ds', C::NS_XDSIG);


        if ($this->getId() !== null) {
            $e->setAttribute('Id', strval($this->getId()));
        }

        $this->getSignedInfo()->toXML($e);
        $this->getSignatureValue()->toXML($e);

        $this->getKeyInfo()?->toXML($e);

        foreach ($this->getObjects() as $o) {
            $o->toXML($e);
        }

        return $e;
    }

then the fixtures need to change. I think for SAML2 xsd both ns attribute placements are correct.

[tvdijen]

CI complained about localName not existing on Dom\Node, so I added the check for Dom\Element

[ioigoume]

+1
Perhaps we could consider:

if ($nsnode instanceof Dom\Attr || $nsnode instanceof Dom\Element) {

since DOMXPath namespace::* can return nodes that CI/PHPStan types as Dom\Node (often Dom\Attr)

[ioigoume]

PR looks good. I’ve added a few comments for consideration and an additional PR with a test fix and a composer improvement.