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

Extract API from Implementation #6129

Closed
webmozart opened this issue Nov 27, 2012 · 72 comments
Closed

Extract API from Implementation #6129

webmozart opened this issue Nov 27, 2012 · 72 comments
Labels
Form RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@webmozart
Copy link
Contributor

Hi everyone,

We had this discussion before, but at a point when we didn't have composer yet. I would like to bring this topic up again now.

Problem (updated December 5, 2012)

I just had a longer discussion with @fago, who is currently integrating the Validator component into Drupal. For Drupal it is necessary to replace the translation mechanism used in the Validator component by a custom implementation. Even when implementing Symfony\Component\Translation\TranslatorInterface in their code, this still pulls in all of the Symfony Translation implementation (and its potential dependencies).

The same is true for any Symfony component/bundle/bridge. When someone wants to use A which relies on B's interfaces, and B relies on C and D, he needs to pull in A, B, C and D and provide a custom implementation for B (that might depend on E and F). Lots of installed dependencies: A, B, C, D, Bcustom, E, F

         A
      .·´ `·.
     B     Bcustom
   .´`.     .´`.
  C    D   E    F
Proposal (updated January 7, 2013)

I would like to suggest and discuss a backwards compatible extraction of the Symfony API. In a nutshell, make it possible to use A which relies on B's interfaces but replace B with a custom implementation (i.e. only install A, Bcustom, E, F).

      A
      |
   Bcustom
    .´`.
   E    F
Implementation
api/
    Symfony/
        Component/
            Translation/
                TranslatorInterface.php
    composer.json [symfony/api]
src/
    Symfony/
        Component/
            Translation/
                Translator.php
                composer.json [symfony/translation, requires symfony/api, provides symfony/translation-implementation]
            Validator/
                composer.json [symfony/validator, requires symfony/api, suggests symfony/translation-implementation]
Example: Replacing the translator
Drupal/
    Translation/
        composer.json [drupal/translation, requires symfony/api, provides symfony/translation-implementation]

The package MUST NOT add symfony/translation to its "replace" section. Multiple implementations of the same interface are NOT mutually exclusive and CAN be used at the same time.

Example: Using the Validator with its simplistic default translator
{
    "require": {
        "symfony/validator": "2.2.*"
    }
}
Example: Using the Validator with the Symfony Translation component
{
    "require": {
        "symfony/validator": "2.2.*",
        "symfony/translation": "2.2.*"
    }
}
Example: Using the Validator with a custom implementation
{
    "require": {
        "symfony/validator": "2.2.*",
        "drupal/translation": "..."
    }
}

The API would comprise a closed set of interfaces and classes of the components, that is:

  • the top-level interface(s)
  • the classes and interfaces that the top-level interface(s) refer to in type hints and @return tags
  • the classes and interfaces that these classes and interfaces refer to
  • etc.

Because of composer, this change would be fully BC.

Benefits (updated December 5, 2012)
  1. A Symfony component A can rely on interfaces of another component B without forcing the user to use B.
  2. Other projects can depend on Symfony's interfaces with a lower barrier to entry.
  3. The API clearly documents the stable part of the components (it is ideally guaranteed not to change)
  4. The separation helps us to decouple our components more. For example, if a component A instantiates a class of another component B and thus requires "symfony/b", we can improve it to make use of DI so that it only depends on "symfony/api" instead.
Drawbacks (updated January 8, 2013)
  1. One more composer package (symfony/api) has to be downloaded when using a Symfony component.
  2. Core developers have to maintain interfaces in a separate directory.

Please let me know what you think.

@marijn
Copy link

marijn commented Nov 27, 2012

👍

1 similar comment
@iambrosi
Copy link
Contributor

+1

@havvg
Copy link
Contributor

havvg commented Nov 27, 2012

For anybody not being allowed to use composer this will (worst case) double the amount of dependencies.
I don't see an actual benefit from it. What's wrong with having an implementation within the component? I mean it's about shipping some KB source code, or did I miss the point?

@fago
Copy link
Contributor

fago commented Nov 27, 2012

Having the interface separated would be really useful. In fact, we even think about extracting it ourselves for Drupal - see http://drupal.org/node/1852106.

@havvg: Having a bunch of unused source code lying around which is actually duplicating existing functionality isn't very nice. The duplicated code is potentially confusing for newcomers who try to wrap their heads around the source code as it isn't immediatelly clear that the implementation is not used.

@tPl0ch
Copy link

tPl0ch commented Nov 27, 2012

👍 for separation of API

@DavidBadura
Copy link
Contributor

+1

@adrienbrault
Copy link
Contributor

@fago What about symfony developers who'd have to look into 2 differents directories ?

👎 because IMHO saving some KB doesn't justify complexifying the directory structure.

