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

Update commands #1765

Merged
merged 1 commit into from Jul 28, 2020
Merged

Update commands #1765

merged 1 commit into from Jul 28, 2020

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Jul 14, 2020

Subject

Replace ContainerAwareCommand in favor for Command and use DI in favor for container.

I am targeting this branch, because this should be done in this branch but technicly it is BC-break.

Changelog

### Changed
- Change based command class from `ContainerAwareCommand` to `Command` and inject services instead container

@wbloszyk
Copy link
Member Author

Can we add this to 3.x branch and should I add changelog for it?

@phansys
Copy link
Member

phansys commented Jul 14, 2020

I made the same change some time ago at sonata-project/SonataAdminBundle#5579, so I think we could assume the same BC break here.

@jordisala1991
Copy link
Member

Can you add changelog? @phansys do we need an upgrade note about this BC break? No one is suposed to extend those commands and IMO we should consider it as non BC break (as we did on admin bundle) but maybe it is good to add a note

@phansys
Copy link
Member

phansys commented Jul 14, 2020

No one is suposed to extend those commands

Many of our classes are not supposed to be extended, but since it's possible we usually treat them as part of our API.
I agree an upgrade note should help to cover any edge case.

@wbloszyk
Copy link
Member Author

@jordisala1991 @phansys Can you help me with finish this PR? What more I should do?

@jordisala1991
Copy link
Member

You should add an upgrade note to mention the BC break on this PR

src/Command/AddMassMediaCommand.php Outdated Show resolved Hide resolved
src/Command/MigrateToJsonTypeCommand.php Outdated Show resolved Hide resolved
src/Command/MigrateToJsonTypeCommand.php Show resolved Hide resolved
src/Command/MigrateToJsonTypeCommand.php Outdated Show resolved Hide resolved
src/Command/AddMassMediaCommand.php Show resolved Hide resolved
src/Command/AddMassMediaCommand.php Outdated Show resolved Hide resolved
src/Command/BaseCommand.php Outdated Show resolved Hide resolved
src/Command/FixMediaContextCommand.php Outdated Show resolved Hide resolved
src/Model/CategoryManager.php Show resolved Hide resolved
@wbloszyk wbloszyk force-pushed the update_commands branch 2 times, most recently from 6a54322 to be0d9eb Compare July 17, 2020 07:47
@wbloszyk
Copy link
Member Author

In MigrateToJsonTypeCommad entityManager is use for getting connetion. Mybe injection doctrine.dbal.connection will be better option? WDYT? @jordisala1991 @franmomu

@franmomu
Copy link
Member

In MigrateToJsonTypeCommad entityManager is use for getting connetion. Mybe injection doctrine.dbal.connection will be better option? WDYT? @jordisala1991 @franmomu

👍 and I guess as the other one that it's possible that the service is not available.

src/Command/BaseCommand.php Show resolved Hide resolved
@wbloszyk
Copy link
Member Author

@jordisala1991 @phansys Can you check upgrade and changelog notes?

@wbloszyk wbloszyk requested a review from core23 July 18, 2020 10:59
@wbloszyk wbloszyk force-pushed the update_commands branch 2 times, most recently from 510e765 to 6a1d46d Compare July 18, 2020 11:33
src/Command/AddMassMediaCommand.php Outdated Show resolved Hide resolved
UPGRADE-3.x.md Outdated Show resolved Hide resolved
src/Command/MigrateToJsonTypeCommand.php Outdated Show resolved Hide resolved
@wbloszyk wbloszyk force-pushed the update_commands branch 2 times, most recently from 16ca2fd to 5abadb7 Compare July 22, 2020 13:54
@wbloszyk wbloszyk requested a review from phansys July 22, 2020 13:55
@wbloszyk
Copy link
Member Author

https://github.com/doctrine/DoctrineBundle/blob/89535170a0ac4a0ebe8f8f0b950e5d47218bcb36/DependencyInjection/DoctrineExtension.php#L95-L99

if (method_exists(Alias::class, 'setDeprecated')) {
    $container->getAlias('Symfony\Bridge\Doctrine\RegistryInterface')->setDeprecated(true, 'The "%alias_id%" service alias is deprecated, use "Doctrine\Persistence\ManagerRegistry" instead.');
    $container->getAlias('Doctrine\Bundle\DoctrineBundle\Registry')->setDeprecated(true, 'The "%alias_id%" service alias is deprecated, use "Doctrine\Persistence\ManagerRegistry" instead.');
    $container->getAlias('Doctrine\Common\Persistence\ManagerRegistry')->setDeprecated(true, 'The "%alias_id%" service alias is deprecated, use "Doctrine\Persistence\ManagerRegistry" instead.');
}

core23
core23 previously approved these changes Jul 23, 2020
@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

src/Command/FixMediaContextCommand.php Outdated Show resolved Hide resolved
Update tests/Command/RemoveThumbsCommandTest.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update tests/Command/RemoveThumbsCommandTest.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update tests/Command/FixMediaContextCommandTest.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update UPGRADE-3.x.md

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/Command/MigrateToJsonTypeCommand.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/Command/FixMediaContextCommand.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>
@jordisala1991 jordisala1991 merged commit df1dee2 into sonata-project:3.x Jul 28, 2020
@jordisala1991
Copy link
Member

Thank you @wbloszyk

@wbloszyk wbloszyk deleted the update_commands branch July 28, 2020 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants