Skip to content

Drop old Commerce tables #2510

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
Merged
merged 9 commits into from
Oct 14, 2024
Merged

Drop old Commerce tables #2510

merged 9 commits into from
Oct 14, 2024

Conversation

@adriendupuis
Copy link
Contributor

@adriendupuis adriendupuis commented Oct 7, 2024 •
edited
Loading

Question Answer
JIRA Ticket N/A
Versions 4.4 but all doc branches
Edition All

ses_ and sve_ tables can be removed after migrating from old to new Commerce.

Preview: https://ez-systems-developer-documentation--2510.com.readthedocs.build/en/2510/update_and_migration/from_4.3/update_from_4.3_new_commerce/#update-the-database

Checklist

  • Text renders correctly
  • Text has been checked with vale
  • Description metadata is up to date
  • Redirects cover removed/moved pages
  • Code samples are working
  • PHP code samples have been fixed with PHP CS fixer
  • Added link to this PR in relevant JIRA ticket or code PR

adamwojs
adamwojs reviewed Oct 7, 2024
View reviewed changes
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

Can we include list of all necessary DELETE statements instead of dynamically generate SQL?

@adriendupuis
Copy link
Contributor Author

adriendupuis commented Oct 8, 2024

Can we include list of all necessary DELETE statements instead of dynamically generate SQL?

@adamwojs I thought it was more safe with the dynamic way. And I don't have the table list. I could try to find it.

@adriendupuis adriendupuis changed the title Drop old commerce tables Drop old Commerce tables Oct 8, 2024
@adriendupuis
Copy link
Contributor Author

adriendupuis commented Oct 8, 2024

Can we include list of all necessary DELETE statements instead of dynamically generate SQL?

@adamwojs I get the DROPable TABLE list from Commerce 3.3.40. Do you think I could have missed some?

@adriendupuis adriendupuis requested a review from adamwojs October 8, 2024 14:29
@adriendupuis adriendupuis marked this pull request as ready for review October 8, 2024 14:30
@adriendupuis adriendupuis requested a review from reithor October 8, 2024 14:43
reithor
reithor reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@reithor reithor left a comment

Choose a reason for hiding this comment

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

Not sure, if we have a standard. Wouldn't it be better to just list the tables that should be deleted - and leave the sql part to the user?

@adriendupuis
Copy link
Contributor Author

adriendupuis commented Oct 9, 2024 •
edited
Loading

Not sure, if we have a standard. Wouldn't it be better to just list the tables that should be deleted - and leave the sql part to the user?

@reithor @adamwojs
I could add (back from 7206ea5) how to list the tables before the list.
But I think that "The removable tables are prefixed with ses_ and sve_." sentence is enough for the advanced developer to start building own solution, while the lazy developer can just copy the hard-coded solution below.

@reithor
Copy link
Contributor

reithor commented Oct 9, 2024

I could add (back from 7206ea5) how to list the tables before the list. But I think that "The removable tables are prefixed with ses_ and sve_." sentence is enough for the advanced developer to start building own solution, while the lazy developer can just copy the hard-coded solution below.

@adriendupuis
Makes sense - I prefer an explicit list of tables, so leaving it as it is now is fine imho.

mnocon
mnocon approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

+1 for the explicit list of tables, thank you!

@reithor reithor self-requested a review October 9, 2024 11:45
dabrt
dabrt reviewed Oct 9, 2024
View reviewed changes
dabrt
dabrt reviewed Oct 9, 2024
View reviewed changes
dabrt
dabrt reviewed Oct 9, 2024
View reviewed changes
@adriendupuis @dabrt
Apply suggestions from code review
048749c 
Co-authored-by: Tomasz Dąbrowski <64841871+dabrt@users.noreply.github.com>
@adriendupuis adriendupuis merged commit 61fe28f into master Oct 14, 2024
6 checks passed
@adriendupuis adriendupuis deleted the drop-old-commerce-tables branch October 14, 2024 12:22
adriendupuis added a commit that referenced this pull request Oct 14, 2024
@adriendupuis @dabrt
Drop old Commerce tables (#2510)
f40052e 
---------

Co-authored-by: Tomasz Dąbrowski <64841871+dabrt@users.noreply.github.com>
(cherry picked from commit 61fe28f)
adriendupuis added a commit that referenced this pull request Oct 14, 2024
@adriendupuis @dabrt
Drop old Commerce tables (#2510)
ecc5a53 
---------

Co-authored-by: Tomasz Dąbrowski <64841871+dabrt@users.noreply.github.com>
(cherry picked from commit 61fe28f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adamwojs adamwojs adamwojs approved these changes

@mnocon mnocon mnocon approved these changes

@reithor reithor reithor approved these changes

@dabrt dabrt dabrt approved these changes

Assignees

No one assigned

Labels

Ready for MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@adriendupuis @reithor @adamwojs @mnocon @dabrt