Port commands for Symfony 4.2 (follows #876) #877

dunglas · 2018-11-15T23:48:42Z

Follows (and needs) #876.

It works, but it introduces several BC breaks:

Commands doesn't extend ContainerAwareCommand anymore (as this class is deprecated)
Some constructor parameters are now mandatory (because we use DI instead of injection the container)

Making the commands fully BC would be possible by using hacks like this one to introduce some conditional inheritance, but I'm not sure that it is worth it: ContainerAwareCommand commands were designed to be initialized by the framework itself.

Also, to prevent future similar issues, I marked these commands @final, and the abstract DoctrineCommand as @internal.

Resources/config/orm.xml

dunglas · 2018-11-16T15:52:19Z

It looks like dependencies are not injected for SF 2.8 and 3.2, any idea of what's going on (@chalasr maybe)?

Resources/config/dbal.xml

Resources/config/orm.xml

dunglas · 2018-11-23T18:24:34Z

All green

kimhemsoe · 2018-11-29T10:12:25Z

Is this ready?

dunglas · 2018-11-29T10:45:43Z

it is. Just beware of the tiny BC break: commands don't extend ContainerAwareCommand anymore.

alcaeus · 2018-11-29T13:01:11Z

it is. Just beware of the tiny BC break: commands don't extend ContainerAwareCommand anymore.

Are we still exposing the methods from ContainerAwareCommand or are those no longer there? Wondering about the impact of the BC break. How did Symfony take care of this?

chalasr · 2018-11-30T01:21:11Z

@alcaeus no, they are not exposed anymore.

Symfony core commands stopped extending this class in 3.4 but kept a BC layer until 4.0 which was:

all added constructor args are nullable at first, triggering deprecation notices if given null
the setContainer() and getContainer() methods are copy-pasted into commands and deprecated with notice
calls to getContainer are preserved in case dependencies are not wired via constructor args

It could be worth using the same approach here.

alcaeus

We either need to make master 2.0.x-dev before merging this or make these changes backwards compatible. These classes are covered by the BC promise.

dunglas · 2018-11-30T06:29:01Z

@chalasr, as far as I understand, what you suggest limit the BC break, but doesn’t prevent it totally: $command instanceof ContainerAwareCommand will now be false, and it may break existing code.

To limit the BC break, I’ve marked the commands as final (but it hasn’t been done in an earlier release, it’s kind of useless for the next version).

Bumping the major version just for that looks a bit overkill, but it’s the only solution I have in mind to be fully semver compliant.

chalasr · 2018-11-30T08:18:06Z

@dunglas Yea, that would just considerably reduce the impact

@alcaeus would the suggested layer be enough to merge this in 1.x? We can’t avoid the type change

alcaeus · 2018-11-30T08:41:12Z

Agree on making the classes "final" using @final - since it's documentation only it's just a suggestion so no BC break. We should follow up on that by making them final in the next major release unless there's a good point not to do so. This would help prevent such situations in the future.

I also agree that tagging a major version for this is overkill, but I'm worried about breaking BC. This could happen if someone extended the commands in their projects and relied on $this->getContainer() being present, in which case they'd have a fatal error after upgrading. It could also happen if someone instantiated the commands themselves without passing the now required constructor options. We should definitely prepare a BC layer for the latter, maybe we can even build a replacement for getContainer(). I can live with the inheritance chain being "broken".

kimhemsoe · 2018-11-30T09:24:40Z

@weaverryan Is this BC break ok with you guys?

chalasr · 2018-11-30T12:01:22Z

BC layer added, PR rebased. Ready on my side (build failure seems unrelated)

Majkl578

Please fix coding standard violations. I restarted the stage manually, you can see it here: https://travis-ci.org/doctrine/DoctrineBundle/jobs/461744294

Majkl578 · 2018-11-30T12:23:17Z

Command/DoctrineCommand.php

+     */
+    public function setContainer(ContainerInterface $container = null)
+    {
+        @trigger_error(sprintf('The "%s()" method is deprecated and will be removed in DoctrineBundle 2.0.', __METHOD__), E_USER_DEPRECATED);


As per Doctrine's Deprecation Policy, silenced deprecations should not be used - @deprecated annotation should be used instead.

Since we're dealing with a Symfony Bundle here, it makes sense to use the Symfony way of dealing with deprecations, don't you think?

Note that silenced deprecations are used elsewhere in the code base already.

don't you think?

Not convinced, just like with CS - it sits under Doctrine GH org. We had this discussion many times already, with @Ocramius too.

Ok I won't fight here, deprecation notices removed.

chalasr · 2018-11-30T12:55:14Z

@Majkl578 CS fixed, thanks.
The remaining failure https://travis-ci.org/doctrine/DoctrineBundle/jobs/461761448 is due to the incompatibility between symfony/doctrine-bridge:4.1 and symfony/messenger:4.2, not related to this PR.

alcaeus

Thanks @chalasr!

kimhemsoe · 2018-11-30T13:53:35Z

Thanks @dunglas and @chalasr

dunglas · 2018-11-30T13:59:39Z

@kimhemsoe thanks for the merge! Can you tag a release too please (Symfony 4.2 has been released, so the current version triggers deprecations).

kimhemsoe · 2018-11-30T15:00:12Z

@dunglas Done :)

dunglas mentioned this pull request Nov 15, 2018

Fix Symfony 4.2 deprecations, and Travis build #876

Merged

dunglas force-pushed the commands-sf42 branch from f9e03e8 to 6901617 Compare November 15, 2018 23:58

dunglas changed the title ~~Port commands for Symfony 4.2 (follows )~~ Port commands for Symfony 4.2 (follows #876) Nov 16, 2018

stof requested changes Nov 16, 2018

View reviewed changes

Resources/config/orm.xml Outdated Show resolved Hide resolved

Resources/config/orm.xml Outdated Show resolved Hide resolved

kimhemsoe added this to the 1.9.2 milestone Nov 16, 2018

chalasr reviewed Nov 17, 2018

View reviewed changes

Resources/config/dbal.xml Outdated Show resolved Hide resolved

chalasr reviewed Nov 17, 2018

View reviewed changes

Resources/config/orm.xml Outdated Show resolved Hide resolved

kimhemsoe self-assigned this Nov 23, 2018

chalasr approved these changes Nov 29, 2018

View reviewed changes

alcaeus requested changes Nov 30, 2018

View reviewed changes

dunglas added 5 commits November 30, 2018 12:51

Port commands for Symfony 4.2 (follows doctrine#876)

af81538

Move DBAL commands to dbal.xml

557ceab

Fix command definitions

8a7fe90

Fix CS

e8923b6

Fix CS

f2abf93

chalasr force-pushed the commands-sf42 branch from f069ec6 to 489e5d3 Compare November 30, 2018 11:56

Majkl578 suggested changes Nov 30, 2018

View reviewed changes

chalasr force-pushed the commands-sf42 branch from 489e5d3 to c94a199 Compare November 30, 2018 12:46

Add BC layer for container-aware commands

b69d78a

chalasr force-pushed the commands-sf42 branch from c94a199 to b69d78a Compare November 30, 2018 13:03

alcaeus approved these changes Nov 30, 2018

View reviewed changes

alcaeus requested a review from Majkl578 November 30, 2018 13:32

alcaeus mentioned this pull request Nov 30, 2018

Compatibility with Symfony 4.2 #880

Closed

kimhemsoe merged commit 82d2c63 into doctrine:master Nov 30, 2018

dunglas deleted the commands-sf42 branch November 30, 2018 13:58

This was referenced Nov 30, 2018

DoctrineCommand class extends the deprecated ContainerAwareCommand class #863

Closed

Remove dependency on symfony/framework-bundle #842

Closed

alcaeus mentioned this pull request Apr 4, 2019

Add deprecation notices for container in DoctrineCommand #946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port commands for Symfony 4.2 (follows #876) #877

Port commands for Symfony 4.2 (follows #876) #877

dunglas commented Nov 15, 2018

dunglas commented Nov 16, 2018

dunglas commented Nov 23, 2018 •

edited

kimhemsoe commented Nov 29, 2018

dunglas commented Nov 29, 2018

alcaeus commented Nov 29, 2018

chalasr commented Nov 30, 2018 •

edited

alcaeus left a comment

dunglas commented Nov 30, 2018 •

edited

chalasr commented Nov 30, 2018

alcaeus commented Nov 30, 2018

kimhemsoe commented Nov 30, 2018

chalasr commented Nov 30, 2018

Majkl578 left a comment

Majkl578 Nov 30, 2018

alcaeus Nov 30, 2018

chalasr Nov 30, 2018

Majkl578 Nov 30, 2018

chalasr Nov 30, 2018

chalasr commented Nov 30, 2018

alcaeus left a comment

kimhemsoe commented Nov 30, 2018

dunglas commented Nov 30, 2018

kimhemsoe commented Nov 30, 2018

Port commands for Symfony 4.2 (follows #876) #877

Port commands for Symfony 4.2 (follows #876) #877

Conversation

dunglas commented Nov 15, 2018

dunglas commented Nov 16, 2018

dunglas commented Nov 23, 2018 • edited

kimhemsoe commented Nov 29, 2018

dunglas commented Nov 29, 2018

alcaeus commented Nov 29, 2018

chalasr commented Nov 30, 2018 • edited

alcaeus left a comment

Choose a reason for hiding this comment

dunglas commented Nov 30, 2018 • edited

chalasr commented Nov 30, 2018

alcaeus commented Nov 30, 2018

kimhemsoe commented Nov 30, 2018

chalasr commented Nov 30, 2018

Majkl578 left a comment

Choose a reason for hiding this comment

Majkl578 Nov 30, 2018

Choose a reason for hiding this comment

alcaeus Nov 30, 2018

Choose a reason for hiding this comment

chalasr Nov 30, 2018

Choose a reason for hiding this comment

Majkl578 Nov 30, 2018

Choose a reason for hiding this comment

chalasr Nov 30, 2018

Choose a reason for hiding this comment

chalasr commented Nov 30, 2018

alcaeus left a comment

Choose a reason for hiding this comment

kimhemsoe commented Nov 30, 2018

dunglas commented Nov 30, 2018

kimhemsoe commented Nov 30, 2018

dunglas commented Nov 23, 2018 •

edited

chalasr commented Nov 30, 2018 •

edited

dunglas commented Nov 30, 2018 •

edited