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 a delimiter in the timezone to allow MySQL insert the value into the database #97

Closed
wants to merge 1 commit into from

Conversation

kiettran
Copy link

@kiettran kiettran commented Nov 9, 2020

Hi @frankdejonge ,

This pull request is to change the format of PointInTime::DATE_TIME_FORMAT. The reason is that MySQL (8.0.22) does not accept timezones without delimiters.

The following MySQL (8.0.22) error occurs with a DATETIME column in the events table.

SQLSTATE[22007]: Invalid datetime format: 1292 Incorrect datetime value: '2020-11-09 10:52:03.707881+0000' for column 'time_of_recording' at row 1"

The database table has the following reference:

CREATE TABLE IF NOT EXISTS domain_messages (
    event_id VARCHAR(36) NOT NULL,
    event_type VARCHAR(100) NOT NULL,
    aggregate_root_id VARCHAR(36) NOT NULL,
    aggregate_root_version MEDIUMINT(36) UNSIGNED NOT NULL,
    time_of_recording DATETIME(6) NOT NULL,
    payload JSON NOT NULL,
    INDEX aggregate_root_id (aggregate_root_id),
    UNIQUE KEY unique_id_and_version (aggregate_root_id, aggregate_root_version ASC)
) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci ENGINE = InnoDB

With this change the DATETIME value (2020-11-09 10:52:03.707881+00:00; with delimiter in the timezone) can be saved in the database.

@frankdejonge
Copy link
Member

Hi @kiettran, for this I'd suggest to account for this in the MessageRepository implementation rather than the PointInTime string format. The reason for this that this format is implementation specific, so what's good for one is not good for the other. In those cases you should err on the side of being specific. This means that every implementation of storage should convert the date-time format for their use-case.

@reksc
Copy link

reksc commented Dec 29, 2020

Hi @frankdejonge @kiettran I just stumbled upon the same problem.

This library is nicely type-hinted to interfaces and gives a lot of freedom. However, tight binding to PointInTime is an exception here. We can swap Clock implementation, but still the Clock interface requires to return PointInTime. The problem with PointInTime is that it's very opinionated, event when it comes to the datetime formatting discussed above.

Implementing the whole MessageRepository::persist() just to format a single date feels a bit overwhelming :).

@frankdejonge
Copy link
Member

Hi @reksc,

In my latest version of the 1.0.0 branch I've removed the PointInTime class and rely only on DateTimeImmutable instances. As for the formatting, what would your suggestion be? The MessageRepository interface is not particularly large, so it should not be a huge burden to implement. Having said that, I'm open to suggestions.

@reksc
Copy link

reksc commented Dec 30, 2020

@frankdejonge that's good news with using pure DateTimeImmutable and perhaps a reason to switch to ^1@dev.

I agree that the MessageRepository interface isn't large, what I wanted to point out is that the bundled implementation of persist() method is pretty fat (for a reason), and I had to override it just to change the date formatting. Maybe moving the $params preparation to an extra Mapper class or at least to an extra method would be a good idea?

Generally I tried to fix the date issue with custom Clock implementation (failed), custom Decorator (failed) and settled with MessageRepository :).

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

Successfully merging this pull request may close these issues.

3 participants