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

Add explicit renameColumn method (4.x) #6280

Open
wants to merge 4 commits into
base: 4.1.x
Choose a base branch
from

Conversation

Tofandel
Copy link
Contributor

Merges the changes of #6080 into 4.0.x

return '';
}

if (! empty($column['version'])) {
if ((string) $column['type'] !== 'DateTime') {
Copy link
Contributor Author

@Tofandel Tofandel Jan 26, 2024

Choose a reason for hiding this comment

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

This might not be covered by tests, when I messed around and removed the if there was an error that Dbal/Types/DateTimeType cannot be converted to string, which makes sense because 'type' in column is a DbalType which doesn't have a __toString(), might need to be backported to 3.8 as well

I also think in 4.x we should abolish column converted to arrays in all the AbstractPlatform methods and use the Column class to get the correct type hints and static analysis (for a different PR)

@derrabus derrabus changed the title Merge branch '3.8.x' into 4.0.x Add explicit renameColumn method (4.x) Jan 26, 2024
@derrabus
Copy link
Member

Since we've reverted the feature, this is technically not a merge anymore. I've adjusted the PR title accordingly. Can you please rebase your changes onto 4.0.x? Currently the PR contains irrelevant changes that 4.0.x contains already.

@derrabus derrabus added this to the 4.1.0 milestone Feb 3, 2024
@derrabus derrabus changed the base branch from 4.0.x to 4.1.x February 3, 2024 18:37
greg0ire
greg0ire previously approved these changes Feb 6, 2024
src/Schema/TableDiff.php Outdated Show resolved Hide resolved
@Tofandel
Copy link
Contributor Author

Tofandel commented Feb 21, 2024

Hmmm, 4.0.x was released already? I though you would wait for this PR on both 3.9 and 4.0

Then this PR which includes breaking changes which were meant to be deprecated in 3.x cannot be released anymore in 4.x from my understanding and only in 5.x and the PR #6080 without the breaking changes need to be redone for 4.x?

This is way more work than I can put into this ATM

@greg0ire
Copy link
Member

Hmmm, 4.0.x was released already? I though you would wait for this PR on both 3.9 and 4.0

Sorry we didn't warn you about it, we had a lot on our plate and this apparently fell through the cracks.

Then this PR which includes breaking changes which were meant to be deprecated in 3.x cannot be released anymore in 4.x from my understanding and only in 5.x and the PR #6080 without the breaking changes need to be redone for 4.x?

Yes, exactly. There is very little difference between 5.0.x and 4.1.x right now, so the most convenient time to do it is now. I think what you can do is:

  • retarget this to 5.0.x, which shouldn't be very hard because of how little conflicts there should be
  • open another PR that is a revert of @derrabus 's revert. I don't expect that to be a lot of work either.

@Tofandel
Copy link
Contributor Author

Tofandel commented Feb 21, 2024

Should the revert be for 3.9 or 4.1 ?

If it's for 4.x we still have the same problem with tons of conflict with v4 so it's easier to start from this PR and simply revert the breaking changes using the old PR

I could open 3 PRs, one against 3.9 again without breaking change which is basically the old reverted PR, 4.1 which is this port still but without breaking changes and 5.0.x which is this PR

@derrabus
Copy link
Member

Sorry, I'm kinda busy right now, but I haven forgotten this PR. Promise!

  • 4.1.x is the target for this PR.
  • I can revert my revert on 3.9 myself when I merge this one. Don't worry.
  • Breaking changes will need to go into 5.x.

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.

None yet

3 participants