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

feat(event-log): migrate data db column to longtext #227

Merged
merged 4 commits into from
Mar 5, 2025

Conversation

miguelpeixe
Copy link
Member

@miguelpeixe miguelpeixe commented Mar 5, 2025

The introduction of content distribution requires the event log data column to hold contents much larger than 64kb. The column type should be migrated to longtext. This PR also introduces the use of dbDelta() to manage table changes.

Testing

  1. While on trunk, publish a post ensuring it exceeds 64kb in text (you can generate text using https://lipsum.com/)
  2. Attempt to distribute that post to a node and confirm the event log is never created and the post doesn't reach the destination
  3. Checkout this branch, publish a new post exceeding 64kb and distribute to a node
  4. Confirm the event log item is created and the post reaches the destination
  5. Run wp option get newspack_db_version_event_log and confirm the value is 2
  6. Run wp db columns wp_newspack_hub_event_log and confirm data is longtext

@miguelpeixe miguelpeixe self-assigned this Mar 5, 2025
@miguelpeixe miguelpeixe requested a review from a team as a code owner March 5, 2025 17:42
@@ -47,32 +47,39 @@ protected static function get_current_option_name() {
* @return void
*/
protected static function maybe_update_db() {
$db_version = get_option( self::get_current_option_name(), 0 );
$db_version = absint( get_option( self::get_current_option_name(), 0 ) );
update_option( self::get_current_option_name(), self::DB_VERSION );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to also delete this line

Copy link
Member Author

Choose a reason for hiding this comment

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

We still want the db version to be tracked so it runs dbDelta() if necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, I now realize it's a duplicated line

@leogermani
Copy link
Contributor

On my site I ended up on an infinite loop trying to update the DB table when visiting the Event Log.

Fixed by moving the update_option call up and outside the if ( $db_version... as it was before.

Also tested deleting the table and checking if it would work well on the first table creation. It did :)

@miguelpeixe miguelpeixe requested a review from leogermani March 5, 2025 18:21
@miguelpeixe miguelpeixe merged commit ef2353b into trunk Mar 5, 2025
4 checks passed
@miguelpeixe miguelpeixe deleted the fix/event-log-db-data-type branch March 5, 2025 18:23
Copy link

github-actions bot commented Mar 5, 2025

Hey @miguelpeixe, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

matticbot pushed a commit that referenced this pull request Mar 5, 2025
# [2.8.0-alpha.1](v2.7.0...v2.8.0-alpha.1) (2025-03-05)

### Features

* **content-distribution:** remove distribution on incoming post deletion ([#225](#225)) ([24a421a](24a421a))
* **event-log:** migrate `data` db column to `longtext` ([#227](#227)) ([ef2353b](ef2353b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.8.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Mar 18, 2025
# [2.8.0](v2.7.0...v2.8.0) (2025-03-18)

### Bug Fixes

* **content-distribution:** pages table column ([#230](#230)) ([fe5fa5c](fe5fa5c))

### Features

* **content-distribution:** remove distribution on incoming post deletion ([#225](#225)) ([24a421a](24a421a))
* **content-distribution:** support page post type ([#228](#228)) ([1d21b4d](1d21b4d))
* **event-log:** migrate `data` db column to `longtext` ([#227](#227)) ([ef2353b](ef2353b))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants