Update SsnValidator implement

[soullivaneuh]

If ronanguilloux/IsoCodes#34 is merged, review SsnValidator class to handle singleton class implementation.

[enumag]

Singleton is wrong, you should use Ssn as service in DIC and require it in constructor of SsnValidator.

[soullivaneuh]

That's an idea, but I don't want to require symfony/dependency-injection here because a provide both Symfony and Silex support.

[enumag]

You should NOT require symfony/dependency-injection. The SsnValidator doen't care where the instance will come from. And you can use a fallback:

public function __construct(IsoCodes\Ssn $ssn = null)
{
    $this->ssn = $ssn ?: new IsoCodes\Ssn();
}

This is the correct solution in my opinion. The singleton PR should not get merged.

[soullivaneuh]

I don't get it. From where the Ssn instance passed on the constructor come from then?

And BTW, singleton system also have goal to match the IsoCodesInterface later: ronanguilloux/IsoCodes#33

[enumag]

I'm not sure how symfony/dependency-injection works but presumably when you want to add a service you write some kind of extension class right? You should add such class to this package for those users that use symfony/dependency-injection. But you should not make it a dependency as it is not an essential part of this package. I could implement an extension for nette/di if you want me to.

Oh and singletons are plain wrong pretty much everywhere.

[enumag]

Since you still don't understand the optional dependencies, this package is a nice example of what I have in mind. If you look into composer.json it doesn't have any dependencies. Yet it contains classes which use parts of Symfony/Twig/Zend/Nette/Latte and god knows what else. None od them is required because these classes are just bridges, not essential parts of the package. But you can find them all in require-dev section in composer json because they are obviously nedded for tests.

In theory you should have a separate composer package for every single one of these bridges. It would be difficult to maintain though which is why such solution is not used.

You should do the same here - add a bridge for symfony/dependency-injection and maybe some other packages (you mentioned Silex) or just wait for PRs. But don't add it to dependencies.

[soullivaneuh]

I could implement an extension for nette/di if you want me to.

Not sure if it will be accepted, but I'm curious to see your POC.

Oh and singletons are plain wrong pretty much everywhere.

May I ask why? :-)

Honestly, I'm not a big fan of singleton too. I chose this because it's the only way I found to match IsoCodesInterface as I said before.

In theory you should have a separate composer package for every single one of these bridges.

Yeah, this what I did before, but I deprecate them to get all on the same because of pain to maintain 5 lines of codes for each new release:

• https://github.com/Soullivaneuh/SLLHIsoCodesValidatorBundle
• https://github.com/Soullivaneuh/IsoCodesValidatorServiceProvider

[enumag]

I could implement an extension for nette/di if you want me to.

Not sure if it will be accepted, but I'm curious to see your POC.

I'd have to study your bundle in more detail to see if there is anything else I should register as service but for the SsnValidator (if you add the constructor from above) the following extension class would be enough for nette bridge. The Ssn instance from DI container would be passed to SsnValidator because of this line in Kdyby/Validator which is an integration for symfony/validator to nette/di. Or I could just add SsnValidator as another service, which is a bit cleaner but doesn't matter.

<?php

namespace SLLH\IsoCodesValidator\Bridge\Nette;

use Nette\DI\CompilerExtension;

class IsoCodesExtension extends CompilerExtension
{

    public function loadConfiguration()
    {
        $builder = $this->getContainerBuilder();

        $builder->addDefinition($this->prefix('ssn'))
            ->setClass('IsoCodes\Ssn');
    }

}

Oh and singletons are plain wrong pretty much everywhere.

May I ask why? :-)

My main reason is that when you use a DI container it pretty much solves the need of only having one instance of a class with the benefits that 1) The class itself doesn't know anything about it. 2) In case you really need two instances with different configuration it is achievable.

---

So what do you think about the solution with SsnValidator::__construct from above + having bridges for Symfony and perhaps Silex in this package (without explicitly requiring any of them)? You would not need the singleton that way, right?

[soullivaneuh]

In case you really need two instances with different configuration it is achievable.

Well, for this case we really don't need two instances of Ssn.

So what do you think about the solution with SsnValidator::__construct from above + having bridges for Symfony and perhaps Silex in this package (without explicitly requiring any of them)

I also have bridge for Symfony and Silex. See Bundle and Provider folders. But yes, I think this should be move to Bridge\[Symfony|Silex] folder.

For SsnValidator::__construct Nette framework, as I said, feel free to propose a PR. 👍

You would not need the singleton that way, right?

If it's integrated, no indeed. But this will still be needed on IsoCodes library to match IsoCodesInterface.

[enumag]

I don't see why singleton would be necessary for IsoCodesInterface. Can you elaborate the reason? What should the IsoCodesInterface look like?

In this case I really believe that singletons can (and should) be avoided.

[soullivaneuh]

@enumag This one: https://github.com/ronanguilloux/IsoCodes/blob/master/src/IsoCodes/IsoCodeInterface.php

If you have a better implementation, feel free to propose a PR on the related repository (I'm not the maintainer of IsoCodes).

[enumag]

@soullivaneuh Well that's easy. Unless I'm missing something which is always possible of course. ;-)

ronanguilloux/IsoCodes#73

[soullivaneuh]

Resolved by #63.