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

Introduce Doctrine\DBAL\Migrations\AbstractMigration deprecation. #690

Merged
merged 3 commits into from Jun 6, 2018

Conversation

Projects
None yet
2 participants
@jwage
Copy link
Member

jwage commented May 18, 2018

Q A
Type improvement
BC Break no
Fixed issues Fixes #642

Summary

Prepare for the 2.0 release and deprecate things in 1.8

  • Introduce forward compatibility layer for Doctrine\Migrations\AbstractMigration
  • Deprecate Doctrine\DBAL\Migrations\AbstractMigration
  • Update migration generation to use Doctrine\Migrations\AbstractMigration

@jwage jwage force-pushed the abstract-migration-deprecation branch 4 times, most recently from 231e0bb to d4f7f48 May 18, 2018

@jwage jwage self-assigned this May 18, 2018

@jwage jwage added this to the 1.8 milestone May 18, 2018

@jwage jwage requested a review from stof May 18, 2018

@jwage jwage added the Improvement label May 18, 2018

@stof

This comment has been minimized.

Copy link
Member

stof commented May 31, 2018

Do we have any place typehinting AbstractMigration or having it as a return type in a public API ? If yes, the current migration path (making the old class extend the new one) is a BC break.

@jwage

This comment has been minimized.

Copy link
Member Author

jwage commented May 31, 2018

I don't think we do, but why would it be a BC break? The type hint would still work.

@jwage jwage force-pushed the abstract-migration-deprecation branch 4 times, most recently from 3eddc37 to 2e743a7 Jun 5, 2018

@jwage

This comment has been minimized.

Copy link
Member Author

jwage commented Jun 5, 2018

@stof I understand what you mean now. I reversed the inheritance, but with this approach, the deprecation warning will never go away after they start using the new class. I am not sure the effort here of releasing 1.8 with this deprecation the forward compatibility is worth it. People can just change to use Doctrine\Migrations\AbstractMigration when they upgrade to 2.0

@jwage jwage force-pushed the abstract-migration-deprecation branch 2 times, most recently from 3e2cfb9 to 8f41d59 Jun 6, 2018

@jwage

This comment has been minimized.

Copy link
Member Author

jwage commented Jun 6, 2018

I had an idea to move the deprecation warning to the constructor and override the constructor in the new class but this has other implications. Let me know your thoughts.

@stof

This comment has been minimized.

Copy link
Member

stof commented Jun 6, 2018

@jwage what you could do is trigger the deprecation in the constructor, but wrapped in a if (!$this instanceof NewClass) check. This way, no need to override the constructor (and so no need to make everything protected).

@jwage jwage force-pushed the abstract-migration-deprecation branch from 8f41d59 to b150140 Jun 6, 2018

@jwage jwage force-pushed the abstract-migration-deprecation branch from b150140 to f1a0e1f Jun 6, 2018

jwage added some commits Jun 6, 2018

@jwage jwage merged commit 4040df9 into 1.8 Jun 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jwage jwage deleted the abstract-migration-deprecation branch Jun 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.