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

Bug on created_at in customer_entity table on update #4069

Open
sdecalom opened this issue Jul 2, 2024 · 17 comments · May be fixed by #4070
Open

Bug on created_at in customer_entity table on update #4069

sdecalom opened this issue Jul 2, 2024 · 17 comments · May be fixed by #4070

Comments

@sdecalom
Copy link
Contributor

sdecalom commented Jul 2, 2024

Preconditions (*)

1.OpenMage 19.4.23
2.Mysql 8.0

Steps to reproduce (*)

Update a customer created on 2024-07-01 at 15:00.

Expected result (*)

The date of created_at on customer_entity should be 2024.07.01 15:00
image

Actual result (*)

The created_at date on customer_entity is 2024.07.01 17:00
image

@addison74
Copy link
Contributor

You have a very old version, unmaintained.

I confirm with the cloned version of the repository from today I cannot reproduce it. I created a customer account in the Backend and the created_at column shows correct information. I updated an existing customer then the column changes with the correct timestamp.

Please try reproducing this issue in a test environment just with the latest version without any extension, modules, or custom work.

@sdecalom sdecalom linked a pull request Jul 2, 2024 that will close this issue
4 tasks
@sdecalom
Copy link
Contributor Author

sdecalom commented Jul 3, 2024

OK thank you for your support, I will try with the latest version.

@sdecalom sdecalom closed this as completed Jul 3, 2024
@addison74
Copy link
Contributor

Did you manage to solve this issue?

In my tests I used MariaDB and Percona, but I will try to MySQL 8 in order to reproduce this issue.

@sdecalom sdecalom reopened this Jul 4, 2024
@sdecalom
Copy link
Contributor Author

sdecalom commented Jul 4, 2024 via email

@ma4nn
Copy link
Contributor

ma4nn commented Nov 3, 2024

Why is the created_at column on the customer_entity table set to on update current_timestamp() at all?
I noticed this coincidentally after doing a manual UPDATE on the table and now all customer created dates are overwritten. This must be a bug, right?

@sreichel sreichel reopened this Nov 3, 2024
@sreichel
Copy link
Contributor

sreichel commented Nov 3, 2024

@ma4nn its not only customer_entity ... its for all entity tables. Seems not correct to me. updated_at should be updated.

@ma4nn
Copy link
Contributor

ma4nn commented Nov 3, 2024

@sreichel ah yes you're right, even worse 👍 any idea why this has been done? I also don't get why nobody complained about that yet? It's a massive bug and the data cannot be recovered at all (or at least it's an immense effort depending on the backups) if you didn't notice immediately.

Sure it's only relevant if you do direct sql updates, but this should not be so uncommon I think..

@sreichel
Copy link
Contributor

sreichel commented Nov 3, 2024

I also don't get why nobody complained about that yet?

Using models work and the most used, product and category are correct?

Should be fixed ....

Suggestion for all?

    created_at        timestamp       default current_timestamp()   not null comment 'Created At',
    updated_at        timestamp       default '0000-00-00 00:00:00' not null on update current_timestamp() comment 'Updated At',

@justinbeaty
Copy link
Contributor

I guess everyone is using the sample data. 😅 Without it:

  `created_at` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00' COMMENT 'Created At',
  `updated_at` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00' COMMENT 'Updated At',

@sreichel
Copy link
Contributor

sreichel commented Nov 3, 2024

Removing current timestamp for created_at could break import-scripts expected behavoir. (?)

@justinbeaty
Copy link
Contributor

@sreichel What I'm saying is that without installing the sample data, that is how the columns are defined.

@justinbeaty
Copy link
Contributor

justinbeaty commented Nov 3, 2024

However, I'm sure it would be better to be:

  `created_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP COMMENT 'Created At',
  `updated_at` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP COMMENT 'Updated At',

Then entirely remove the two files at app/code/core/Mage/Eav/Model/Entity/Attribute/Backend/Time/, and just convert to store timezone in Mage_Eav_Model_Entity_Abstract::_afterLoad (which is where the Mage_Eav_Model_Entity_Attribute_Backend_Time_Created::afterLoad method was called).

@sreichel
Copy link
Contributor

sreichel commented Nov 3, 2024

@sreichel What I'm saying is that without installing the sample data, that is how the columns are defined.

Ahhhh. Have seen it now. 🤣

So "only" sample data needs to be updated for #4069 (comment)? After years it needs also some other updates ...

@justinbeaty
Copy link
Contributor

So "only" sample data needs to be updated for #4069 (comment)? After years it needs also some other updates ...

Normally sample data shouldn't need to be updated for schema changes, because upgrade scripts would take care of that. So how did it happen? Did the author of the sample data just change the schema manually!?

This original issue and #4070 still seem valid, but my idea in the last comment could solve it.

@ma4nn
Copy link
Contributor

ma4nn commented Nov 4, 2024

@justinbeaty nice find 👍 I had a look at three different customer installations and all had the same table structure so I assumed it was created by default, but just verified with a clean installation that this is not the case. So all these instances must have had installed the sample data at some point 🙈

I agree with your solution in your comment above.

@justinbeaty
Copy link
Contributor

@ma4nn Yeah it's definitely odd because as far back as Magento 1.6.0.0-alpha1 it never had the ON UPDATE statement.

Also, Varien has some constants to help up with this:

    /**
     * Default values for timestampses - fill with current timestamp on inserting record, on changing and both cases
     */
    public const TIMESTAMP_INIT_UPDATE = 'TIMESTAMP_INIT_UPDATE';
    public const TIMESTAMP_INIT        = 'TIMESTAMP_INIT';
    public const TIMESTAMP_UPDATE      = 'TIMESTAMP_UPDATE';

@sreichel
Copy link
Contributor

I will update sample-data ASAP. (nearly done)

Changing install scripts is another task.

@justinbeaty thanks!

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 a pull request may close this issue.

5 participants