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

Fix Cache Issue In Schema Difference Calculation For Log Tables #30697

Merged

Conversation

shahrukh-compuco
Copy link

Overview

This pr solves a very specific caching issue related to calculation of schema difference between a table and its log table during the upgrades that includes multiple changes to the same table.

Technical Details

While upgrading from version 5.51.3 to 5.75.0 we change the columns post_URL and cancel_URL here for table civicrm_uf_group and later in the upgrade process we make changes to same table again here and this time civi also tries to sync log tables here if logging is enabled.

During this sync when civi is trying to find differences between civicrm_uf_group table and log_civicrm_uf_group table there appears to be an issue that in cache for civicrm_uf_group there exists both columns post_URL (old column name) and post_url (new column name) as well. And since post_URL does not exist in log_civicrm_uf_group it appears in the differences as a new column.

When civi tries to find the spec of this new column here from database there appears to be none due to which here it forms an incomplete query and thus results in an error. Same issue happens for cancel_URL column as well.

This pr adds an ability to refresh the table cache during schema difference calculations during the upgrades.

Copy link

civibot bot commented Jul 17, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Jul 17, 2024
shahrukh-compuco pushed a commit to compucorp/civicrm-core that referenced this pull request Jul 17, 2024
shahrukh-compuco pushed a commit to compucorp/civicrm-core that referenced this pull request Jul 17, 2024
@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Jul 24, 2024
@demeritcowboy
Copy link
Contributor

This looks ok to me and I can reproduce and it fixes it.
There's an argument to put against 5.76 but it's borderline agewise and while it's a supported upgrade path, large jumps are expected to have bumps so I think master is fine.

@demeritcowboy demeritcowboy merged commit bf85697 into civicrm:master Jul 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants