[RFC] Extract configurables

[umpirsky]

We have some code duplicated across bundles currently.

For checkers and acitons in promotions bundle, we implemented configurable forms that allowed us to configure both checkers and acitons with any configuration set. And create promotions by combining different checkers and actions.

So we have for each of them (checkers and actions) a registry, a configuration form type, a form listener and compiler pass.

We done same thing for shipping and now I am trying to apply same pattern for reports.

I think that it would be nice to extract common functionality into a separate library.

It will probably be some common interfaces, form event listener to add configuration to form.registry, abstract CompilerPass...

Any opinion on this is welcome.

[marcospassos]

I think it will be a good refactoring in favor of code reusability.

[jjanvier]

+1 for the idea, but will you do it ? By creating a new bundle again ?

[umpirsky]

No, I'm thinking about a library (component).

[marcospassos]

@umpirsky, the registry is something that repeats a lot and always has the same behavior. Would be nice abstract this kind of classes and provide interfaces and base classes. To exemplify, I'm thinking in something like the following code for registries:

<?php 

interface Registry
{
    public function register($object);
    public function unregister($object);
    public function has($object);
    public function remove($object);
    public function supports($object);
}

class Registry implements Registry
{
    protected $type;

    public function __construct($type)
    {
        $this->type = $type;
    }

    // methods implementation...

    public function supports($object)
    {
        return $object instanceof $this->type;
    }
}

Thus, add a new registry will be simple like:

<service id="acme.registry.car_type" class="%sylius.common.registry%">
    <argument>%acme.model.car_type%</argument>
</service>

What do you think?

[umpirsky]

Yes. Thats what I had in mind. Maybe even without isSupported, so it will
be generic and type hint on interface.
On 26 Jun 2013 18:55, "Marcos Passos" notifications@github.com wrote:

Sasha, the registry is something that repeat a lot and always has the same
behavior. Would be nice abstract it and provide interfaces and abstract
classes. To exemplify, I'm thinking in something like:

type = $type; } // methods implementation... public function isSupported($object) { return $object instanceof $this->type; }} ``` — Reply to this email directly or view it on GitHubhttps://github.com//issues/202#issuecomment-20063030 .

[marcospassos]

I didn't understand what you meant with "so it will be generic and type hint on interface".

[umpirsky]

<?php 

interface Registerable
{
   public function getType();
   public function getConfigurationFormType();
}

interface RuleCheckerInterface extends Registerable
{
    public function isEligible(PromotionSubjectInterface $subject, array $configuration);
}

interface Registry
{
    public function register(Registerable $object);
    public function unregister(Registerable $object);
    public function has(Registerable $object);
    public function remove(Registerable $object);
}

And you can have generic implementation of Registry interface.

[marcospassos]

Yes, but you are limiting to a very specific type of Registry. Registries are used in many places and most of the times the base code is the same. So, I insist with my suggestion instead of implicit type hinting (just in this case).

[umpirsky]

We can have two implementations then.

Where are they used in Sylius other then for this things I mentioned?

[marcospassos]

Will be used a lot in the customized product and flexible variant and probably in other tag-based situations.

[umpirsky]

Yes, then we can go with your approach, having two implementation would just add complexity. Its up to user then to implement just this suppports() method.

[jjanvier]

Where will this library be available ? Will all the bundles require it ?
It's really a good improvement :)

[pjedrzejewski]

My idea for this registry was exactly as @marcospassos said. Generic registry with a type (interface in most cases) passed as argument. I'd include it in Sylius\Component\DependencyInjection namespace... any better ideas? There are few things to abstract out into libraries too (maybe everything? 👿) - so the question is - what is right repository? I created Components repo, what do you think?

[umpirsky]

@pjedrzejewski Components is fine, but you must create composer package/git mirror for each.

[marcospassos]

IMHO, it is more of bundle than a library. What is the point of creating a library instead of a bundle? @umpirsky why would I use it out of Sylius?

I think that these generic things should be added to a SyliusCommonBundle.

[umpirsky]

@marcospassos I don't see why wouldn't one use it out of Sylius?

[marcospassos]

It's something pretty simple to implement. If I need it in my non-sylius project, probably I would just implement a Registry instead of adding a new library dependency. I'm not sure if I'm understanding your point, can you explain what could we add to this library to be useful for other people out of Sylius?

[pjedrzejewski]

I believe we should abstract as much as possible from bundles to libraries... It's too much work to do it now probably, but with new things, used across many bundles - I think we should use libraries/components. SyliusCommonBundle is an idea I was considering for pretty long time, but my fear is that it will get a total mess. Bundles should have one concrete feature/responsibility.

[marcospassos]

I agree that things that can be clearly reusable by someone else should moved to libraries, using a integration layer (Bundle) to integrate to Sylius (Vespolina is migrating to this approach - a lot of work that seems had frozen the development), but the "CommonBundle" is something related to a few things (and must be "few" otherwise we'll get a total mess) that I can't see why I would use out of Sylius. But maybe I just can't imagine what are your use cases.

About splitting in small bundles, I don't know if it's a good idea, once you will get a ton of bundles with small responsibilities, what is impracticable due performatic issues and some other reasons.

[molchanoviv]

Any changes here? I need something like ComonBundle for #221

[elliot]

Is there any news on this?

There was a Sylius/Components repo for a short period of time but it looks like that has been removed in the latest reorganization.

I was going to have a look at moving the Promo/Tax/Shipping registries over to using the generic component, but with the Components repo now gone has that approach been scrapped?

[pjedrzejewski]

@elliot Nope, we will move it to the Component namespace in this repo. :)

[pjedrzejewski]

Registry part is covered now.

[pjedrzejewski]

For configurable I think DRY does not apply here, cause all these are fairly different. Maybe we can look at it in the future, but for now extracting registry is enough.