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

Only wire ConnectionHelper if it's available #8957

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Aug 24, 2021

DBAL deprecated its ConnectionHelper class, a helper for Symfony console applications. I propose to not use that class if it's not there.

Part of #8884, #8885.

I haven't found any tests nor usage of the createHelperSet() method I've changed here. The other helper that is wired here simply exposes a getEntityManager() method that… well… returns an entity manager. In modern Symfony applications, we would probably use DI to make the EM available to commands. I feel like we should follow the example of DBAL here and deprecate createHelperSet(), EntityManagerHelper and related classes and change our own commands to use DI instead. That could be done in a follow-up PR. WDYT?

Signed-off-by: Alexander M. Turek <me@derrabus.de>
@derrabus derrabus added this to the 2.10.0 milestone Aug 24, 2021
@greg0ire
Copy link
Member

greg0ire commented Aug 24, 2021

Maybe you figured out a lot of the following already, but in case it helps another reviewer understand this, let's go through it anyway.

This area of the code is unfamiliar to me so I did some digging, and found this page of the docs: https://www.doctrine-project.org/projects/doctrine-orm/en/2.9/reference/tools.html#tools

In modern Symfony applications, we would probably use DI to make the EM available to commands.

I think projects impacted by this are not Symfony applications, but non-Symfony applications. Users of these applications can still run doctrine commands by using vendor/bin/doctrine, provided they create a config file.

I haven't found any tests nor usage of the createHelperSet() method I've changed here.

That would be because it's supposed to be called in a file created by the users, not by Doctrine, as documented in the page above.

I think not wiring db anymore might be the right call though, because it's only useful for DBAL commands, and they no longer use it since https://github.com/doctrine/dbal/pull/3956/files#diff-48ddc2d792448b076086b4663a7aad88971526a9cc5ce6275964585258d3745eL124, so as long as we require DBAL 2.11 we should be fine IMO

@greg0ire
Copy link
Member

@PowerKiKi @dmaicher can you please advise about this? I think the BC break that might exist would be on commands manually registered by the user (in the config file). The docs say:

If you want to use additional commands you have to register them yourself.

@derrabus
Copy link
Member Author

In modern Symfony applications, we would probably use DI to make the EM available to commands.

I think projects impacted by this are not Symfony applications, but non-Symfony applications. Users of these applications can still run doctrine commands by using vendor/bin/doctrine, provided they create a config file.

Fair enough, but the bin/doctrine script is code that we control. We can refactor it to use DI instead of that helper without changing the external behavior of the script.

I think not wiring db anymore might be the right call though, because it's only useful for DBAL commands

We don't know if there is code out there that relies on createHelperSet returning a helper set that includes the db helper. Removing the helper entirely is a breaking change. This is why I'm only excluding it if it's not available anymore for now.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

No reaction from the others, let's merge this 👍

@greg0ire greg0ire merged commit a06bbaf into doctrine:2.10.x Aug 25, 2021
@greg0ire
Copy link
Member

Thanks @derrabus !

@derrabus derrabus deleted the remove/connection-helper branch August 25, 2021 18:44
@PowerKiKi
Copy link
Contributor

I assumed you asked for my inputs because I created #8327 a while ago.

My use-case is relatively simple. It is a non-Symfony app (Laminas Mezzio), and I strictly only use commands via ./vendor/bin/doctrine and ./vendor/bin/doctrine-migrations. So I have no direct use of those internal details. As long as the commands from both packages end up working (almost) out of the box with a shared configuration, I'm OK with the merge here.

@greg0ire
Copy link
Member

@PowerKiKi yes, that's why 🙂 I was thinking you would be a relevant reviewer since you are familiar with the binary (I don't use it at all, for instance)

Copy link
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

Sorry I was busy the last days. This change looks good to me 👍

@PowerKiKi
Copy link
Contributor

I just tested this PR and can confirm that the following commands still work as expected (after a few minor adjustments). So it's all good 🎉

  • ./vendor/bin/doctrine-dbal dbal:run-sql 'SELECT 2+9'
  • ./vendor/bin/doctrine dbal:run-sql 'SELECT 2+9'
  • ./vendor/bin/doctrine orm:validate-schema
  • ./vendor/bin/doctrine-migrations migrations:status

@greg0ire
Copy link
Member

Thanks for checking 🙇

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