@webmozart
Copy link
Contributor Author

@adrienbrault The more complex directory structure is mainly relevant for us core developers, and then only in part, because the interface certainly will change much less frequently than the implementation.

@willdurand
Copy link
Contributor

-1 for the same reasons @havvg mentioned.

@lolautruche
Copy link
Contributor

This is indeed very interesting, but then we're not really talking about API, but SPI (Service Provider Interface). We already have this approach in eZ Publish 5 (though not separated in Composer), so I'm obviously +1 😃 .

@webmozart
Copy link
Contributor Author

I added an example above on how this would make it possible to replace Symfony packages by custom implementations.

@webmozart
Copy link
Contributor Author

Updated the description to be less vague (I hope).

@lsmith77
Copy link
Contributor

its certainly interesting .. i am hoping that we will have a general trend where PSR will provide those interface packages rather than Symfony2.

@trompette
Copy link
Contributor

It shouldn't be done by changing the structure of the source code.
Either the interfaces are standard (as @lsmith77 stated) or there is a way to programmatically "replace" an interface.

@Seldaek
Copy link
Member

Seldaek commented Nov 28, 2012

I am kind of -1, it will duplicate the amount of symfony packages you have installed for not so much gain. If it's done in a generalized manner for all components I'm definitely -1. If targetted to a few specific components maybe, but I am not sure where we would draw the line.

@magnusnordlander
Copy link
Contributor

I'm +1, but wouldn't a possible compromise be to store the interfaces both in a separate API package, and in the Symfony component package, and mark the component as a replacement for the API package?

It would make it a bit more of a hassle when the interfaces are changed (but still possible to automate), but it's not THAT different from the component subtree splits on GitHub.

@webmozart
Copy link
Contributor Author

@Seldaek Could the people who vote -1 please also propose an alternative? The use case to replace a core component apparently exists, so we need to find a solution.

@DavidBadura
Copy link
Contributor

@Seldaek Maybe a package called "symfony-api", that includes all api interfaces?

@Seldaek
Copy link
Member

Seldaek commented Nov 28, 2012

Let's be clear that replacing a core component is already possible. I read @fago's point and I can appreciate that, but I don't agree that this is the best solution.

Maybe having one Symfony\Interface package with all the main interfaces that really glue stuff together would be ok. I'd guess we only need 5-10 interfaces in there, and the components needing them could require it. That way we don't have an explosion of packages/complexity, but we still provide some amount of separation for those that really need it. Most likely the drupal guys would anyway need most of the other interfaces that are put in there, so combining them is not a huge problem.

Edit: Eh well, @DavidBadura beat me to it :)

@fspillner
Copy link

I'm kind of -1 and don't like the Symfony\Interface package, too.

An alternative: Maybe this cannot work, but just an idea: Let's pimp Composer to be able to download only certain packages / directories like this for example (I hope, @Seldaek doesn't get headache, after he has read this suggestion ;-)):

{
    "require": {
        "symfony/translation": {
            "version": "2.1.4", 
            "directories": ["Interface/"]
        }
    }
}

All interfaces of Translation component are moved into Interface directory.

@schmittjoh
Copy link
Contributor

Do a few classes more or less really matter that much?

On Sun, Dec 2, 2012 at 10:37 PM, Fabian Spillner
notifications@github.comwrote:

I'm kind of -1 and don't like the Symfony\Interface package, too.

An alternative: Maybe this cannot work, but just an idea: Let's pimp
Composer to be able to download only certain packages / directories like
this for example (I hope, @Seldaek https://github.com/Seldaek doesn't
get headache, after he has read this suggestion ;-)):

{
"require": {
"symfony/translation": {
"version": "2.1.4",
"directories": ["Interface/"]
}
}}


Reply to this email directly or view it on GitHubhttps://github.com//issues/6129#issuecomment-10935630.

@dlsniper
Copy link
Contributor

dlsniper commented Dec 2, 2012

I'm also -1 for this. Besides skipping a few files what's the point?

If it's not extensible for Drupal or any other project it means it's not extensible for many other projects so maybe we need to have a look on how to change that rather that change the whole structure of the framework.

Splitting the structure now would confuse the new developers even more and to be honest it would be really annoying to have the files split like that :)

@lennerd
Copy link

lennerd commented Dec 3, 2012

-1

Agree on @dlsniper.

@seiffert
Copy link
Contributor

seiffert commented Dec 3, 2012

Why do you think that this would confuse? How often do you change an API method and touch both files - interface and implementation? And as a beginner, you don't dive into the core components on your first day...

Separating interfaces and implementation is a good idea IMO. I would even say that interfaces should always be designed from the client's point of view and therefore be placed outside of the implementation's structure.

@schmittjoh
Copy link
Contributor

