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

closing classes from accidental extension and symfony best practices #416

Merged
merged 1 commit into from
Apr 4, 2019
Merged

closing classes from accidental extension and symfony best practices #416

merged 1 commit into from
Apr 4, 2019

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Mar 12, 2019

symfony recommends to not use parameters for the classes that implement services. i only kept the ones that are actually configurable. everything else should be done with service decorators or custom compiler passes if ever needed, to make things less implicit.

@dbu dbu mentioned this pull request Mar 12, 2019
@dbu
Copy link
Contributor Author

dbu commented Mar 12, 2019

@WyriHaximus as you seem to use sculpin actively, do you maybe have input if i have been too agressive with making classes final? there are interfaces for pretty much everything, so it should be possible to proxy if only a small thing should be customized. a lot of things where protected but did not look suitable to extend

@WyriHaximus
Copy link
Member

@dbu I'm all in favour of make everything (where possible) final 👍

@GwendolenLynch
Copy link
Contributor

A vote in favour of this PR, especially for this:

-   <service id="sculpin_markdown_twig_compat.converter_listener" class="%sculpin_markdown_twig_compat.convert_listener.class%" public="true">
+   <service id="sculpin_markdown_twig_compat.converter_listener" class="Sculpin\Bundle\MarkdownTwigCompatBundle\ConvertListener" public="true">

@dbu This branch needs a rebase now, as some of my recent PRs have caused conflicts 😇

@dbu
Copy link
Contributor Author

dbu commented Mar 29, 2019

rebased and solved the conflicts. glad if we can merge this and #417 soon to not get into conflicts again ;-)

@beryllium beryllium merged commit 8c5beeb into sculpin:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants