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

Added initial version of our BC promise #138

Merged
merged 12 commits into from
Nov 29, 2019
Merged

Added initial version of our BC promise #138

merged 12 commits into from
Nov 29, 2019

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented Oct 31, 2019

This is quite an important one so I've requested a few people for review.
Please feel free to notify other Contao devs if you think they would like to extend/modify this document.

@m-vo
Copy link
Member

m-vo commented Oct 31, 2019

Let's talk constructor parameters again.

Personally I think the exact dependencies I'm injecting into a class (that do not leak out via class methods) should be considered implementation detail: If I want to change an algorithm for instance, I'd like to be able to exchange LibraryAGenerator with LibraryBProvider, if I want to improve 'testability', I'd like to insert new dependencies - e.g. a FilesystemAbstraction.

So, when does this hurt anyone? Basically whenever there are calls to my class's constructor with a now changed (and incompatible = no respective optional parameters) signature due to...

  1. ... someone creating an instance of my class.
  2. ... someone calling parent::__construct().

In a DI architecture where classes (services) are instantiated by the container 1) can be neglected. 2) is only the case when inheriting and there are only a few reasons someone should ever do this:

  1. extending an abstract base class (Contao\CoreBundle\Controller\ContentElement\AbstractContentElementController)
  2. extending a non-abstract class that is meant to be a building block (Symfony\Component\Console\Command\Command)
  3. creating a fixture for testing (Contao\CoreBundle\Fixtures\Exception\DerivedPageNotFoundException)
  4. overwriting features of a class (e.g. decorating a service & modifying bits)

Points 1) - 3) are legitimate use cases. 4), however, clearly is a misuse to me (the class is maybe simply not final due to point 4).

So how to work around this? Let's make an example: If I want decorate Doctrine's default AnnotationReader instead of extending and overwriting it I could also implement the Reader interface myself. And If I need to reuse code can inject the existing AnnotationReader and simply delegate calls. This composition pattern works whenever there are appropriate interfaces around.

Long story short, because all of you know where this is going anyway: 😄 I would vote for excluding constructors from BC promise and make some well-thought exceptions to this rule, namely to cover the above building blocks. Note that this is only true for the 'new' code, not the legacy classes inside Resources/contao.

(And imho those exceptions should be visible in code - e.g. with appropriate annotations. )

qzminski
qzminski previously approved these changes Oct 31, 2019
xchs
xchs previously approved these changes Oct 31, 2019
Copy link
Contributor

@xchs xchs left a comment

Choose a reason for hiding this comment

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

👍

docs/dev/internals/bc-promise.md Outdated Show resolved Hide resolved
docs/dev/internals/bc-promise.md Outdated Show resolved Hide resolved
docs/dev/internals/bc-promise.md Outdated Show resolved Hide resolved
@leofeyer
Copy link
Member

I like the initial version and especially the following sentence:

everything that is about integrating Contao into a Symfony application might be subject to change

What we could discuss:

  1. Constructor arguments (see @m-vo's arguments above)
  2. Composer/ScriptHandler (integrating Contao into a Symfony application)
  3. ContaoManager/Plugin (integrating Contao into the Managed Edition)

Co-Authored-By: Hannes <xchs@gmx-topmail.de>
@Toflar Toflar dismissed stale reviews from xchs and qzminski via e23cebf November 1, 2019 08:34
Toflar and others added 3 commits November 1, 2019 09:34
@Toflar
Copy link
Member Author

Toflar commented Nov 1, 2019

Constructor arguments (see @m-vo's arguments above)

I don't think I will ever agree here.

Composer/ScriptHandler (integrating Contao into a Symfony application)

I don't think we need to mention this here. We won't be able to change that in any minor version so that must be covered by BC.

ContaoManager/Plugin (integrating Contao into the Managed Edition)

Added.

@m-vo
Copy link
Member

m-vo commented Nov 1, 2019

don't think I will ever agree here.

Do you mind sharing why?

@Toflar
Copy link
Member Author

Toflar commented Nov 1, 2019

Symfony protects all public methods that obviously includes constructors. Which makes sense because anybody can register their own services and use the classes the way they want.
And that's also the way it should be.
We cannot just adjust the constructor of e.g. a PictureGenerator. I might use my own instance of it with a different instance of ResizerInterface.

richardhj
richardhj previously approved these changes Nov 1, 2019
fritzmg
fritzmg previously approved these changes Nov 1, 2019
@ausi
Copy link
Member

ausi commented Nov 2, 2019

I’m not sure about the constructor arguments. For services it might make sense to exclude them from the BC promise. Should we set this to “up for discussion”?

@leofeyer
Copy link
Member

leofeyer commented Nov 4, 2019

There is no "up for discussion" label in this repo, but we should definitely discuss this. I have set up a reminder for the next call.

@aschempp
Copy link
Member

aschempp commented Nov 4, 2019

I generally like the statement(s). Some thoughts:

@leofeyer leofeyer dismissed stale reviews from fritzmg and richardhj via 46db49c November 21, 2019 14:38
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

As discussed in Mumble on November 28th, we want to add a paragraph about constructors marked as @internal before we merge.

leofeyer
leofeyer previously approved these changes Nov 28, 2019
richardhj
richardhj previously approved these changes Nov 29, 2019
ausi
ausi previously approved these changes Nov 29, 2019

* Our BC promise does not cover classes and methods marked as `@internal`. In most cases this concerns constructors
of services Contao provides. If you want to change the behaviour of a service, do not replace it with your own
instance of the class but instead decorate the original service (see the tip about "Composition over Inheritance").
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +30 to +39
* Because Contao is a Symfony bundle like every other bundle, our BC promise does not cover anything that is about
integrating Contao into the Symfony application. This includes:

* Commands
* Data collectors
* Dependency injection compiler passes
* Event listeners

* Our BC promise also does not cover the `ContaoManager/Plugin` class, which is required to integrate a bundle into
the [Contao Managed Edition][Contao_ME].
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this whole chapter is necessary if we simply mark them as @internal? All of the mentioned class types (except the Plugin) are also services, so they are already covered by you should decorate a service?

@Toflar Toflar dismissed stale reviews from ausi, richardhj, and leofeyer via a1ea177 November 29, 2019 07:57
@Toflar Toflar merged commit 89955e3 into master Nov 29, 2019
@Toflar Toflar deleted the bc-promise branch November 29, 2019 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.