There might be some value in extracting the top-most interfaces of a
hierarchy (like the TranslatorInterface), but any interface that is only
required by a certain implementation of another, higher-level interface
(like for a example a TranslationLoaderInterface) does not make sense to
extract imo.

On Mon, Dec 3, 2012 at 1:13 PM, seiffert notifications@github.com wrote:

Why do you think that this would confuse? How often do you change an API
method and touch both files - interface and implementation? And as a
beginner, you don't dive into the core components on your first day...

Separating interfaces and implementation is a good idea IMO. I would even
say that interfaces should always be designed from the client's point of
view and therefore be placed outside of the implementation's structure.


Reply to this email directly or view it on GitHubhttps://github.com//issues/6129#issuecomment-10950561.

@webmozart
Copy link
Contributor Author

@schmittjoh Your last statement is a bit unclear. It would be necessary to extract a closed set of interfaces and classes, i.e., the topmost interface, all interfaces and classes that this interface refers to (via type hint or @return annotation), all interfaces and classes that these interfaces and classes refer to etc.

Of course, if an interface is not referenced in any way from the topmost interface, it's not necessary to include it in the API.

A single "symfony/api" package would be fine for me (but using the same namespaces that we have now).

@dlsniper
Copy link
Contributor

dlsniper commented Dec 3, 2012

So if I understand correctly, basically we should have:
namspace Symfony\Component\Yaml;

interface YamlInterface {}

but in the file symfony/api/Symfony/Component/Yaml/YamlInterface.php
and used in symfony/src/Symfony/Component/Yaml/Yaml.php for the framework

and then a vendor could write into his lib something like:

namespace Symfony\Component\Yaml
Yaml implementsYamlInterface
under vendor/X/src/Yaml.php in order to replace the functionality but maintain the interface?

Sorry, I really don't understand the use case and I want to learn more about this :) (if the example is not easy to understand I'll write a gist later on)

Thanks!

@cordoval
Copy link
Contributor

👎 I think maybe this problem happens because of coupling between components. Also if someone is using just one package then why do i need all Symfony interfaces? is loading extra files I don't need. So that cons the point from @fago coming from Drupal. Some problem similar happens with the DI and Config component and how Drupal is not putting the last one under vendors folder #10920 (comment). Drupal is already hell heavy so I don't see the point of loading some extra classes. 👎

@harikt
Copy link

harikt commented Nov 22, 2014

Hi @webmozart ,

This is a great proposal and I love to see this. I have seen many of the core people disagree here, but you should consider the advantage of this than saying hard to look that code. API's once finalized will not change like implementations.

Thank you.

@linaori
Copy link
Contributor

linaori commented Nov 22, 2014

I would like to see a generic container interface, event dispatcher interface, response/request perhaps.. But that's not up to Symfony. I think this should be done via PSR and can then be implemented backwards compatible

@unkind
Copy link
Contributor

unkind commented Nov 22, 2014

As I've mentioned in #12544 and #12115, if you want to use some generic service that Symfony doesn't have yet (e.g. service which gets source code of closure), you are in difficult position as you can't add interface in any of existing components. It looks like separate package with API solves my problem.

@harikt
Copy link

harikt commented Nov 22, 2014

Hi @unkind ,

I feel the one you have discussed in #12115 is different .

@unkind
Copy link
Contributor

unkind commented Nov 22, 2014

@harikt well, I was talking about last comment in #12115. ClosureDumperInterface requires by both DependencyInjection and Routing components and there is no reason to put it in one of them.

Current structure forces to couple interface with one of the components.

Yet another example: Symfony has Symfony\Component\DependencyInjection\LazyProxy\Instantiator\InstantiatorInterface. This is a very specific interface for the DependencyInjection component. You are not able to use @Ocramius's ProxyManager somewhere else without creating yet another too specific interface with moving part of component-related logic to the Bridge.

So it looks like a good idea for me. 👍

@unkind
Copy link
Contributor

unkind commented Dec 7, 2014

