Change scoping from private to protected

[jaimeperez]

Using private instead of protected for properties and methods make it difficult or even impossible to extend the classes to implement new functionalities without modifying the base class. Private scoping should only be used in the case of internal variable and methods that really should not be used, not even by child classes.

[Maks3w]

I don't agree with that. May the library should be refactored with interfaces and allow good ways for replace the code.

[jaimeperez]

What kind of interfaces? The problem here is that using private scoping it is not possible to extend the classes if you need to use parts of it that are private. Obviously you could solve this by modifying the classes so that they implement directly the functionality you want, but the point with extending classes is precisely to avoid that, isn't it?

Let's have an example. If I want to implement what xmlseclibx calls internally a "library" (in this case, an alternative to openssl and mcrypt) and be able to construct an instance of XMLSecurityKey which allows me to use that new library transparently, I can't with the current scoping, because I would then need to put values into $this->cryptParams, which is private. Of course I could just use a different property of my own, but then you start doing things differently than the original class, introducing new opportunities for bugs and misunderstandings.

[robrichards]

I need to really go through that patch. I don't fully agree with some of the changes you suggest. i.e. All the xxxOpenSSL and xxxMcrypt should remain private. If additional/alternative crypto libraries are needed then I could see either allowing cryptParams to be private or adding some accessor methods to work with it and new methods for the new crypto libraries should be created and hooked into the appropriate encrypt and sign/verify methods.

[jaimeperez]

I really think nobody should be messing with cryptParams. However, if you want to implement a new library, you then definitely need access to it, so if you are doing so as a class extending XMLSecurityKey, then having cryptParams as protected makes sense. Or having setters and getters.

You can then of course add the methods to work with the new library, say signLibrary() for instance, but as you say you need to hook them inside signData(). There it would be nice if I could replicate the switch statement, adding the new library, but I can live with an if statement calling the new library if needed or calling parent::signData($data) instead, definitely.

[relaxnow]

@jaimeperez I also disagree with these changes, I think fabpot makes a good argument here about why you might not want protected as the default: http://fabien.potencier.org/article/47/pragmatism-over-theory-protected-vs-private

See also the Circle-ellipse problem: http://en.wikipedia.org/wiki/Circle-ellipse_problem

That said It's hard to satisfy the Liskov Substitution Principle without the Single Responsibility Principle, for instance the XMLSecurityKey does way too much.

Adding a new library (like a pure php one: http://phpseclib.sourceforge.net ? ) might be better by moving the factory type switch case statement to a separate class and making it return a strategy that takes over some of the logic from XMLSecurityKey:

So something like:

interface XMLSecurityStrategy {
  public function encryptData();
  // ...
}

class XMLSecurityStrategyMcrypt implements XMLSecurityStrategy {
  public function encryptData() {
    // ...
  }
}

class XMLSecurityStrategyFactory {
  public function createForType($type, $params = array()) {
    $params = new XMLSecurityParams();
    $params->library = 'mcrypt';
    // ...
    return $params;
  }
}

class XMLSecurityKey {
  public function __construct(XMLSecurityLibrary $library) {
    $this->library = $library;
  }
}

So in this case you could implement your own factory that has a fallback to returning a XMLSecurityStrategyPHPSecLib object.