codecov[bot] commented 4 years ago

Codecov Report

Merging #224 into master will increase coverage by 0.67%. The diff coverage is 9.09%.

@@             Coverage Diff              @@
##             master     #224      +/-   ##
============================================
+ Coverage     87.51%   88.19%   +0.67%     
- Complexity     2117     2415     +298     
============================================
  Files           143      143              
  Lines          5376     5846     +470     
============================================
+ Hits           4705     5156     +451     
- Misses          671      690      +19

Impacted Files	Coverage Δ	Complexity Δ
src/SAML2/Compat/ContainerSingleton.php	`16.66% <0%> (-20.84%)`	`6 <2> (+2)`
src/SAML2/XML/AbstractXMLElement.php	`100% <100%> (ø)`	`26 <1> (ø)`	:arrow_down:
src/SAML2/XML/md/ContactPerson.php	`100% <0%> (ø)`	`76% <0%> (+38%)`	:arrow_up:
src/SAML2/XML/shibmd/Scope.php	`100% <0%> (ø)`	`10% <0%> (+2%)`	:arrow_up:
src/SAML2/XML/saml/Attribute.php	`100% <0%> (ø)`	`40% <0%> (+20%)`	:arrow_up:
src/SAML2/XML/ExtendableAttributesTrait.php	`100% <0%> (ø)`	`11% <0%> (ø)`	:arrow_down:
src/SAML2/XML/saml/Assertion.php	`97.71% <0%> (+0.34%)`	`410% <0%> (+205%)`	:arrow_up:
src/SAML2/XML/saml/SubjectConfirmationData.php	`93.58% <0%> (+0.48%)`	`82% <0%> (+31%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 54c1cbb...39739db. Read the comment docs.

tvdijen commented 4 years ago

OK, so the way it is now works as follows:

registerClass('\This\Is\A\Test');

Will lead to a registry like this:

Array(
    [\This\Is\A\Test] => Array(
            [0] => urn:oasis:names:tc:SAML:2.0:assertion
            [1] => Test
        )
)

And then getRegisteredClass(Constants::NS_SAML, 'Test'); will output: \This\Is\A\Test

jaimeperez commented 4 years ago

OK, so the way it is now works as follows:

registerClass('\This\Is\A\Test');

Will lead to a registry like this:
Array(
    [\This\Is\A\Test] => Array(
            [0] => urn:oasis:names:tc:SAML:2.0:assertion
            [1] => Test
        )
)
And then getRegisteredClass(Constants::NS_SAML, 'Test'); will output: \This\Is\A\Test

Uhm, shouldn't we index it the other way around? In this case it's a little harder to search for occurrences (while insertion in the registry has constant time in both alternatives), and also this opens up the possibility to have multiple classes registered for the same namespace and element name, leading to ambiguous behaviour.

Also, I just checked and it is in fact not possible to use an array as an index, so we would need to transform that into a string. I'd suggest something like the following:

$key = join(
    ':',
    [
        urlencode($namespace),
        $element
    ]
);

That way we can keep both insertions and search at linear complexity (O(1)).

simplesamlphp / saml2

Add class registry for extensions #224

Codecov Report