More examples:

  1. Symfony doesn't have Cache component, but some generic API would be useful in order to avoid explicit dependency on Doctrine Cache. Doctrine is just implementation detail.
  2. Lock component request ([Feature] Lock component #9357) was rejected because "there are already solutions to this problem". But later LockHandler was merged in 2.6 as part of Filesystem component. That's a little bit weird for me as you could reject it with the same reason. File-based lock handler is a nice feature, but it's an implementation detail as well. It doesn't work for multiple servers, for instance, so it could be a reason of scaling problem in the future. Lock handler interface is a good trade-off in this case, IMHO.

@ostrolucky
Copy link
Contributor

ostrolucky commented Jul 16, 2017

I think decoupling/modularity is one of the goals of Symfony 4.0? I think this is perfect opportunity to reopen discussion about this. I welcome idea for something like symfony/contracts, it would help php ecosystem to adopt established interfaces instead of writing their own. Ping @fabpot!

@xabbuh
Copy link
Member

xabbuh commented Jul 17, 2017

I am not sure if this is something that Symfony should do. For reusable interfaces across projects there already is the PHP FIG. This group is doing a good job in creating standards for components that need a common interface and we were always fast in implementing those interfaces.

@ostrolucky
Copy link
Contributor

ostrolucky commented Jul 17, 2017

You are probably right, this is ideal job for php-fig, even though I'm not confident in their activity level.

Anyway I investigated current situation and picked interfaces which might seem useful for general purpose. I also found some which might seem they should belong to the list but are IMO too coupled to implementation:

\Symfony\Component\EventDispatcher\EventDispatcherInterface
\Symfony\Component\Lock\LockInterface
\Symfony\Component\PropertyAccess\PropertyAccessorInterface
\Symfony\Component\Serializer\SerializerInterface
\Symfony\Component\Serializer\Encoder\DecoderInterface
\Symfony\Component\Serializer\Encoder\EncoderInterface
\Symfony\Component\Serializer\Normalizer\NormalizerInterface
\Symfony\Component\Serializer\Normalizer\DenormalizerInterface
\Symfony\Component\Serializer\NameConverter\NameConverterInterface
\Symfony\Component\Templating\EngineInterface
\Symfony\Component\Translation\TranslatorInterface
\Symfony\Component\Validator\Validator\ValidatorInterface

As you see in the end there isn't many of them. It's mostly interfaces of Serializer, because they are lean enough.

Also I checked coupling of Validator to Translator and it seems it can be decoupled?

Anyway I would like to put the end to this old RFC and see official decision.

@xabbuh
Copy link
Member

xabbuh commented Jul 17, 2017

👎 for me to make this change.

@symfony/deciders What is your opinion on this topic?

@javiereguiluz
Copy link
Member

If I'm right, this is what Laravel does in its "Contracts" package, which contains all the interfaces used in their framework: https://github.com/laravel/framework/tree/5.4/src/Illuminate/Contracts

@unkind
Copy link
Contributor

unkind commented Jul 17, 2017

@ostrolucky if one doesn't use Symfony at all, I don't see reason to use Symfony contracts. Symfony interfaces don't magically make things better. Moreover, they are designed for Symfony needs and they probably won't satisfy your needs later, so you restrict yourself for no reason. Why people are so obsessed with frameworks so they want to couple their projects with them as much as they can? What's the problem to make your own contracts?

https://speakerdeck.com/jakzal/decoupling-from-the-framework — probably, you may find it interesting (by the way, @jakzal is one of the Symfony maintainers).

@ostrolucky
Copy link
Contributor

ostrolucky commented Jul 17, 2017

Why people are so obsessed with frameworks so they want to couple their projects with them as much as they can? What's the problem to make your own contracts?

This makes me believe you don't understand point of this proposal at all? It's not coupling to a framework but opposite. Currently when you depend on Symfony interface you need to pull all components that depend on implementation. Point of not writing own interface is to allow people of your project to swap implementation with some other (that is already made) without writing adapter. For example Doctrine could adopt many interfaces from symfony serializer and you would be able to use implementations across these projects as you seem fit.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 17, 2017

I think there is value in having shared interfaces, as the FIG does.
In Symfony's case, some interfaces could be worth moving outside of their component (eg ServiceSubscriberInterface from DI)
BUT the FIG would be a better place to publish them.
I think right now, Symfony didn't embrace the mission of providing generic interfaces on their own.
Until that happens (if it does at all), I'm 👎 also because that'd mean added maintenance.

Instead, I strongly suggest people to contribute to the FIG to move useful stuff there.

@javiereguiluz
Copy link
Member

I'm closing this as "won't fix" because at least two core members voted against it. Thank you all for the discussion!

@unkind
Copy link
Contributor

unkind commented Jul 17, 2017

It's not coupling to a framework but opposite
Currently when you depend on Symfony interface you need to pull all components that depend on implementation.

We don't understand each other, because we use this term differently. I mean coupling between classes/interfaces, you talk about, in fact, composer packages. So when you start to use Symfony interfaces directly in your project you make coupling with Symfony. Symfony is a tool with its own restrictions, models, etc.

Let's say you depend on EventDispatcherInterface in your project. You decided to make your events immutable and exclude setName, stopPropogation from the Event? You can't do it anymore. Because it's not your contracts. So what's next? You come here and ask to change Symfony contracts, because it doesn't satisfy your needs now: #21827. So, you first couple with Symfony as much as possible and then you come to "fix" Symfony, because it doesn't meet your internal requirements.

@nicolas-grekas
Copy link
Member

Continued in #27093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Form RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests