Doctrinebundle: Replace outdated jdorn/sql-formatter dependency with doctrine/sql-formatter

[gmponos]

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?

[Ocramius]

I suggest sending a PR there and improving testing first. An old dependency is not necessarily a broken one.

[gmponos]

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.

[Ocramius]

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" [e-mail] 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 https://github.com/jdorn/sql-formatter/pull/100
and it seems that either way travis seems not to be setup
https://github.com/jdorn/sql-formatter/issues/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
https://github.com/doctrine/DoctrineBundle/issues/785#issuecomment-368768615,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAJakGRmiuvZP8XYt15qBCJzPgK8jHbhks5tY6mhgaJpZM4STz0F
.

[gmponos]

Do you think you could revise your opinion about including the class inside doctrine?

[kimhemsoe]

@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?

[gmponos]

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.

1. Sorry but I haven't checked it thoroughly if there are tests inside doctrine bundle that check all the possible scenarios in that class.
2. Having this class/package included in my project makes it available for any developer to use it and I believe that no one likes using code that it is not tested against the PHP version that he is using.

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.

[Majkl578]

Having this class/package included in my project makes it available for any developer to use it

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.

[alcaeus]

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.

[stof]

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:

• syntax highlighting on the displayed query
• pretty-printing (with syntax highlighting) the query in an togglable section

These are nice-to-have features, but we may want to reconsider them entirely if they imply using unmaintained dependencies.

[ostrolucky]

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.

[ostrolucky]

closing as explained, we test it implicitly with our test suite, I have personally written test cases which use it

[ostrolucky]

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.

Thanks to @greg0ire and @goetas

[dmaicher]

Maybe we can wait until https://github.com/doctrine/dbal-bundle will be done :smile: backporting so many changes will be a pain otherwise

[ostrolucky]

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.

[greg0ire]

@dmaicher I think the PR to switch to the new lib will be quite small. Besides, I see no mention of SqlFormatter in dbal bundle, but maybe you plan to add it later?

We can wait a bit with these things, but not indifinitely.

If I can be of some help, tell me

[ostrolucky]

doctrine/sql-formatter has a released version. The way I see it, this is ready to implement in doctrine-bundle.

[greg0ire]

doctrine/sql-formatter requires php 7.2, so it will be ready when we drop php 7.1 from this bundle IMO

[ostrolucky]

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.

[greg0ire]

Why not? The 1.0.0 tag is here to stay and will still be available if the bundle drops 7.1
Also, it's not very clear to me why we couldn't drop 7.1 soon… I mean it's not supported anymore, is it? Or is it something about LTS versions of whatever distro still relying on it? I don't think that's the case anymore, is it?

[xabbuh]

@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.

[greg0ire]

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 ^0.1 || ^1.0 here.

[greg0ire]

https://github.com/doctrine/sql-formatter/pull/54

[greg0ire]

I'm working on closing this.

[gmponos]

Thank you for working on this issue :)