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
Replace outdated jdorn/sql-formatter dependency with doctrine/sql-formatter #785
Comments
I suggest sending a PR there and improving testing first. An old dependency is not necessarily a broken one. |
I would have done that but the latest commit is at 2015. I thought that it would be better for doctrine not to have a dependency to a package like that only for one class. Anyway I opened the PR and it seems that either way travis seems not to be setup. Concluding I believe there are more benefits of copying the class. |
I have projects that run and have no commits since 2013: not a reason to
assume they aren't stable.
…On 27 Feb 2018 08:07, "Mponos George" ***@***.***> wrote:
I suggest sending a PR there and improving testing first. An old
dependency is not necessarily a broken one.
I would have done that but the latest commit is at 2015. I thought that it
would be better for doctrine not to have a dependency to a package like
that only for one class.
Anyway I opened the PR <jdorn/sql-formatter#100>
and it seems that either way travis seems not to be setup
<jdorn/sql-formatter#101>.
Concluding I believe there are more benefits of copying the class.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#785 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakGRmiuvZP8XYt15qBCJzPgK8jHbhks5tY6mhgaJpZM4STz0F>
.
|
Do you think you could revise your opinion about including the class inside doctrine? |
@gmponos May i ask what the issue is? Yes it is old and not tested against newer php versions. But what issues or problems have you experienced? |
No problems. But I am not sure or you may say that I don't feel confident that this class always work with the latest php version.
May I also state here that you do not update travis with the latest version of PHP only when there is a problem/bug. Hence I believe that my concern here makes sense regardless how simple or not a code might be. |
No one should ever depend on your package's dependencies, otherwise when you remove such dependency, their code will break. They should have it as their own explicit dependency in such case. I think we can query the author about the status in long-term and see. I am not a fan of legacy or unmaintained dependencies either, on the other hand, this is a self-contained one-class package (used only for debugging purposes IIRC?), so the risk of breakages is really low. |
Yeah, looks like that package is effectively dead, with the last commit to the repo a few years ago. We'll have to discuss this internally: whether to fork the current package and maintain it ourselves or whether we're going to use any other formatter option. |
A quick search on packagist (https://packagist.org/?query=sql%20format) gives me a few alternatives which are as much dead as that one. We are using this formatter for 2 things:
These are nice-to-have features, but we may want to reconsider them entirely if they imply using unmaintained dependencies. |
We test compatibility of this library with newer php versions by proxy. It works. Unless we want to enhance this library, I don't see why copy it. But I would sleep better if somebody else in PHP ecosystem took the role of continuing maintainance of this (or successor) library. |
closing as explained, we test it implicitly with our test suite, I have personally written test cases which use it |
So we are doing this after all, check https://github.com/doctrine/sql-formatter. Reopening this to keep track of, so we can integrate it in here. |
Maybe we can wait until https://github.com/doctrine/dbal-bundle will be done 😄 backporting so many changes will be a pain otherwise |
dbal-bundle could have been a fork of doctrine-bundle, then backporting would be just git merges. Anyways I see only initial commit in dbal-bundle, so what's the timeline? We can wait a bit with these things, but not indifinitely. |
@dmaicher I think the PR to switch to the new lib will be quite small. Besides, I see no mention of
If I can be of some help, tell me |
doctrine/sql-formatter has a released version. The way I see it, this is ready to implement in doctrine-bundle. |
|
Well that will happen after we drop symfony 4 support here. Till then library will probably bump php version further, so unless we change this, the way I see it, it's unlikely we will adopt this library here ever. |
Why not? The 1.0.0 tag is here to stay and will still be available if the bundle drops 7.1 |
@greg0ire Symfony 4.4 (providing support until November 2022) supports PHP 7.1. Which means that we would have to maintain two versions of DoctrineBundle then. |
What a bummer! I wish Symfony dropped versions in the red zone. I don't like being too aggressive on version constraints but this is a bit too much for my taste. Anyway, I think since DoctrineBundle is one of the 2 big clients of sql-formatter, I need to support it there. I'm going to create a 0.x branch that will only accept security fixes and support 7.1, and tag 0.1.0 from it. That way we can use |
I'm working on closing this. |
Thank you for working on this issue :) |
Hello,
I can see that DoctrineBundle uses the jdorn/sql-formatter package.
If you check the travis file of the latest tagged version the latest version of the PHP tested against is PHP5.4
Do you believe it would be better to copy this library under the doctrine/common package and drop the dependency to this package?
The text was updated successfully, but these errors were encountered: