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.

Sign up for GitHub

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

Jump to bottom

Deprecate using --all-or-nothing with values #1305

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Deprecate using --all-or-nothing with values #1305

merged 2 commits into from
Oct 11, 2023

Conversation

agustingomes
Copy link
Contributor

Q A
Type deprecation
BC Break yes, in the major version 4
Fixed issues #1304

Summary

During the iteration of the PR #1296 an idea to deprecate passing values to this option was suggested, given how complicated was to handle using that option without any value, which led to a bug and inconsistency with the documentation.

As specified in the deprecation message, using this option from 4.0.x onwards in the CLI will be treated as true, and the option itself won't accept values.

@greg0ire greg0ire added the Deprecation This PR introduces a deprecation label Jan 15, 2023
@greg0ire
Copy link
Member

Please add an Upgrade to 3.6 section to UPGRADE.md

@agustingomes
Copy link
Contributor Author

agustingomes commented Jan 16, 2023 •
edited
Loading

@greg0ire I'll apply the suggestions later today

@agustingomes agustingomes force-pushed the deprecate-all-or-nothing-options branch from ef571b5 to e52e88d Compare January 16, 2023 18:51
@agustingomes agustingomes requested a review from greg0ire January 16, 2023 18:51
@agustingomes agustingomes mentioned this pull request Jan 26, 2023
Open
greg0ire
greg0ire reviewed Jun 22, 2023
View reviewed changes
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.

@agustingomes sorry I forgot about this… can you rebase please 🙏 ?

@greg0ire
Copy link
Member

greg0ire commented Sep 2, 2023

ping @agustingomes :)

@agustingomes
Copy link
Contributor Author

@greg0ire No worries. I'll do the rebase and push it afterwards.

@agustingomes agustingomes force-pushed the deprecate-all-or-nothing-options branch from e52e88d to 64f23e0 Compare October 10, 2023 13:43
@agustingomes
Copy link
Contributor Author

@greg0ire I rebased it against 3.6.x, let me know if this ok for you, or if I should rebase it against other branch.

@greg0ire
Copy link
Member

Deprecations should go on the next minor branch. Right now that's 3.7.x, sorry.

@agustingomes
Copy link
Contributor Author

@greg0ire no problem, I'll rebase my branch on 3.7.x

@agustingomes
Deprecate using --all-or-nothing with values
5038099 
During the iteration of the PR doctrine#1296 an idea to deprecate passing values to this option was suggested, given how complicated was to handle using that option without any value, which led to a bug and inconsistency with the documentation.

As specified in the deprecation message, using this option from 4.0.x onwards in the CLI will be treated as `true`, and the option itself won't accept values.
@agustingomes agustingomes force-pushed the deprecate-all-or-nothing-options branch from 61a37e7 to 6fd78a4 Compare October 10, 2023 17:02
@agustingomes agustingomes requested a review from greg0ire October 10, 2023 17:02
@agustingomes agustingomes changed the base branch from 3.6.x to 3.7.x October 10, 2023 17:03
@agustingomes agustingomes force-pushed the deprecate-all-or-nothing-options branch from 6fd78a4 to 799f16d Compare October 10, 2023 17:04
@agustingomes
Copy link
Contributor Author

@greg0ire I rebased the branch, but I don't know how to check the BC Break locally, as it seems the tool that does that check is not getting installed when doing composer install.

greg0ire
greg0ire approved these changes Oct 10, 2023
View reviewed changes
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.

The bc check failure looks unrelated, I think it can be ignored. I shall try to look into it at some point

@greg0ire greg0ire merged commit 42a0111 into doctrine:3.7.x Oct 11, 2023
@greg0ire greg0ire added this to the 3.7.0 milestone Oct 11, 2023
@greg0ire
Copy link
Member

Thanks @agustingomes !

@agustingomes agustingomes deleted the deprecate-all-or-nothing-options branch October 12, 2023 10:13
@github-actions github-actions bot mentioned this pull request Nov 13, 2023
Merged
@vitormattos
Copy link

After this merge the follow row started to trigger the deprecated message:

https://github.com/doctrine/migrations/blob/3.7.x/lib/Doctrine/Migrations/Tools/Console/Command/MigrateCommand.php#L81

I think that as a follow up of this PR will be necessary remove this row.

@greg0ire
Copy link
Member

greg0ire commented Dec 4, 2023

I don't understand. Please open a full fledged issue.

@agustingomes
Copy link
Contributor Author

@vitormattos if I understand correctly, this will fix the problem, as the logic to throw/not throw the deprecation was incomplete:
#1381

It already has an issue associated to it, which has been linked in the PR.

cc @greg0ire

@greg0ire
Copy link
Member

greg0ire commented Dec 4, 2023

Oh right, forgot this wasn't merged already.

@agustingomes agustingomes mentioned this pull request Aug 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SenseException SenseException SenseException approved these changes

@greg0ire greg0ire greg0ire approved these changes

@derrabus derrabus Awaiting requested review from derrabus

Assignees
No one assigned
Labels
Deprecation This PR introduces a deprecation
Projects
None yet
Milestone
3.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@agustingomes @greg0ire @vitormattos @SenseException