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

[RFC] [2.2] Convert Timestamp for date widgets #421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

richardhj
Copy link
Member

Description

Convert date widget values to timestamps on encode property value and vice-versa on decode property value.

Closes #415.

Checklist

  • Read and understood the CONTRIBUTING guidelines
  • Created tests, if possible
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself to the @authors in touched PHP files
  • Checked the changes with phpcq and introduced no new issues

This code is simliar to https://github.com/MetaModels/attribute_timestamp/blob/2.0.4/src/MetaModels/Attribute/Timestamp/BootSubscriber.php#L56-L100

We need to ensure that both event listeners (the one of DCG and the one of MM) do not conflict.

@richardhj richardhj changed the base branch from master to feature/contao4 August 14, 2018 13:06
@richardhj
Copy link
Member Author

We need to ensure that both event listeners (the one of DCG and the one of MM) do not conflict.

We can give the event listener a low priority and extend the event listeners condition to:

if (!\is_int($event->getValue()) {
    return;
}

@baumannsven
Copy link
Member

Not sure this is a BC break. In my opinion, this should only be adopted in the next major. You could use a parameter in the evaluation to control whether the date can be converted.

@discordier What do you mean?

@richardhj
Copy link
Member Author

IMO saving the timestamp is the expected behavior while saving the formatted date is a bug.

I appreciate further discussion :-)


screen shot 2018-08-14 at 19 22 40

(we then might need a ModelToLabelListener for the list view)

@baumannsven
Copy link
Member

It's not about whether this is a bug. Hence my idea to activate the conversion in the Configuration field. Otherwise we need a database upgrade for the DCG, which is not up to the DCG. That's why I thought you could handle this properly in the next major release, if it's necessary at all. In the future DCG will use the Doctrine Abstraction Layer.

@zonky2
Copy link
Contributor

zonky2 commented Aug 15, 2018

It is still my opinion that the Unix timestamp is not the most elegant format for MySQL to store date and time!

MySQL has a data format with a much wider range ('1000-01-01 00:00:00:00' to '9999-12-31 23:59:59') and saves the incorrect conversions from the timestamp - see also MetaModels/core#881

(duplicate of #415 (comment))

@richardhj
Copy link
Member Author

It is still my opinion that the Unix timestamp is not the most elegant format for MySQL to store date and time!

I can see the point, but from what I read, the MySQL date format ist time zone agnostic, meaning that you are not able to change the timezone in the system setting.

The second point (@discordier mentioned somewhere) is that you are not compatible with Contao.
Imagine you query a date field from the database. You will not be certain whether you fetched a PHP timestamp or a MySQL timestamp.

@zonky2 zonky2 changed the title [RFC] [2.1] Convert Timestamp for date widgets [RFC] [2.2] Convert Timestamp for date widgets Aug 24, 2018
@discordier discordier changed the base branch from feature/contao4 to master March 15, 2019 00:37
@discordier discordier reopened this Mar 15, 2019
Copy link
Member

@discordier discordier left a comment

Choose a reason for hiding this comment

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

Will there still be some action on this or shall we archive this PR?

@zonky2 zonky2 modified the milestones: 2.2.0, Future May 4, 2022
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.

DCG saves dates as formatted string
4 participants