Coverage decreased (-0.06%) to 63.779% when pulling 5cf2fbb on tdiscuit:master into 381bb70 on simplesamlphp:master.

@tdiscuit could you please add a unit test?

Coverage increased (+1.2%) to 65.086% when pulling 52e9d94 on tdiscuit:master into 381bb70 on simplesamlphp:master.

Coverage increased (+1.3%) to 65.155% when pulling a94403b on tdiscuit:master into 381bb70 on simplesamlphp:master.

@relaxnow I've added unit tests and SAML2\XML\md\ContactPerson is 100% covered.

I also noticed I missed pulling out attributes on the contact person element if XML is supplied, so I added that in.

Thanks so much @tdiscuit!

I was wondering... would it make sense to have this as a new class, extending ContactPerson? Basically, a class specific to this use case, setting whatever needs to be set to comply with the SIRTFI contact, so that you just need to mark it like that in some way and then use the new class instead of the generic one. I think it could be worth evaluating the idea since ContactPerson is quite generic and the SIRTFI stuff is very specific on the contrary.

Anytime @jaimeperez! But I still have more work to do since I'll be submitting a PR for SimpleSAMLphp as well ;) ... once I hash that bit out.

I was thinking the same thing, but I went with this route since I am doing this for Sirtfi now, but there may be other trust frameworks that I don't know about that require additional attributes on other contact types. So by keeping it generic, it becomes very easy to implement. Of course I am open to discussion around that.

That's also true, although I don't know if there are other frameworks doing the same already. Maybe @joostd or @pmeulen know better?

In any case, I think this is also a sensible approach since the implementation is then extremely simple.

I'm not aware of any other trust frameworks that use extensions to the ContactPerson element. Likely the current approach is indeed just fine and suffices for now. To keep it 'agile' we can refactor this later should other use cases pop up. All in all I propose to merge this PR.

Merged. thanks!

Thank you guys!