Skip to content

Model: Handle dates in UTC in Serde and the message factory - #627

Merged
jonathanbossenger merged 5 commits into
WordPress:developfrom
huzaifaalmesbah:fix/614-serde-utc-meta-keys
Nov 3, 2026
Merged

jonathanbossenger merged 5 commits into
WordPress:developfrom
huzaifaalmesbah:fix/614-serde-utc-meta-keys

Conversation

@huzaifaalmesbah

@huzaifaalmesbah huzaifaalmesbah commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

What?

Part of #614. Part of #302.

Makes Helper\Serde read and write dates in UTC, and makes Factory\Message deserialize its dates the same way the other factories do.

The Message::$meta_keys, channel_title and message length changes from #614 are handled in #637, because they follow from the new v2 columns (see #627 (comment)).

Why?

  • Serde depended on the PHP default timezone. maybe_deserialize_mysql_date() called DateTime::createFromFormat() without a timezone. WordPress sets the default to UTC, but anything that changes it shifts every date read from the database. It also accepted dates like 2024-02-30, which PHP silently rolls over to March.
  • Factory\Message could not load a message from the database. It passed created_at / expires_at straight to the model, whose properties are typed ?DateTime, so a MySQL date string threw TypeError: Cannot assign string to property … of type ?DateTime. Implement querying the database for notifications #307 would hit this first.

How?

includes/helper/class-serde.php

  • maybe_deserialize_mysql_date() parses with !Y-m-d H:i:s in UTC (! also resets microseconds).
  • Dates PHP would roll over (e.g. 2024-02-30, month 13, the MySQL zero date) and non-string values now return null.
  • A DateTime passed in is converted to UTC without modifying the original.
  • maybe_serialize_json_date() always outputs UTC (+00:00).

includes/factory/class-message.php

  • created_at and expires_at go through Serde::maybe_deserialize_mysql_date(), matching the Notification and Subscription factories.

Tests

  • New tests/phpunit/tests/test-serde.php. It sets the PHP default timezone to America/New_York to prove Serde doesn't depend on it, and covers the round trip, DateTime conversion, no mutation of the input, and invalid input (including the MySQL zero date).
  • test-factory-message.php: added a test that makes a message from MySQL date strings.

Testing Instructions

  1. npx wp-env start
  2. Run the suite:
    npx wp-env run tests-cli --env-cwd=wp-content/plugins/wp-feature-notifications vendor/bin/phpunit
  3. Run it as multisite:
    npx wp-env run tests-cli --env-cwd=wp-content/plugins/wp-feature-notifications env WP_MULTISITE=1 vendor/bin/phpunit
  4. Run only the affected tests:
    npx wp-env run tests-cli --env-cwd=wp-content/plugins/wp-feature-notifications vendor/bin/phpunit --filter "Test_Serde|Test_Factory_Message"

Local results: 79 tests, 0 failures on single site and multisite (the 24 skipped are the existing REST controller placeholder tests). PHPCS is clean.

Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5.5
Used for: Drafting the fix and unit tests. I chose the approach from the issue, agreed the split with #637, reviewed every change, and verified it locally with PHPUnit (single site and multisite) and PHPCS.

Serde read MySQL datetime strings in the PHP default timezone and
accepted out of range dates, which PHP silently rolls over. It now
reads and writes dates in UTC and rejects invalid dates.

`Message::$meta_keys` listed `expires_at`, `severity` and
`channel_title`, so `encode_meta()` wrote them into the `meta` JSON.
They are removed from `$meta_keys` and kept in `jsonSerialize()`.

`Factory\Message` now deserializes `created_at` and `expires_at` with
Serde, like the Notification and Subscription factories, so a message
loaded from the database no longer throws a `TypeError`.

Also removes the unused `Message::validate_message()`.

Fixes WordPress#614.
@jonathanbossenger jonathanbossenger mentioned this pull request Oct 16, 2026
5 of 22 tasks
Comment thread tests/phpunit/tests/test-serde.php Resolved
@faisalahammad

Copy link
Copy Markdown

Hi @huzaifaalmesbah, #637 and this PR both change includes/model/class-message.php, so the second one to merge will have conflicts there.

In the review on #637 this split was suggested:

Is this OK for you? Whichever PR merges second, please check that channel_title does not come back when the conflict is fixed.

@huzaifaalmesbah

Copy link
Copy Markdown
Member Author

Thanks @faisalahammad, that split works for me. I'll remove the $meta_keys, channel_title, jsonSerialize() and validate_message() changes from this PR, so it only keeps the Serde UTC fixes and the Factory\Message date deserialization (with their tests). The $meta_keys part of #614 will be covered by #637. I'll push the update and ping when it's ready.

Restore includes/model/class-message.php and its tests to develop, so
this PR only keeps the Serde UTC fixes and the Factory\Message date
deserialization. The $meta_keys, channel_title and message length
changes are handled in WordPress#637.
@huzaifaalmesbah huzaifaalmesbah changed the title Model: Handle dates in UTC and keep column fields out of message meta Model: Handle dates in UTC in Serde and the message factory Oct 31, 2026
@huzaifaalmesbah

Copy link
Copy Markdown
Member Author

@jonathanbossenger @faisalahammad I pushed 5601aba. It restores includes/model/class-message.php and its tests to develop, so this PR now only has the Serde UTC fixes and the Factory\Message date deserialization. I updated the title and description, and changed "Closes #614" to "Part of #614" since the $meta_keys part is in #637.

Tests: 79 tests, 0 failures on single site and multisite. PHPCS is clean.

Since the scope changed after the approval, could you take another look, @jonathanbossenger?

@jonathanbossenger
jonathanbossenger merged commit ec95a15 into WordPress:develop Nov 3, 2026
11 checks passed
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.

3 participants