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 for unexpected zero value last edit dates in segment archiving #21189

Merged
merged 4 commits into from
Aug 29, 2023

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Aug 28, 2023

Description:

If a segment has a last edited date of 0000-00-00 then segment archiving will fail with the exception: The date ... is a date before first website was online.

This is caused by attempting to parse the invalid 0000-00-00 date string. This PR adds a simple check to avoid parsing if the last edited date is null or zero along with some additional tests to check an exception isn't thrown when zero dates are provided.

ref: L3-522

Review

@bx80 bx80 added the Bug For errors / faults / flaws / inconsistencies etc. label Aug 28, 2023
@bx80 bx80 added this to the 5.0.0 milestone Aug 28, 2023
@bx80 bx80 self-assigned this Aug 28, 2023
@bx80 bx80 added the Needs Review PRs that need a code review label Aug 28, 2023
@caddoo
Copy link
Contributor

caddoo commented Aug 28, 2023

While you are in this private method, I have some suggestions of small improvements, let me know what you think.

  • Add type hinting / return type
  • The method name doesn't really indicate the full picture of what it's doing, can it be clearer.
  • The method is doing two things, would it be worth splitting into two methods?

@bx80
Copy link
Contributor Author

bx80 commented Aug 28, 2023

Add type hinting / return type

Done.

The method name doesn't really indicate the full picture of what it's doing, can it be clearer.

Not really, the method is called getCreatedTimeOfSegment, it gets the created time of the supplied segment and also returns the last edited time, but I don't think renaming it to getCreatedTimeOfSegmentAndLastEditedTimeOfSegment going to be helpful to anyone other than ultrawidescreen monitor manufacturers.

The method is doing two things, would it be worth splitting into two methods?

Unless we're going to try to enforce a 'method can only do one thing' rule and subsequently undertake long term refactoring of all existing code to this standard then I don't see any meaningful benefit in breaking this particular 13 line method into two slightly smaller methods.

let me know what you think.

The style preferences highlighted here could also apply to most of the codebase so I think it would be more efficient and constructive to address them via general project wide coding conventions.

@sgiehl sgiehl merged commit 6f18b00 into 5.x-dev Aug 29, 2023
16 of 20 checks passed
@sgiehl sgiehl deleted the l3-522-segment-archiving-date-fix branch August 29, 2023 07:04
@sgiehl sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants