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

add createXmlSecurityDSig() method #10

Closed
wants to merge 12 commits into from
Closed

Conversation

zogs
Copy link
Contributor

@zogs zogs commented Jan 30, 2016

allowing to create it with optional properties

allowing to create it with optional properties
@zogs
Copy link
Contributor Author

zogs commented Jan 30, 2016

What do you think of this implementation ?

@zogs zogs closed this Jan 30, 2016
@zogs zogs reopened this Jan 30, 2016
@Maks3w
Copy link
Owner

Maks3w commented Jan 30, 2016

XmlDsig is stateful so its not safe reuse the same instance

@zogs
Copy link
Contributor Author

zogs commented Jan 31, 2016

Ok, so i did as you ask for. Does it fits you ?

/**
* Create the XLMSecurityDSign class
*
* @return new XMLSecurityDSig()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not follow phpdoc syntax. Should be @return XMLSecurityDSig

*
* @return new XMLSecurityDSig()
*/
protected function createXmlSecurityDSig($options = array()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The argument $options is not used. Please remove.

Also move the protected method after public methods.

@Maks3w
Copy link
Owner

Maks3w commented Jan 31, 2016

Watch this other CS violations https://travis-ci.org/Maks3w/xmldsig/jobs/105999819#L245

@zogs
Copy link
Contributor Author

zogs commented Jan 31, 2016

Not used to Travis-ci and php-cs i had some trouble validating the build... But it should be OK now

@Maks3w
Copy link
Owner

Maks3w commented Feb 1, 2016

Looks good. Could you do a last thing? Please squash your commits in a single one

@zogs
Copy link
Contributor Author

zogs commented Feb 1, 2016

Opening a new PR.

@zogs zogs closed this Feb 1, 2016
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.

None yet

2 participants