Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues detected with static analyzer. #188

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jaimeperez
Copy link
Contributor

Running vimeo/psalm on the library yields lots of errors. This PR fixes all the errors. In general, most of them are trivial to fix (type confusions, default values that don't make sense, etc).

The most difficult cases to deal with are those where we need to change the signature of a public method, namely XMLSecEnc::locateKeyInfo()and XMLSecEnc::staticLocateKeyInfo(). However, in both cases it doesn't really make sense to have the base key as an optional parameter (best-case-scenario, the method returns null; worst-case-scenario, the code breaks because we try to call the loadKey() method on a null variable).

Another slightly more complicated case is XMLSecurityDSig::getXPathObj(). Originally, it was returning null when xPathCtx and sigNode were both null. That's only possible if someone is messing up with sigNode manually (since the property is public), setting it to null, which wouldn't make any sense (you cannot expect the library to give you an XPath context that you can use to search a node, if you have actually cleared that node and there's nowhere you can search in). The proposed change assumes getXPathObj() will always return a valid XPath object, or raise an exception if none can be created (as per the case described above).

Psalm complains that arguments 3 and 4 of DOMNode::C14N() need to be an array, and null might be given. However, those arguments are optional, so it is fine to pass null there. This must be a bug in psalm.
psalm.xml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants