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

Check year range in both side #322

Closed
wants to merge 1 commit into from

Conversation

gnattu
Copy link
Contributor

@gnattu gnattu commented Mar 7, 2025

We cannot expect all users will always tag the files in the year tag with valid years. A lot of them will abuse this tag field to tag a full date in pure number, like 19991231. Such date is not parseable by DateTime.TryParse and is not within valid year range for DateTime constructor. We check if that is the case before creating a new DateTime instance.

See jellyfin/jellyfin#13669 and jellyfin/jellyfin#11576

For some format (like mp4) this case is already handled and fallback to the smallest date: jellyfin/jellyfin#13514

This PR makes the explicit years to also behave similar to that, which is an acceptable behavior when the user has specified an invalid year.

We cannot expect all users will always tag the files in the year tag with valid years. A lot of them will abuse this tag field to tag a full date in pure number, like 19991231. Such date is not parseable by DateTime.TryParse and is not within valid year range for DateTime constructor. We check if that is the case before creating a new DateTime instance.
@Zeugma440 Zeugma440 self-assigned this Mar 7, 2025
@Zeugma440
Copy link
Owner

We cannot expect all users will always tag the files in the year tag with valid years. A lot of them will abuse this tag field to tag a full date in pure number, like 19991231.

Thanks for the insight; I didn't realize that thing even existed.

For some format (like mp4) this case is already handled and fallback to the smallest date:

Falling back to 0 will certainly prevent the crash, but I'm sure we can do better than that in terms of added value.

Lemme work on my test cases, I'll get back to you with suggestions. The tricky part is to determine what's acceptable regarding specs and what we can force.

@Zeugma440
Copy link
Owner

Just pushed an update to handle that case more gracefully : any Year field recognized as a Date will be used to value the Date field instead. I'm closing this PR (thanks anyway for bringing that up~)

@Zeugma440 Zeugma440 closed this Mar 9, 2025
@Zeugma440
Copy link
Owner

Fix is available on today's v6.19

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.

2 participants