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: Article publish date is always replaced by current timestamp #283

Merged
merged 2 commits into from
Jul 20, 2023

Conversation

LeoniePhiline
Copy link
Contributor

Fixes #281

Explanation:

In b77f22d#diff-62c740cd7ba615b9771602fedd2cab6bc50ac4052c3f8432913f59dbaf3d5effR48 an attempted migration away from the deprecated DBAL fetch() method failed.
Instead of the drop-in replacement fetchAssociative, the wrong method fetchOne was chosen.
Maybe the author @benjaminkott had intended to use fetchOne, but select the desired field rather than the entire row. (The latter approach is taken by this PR.)

In v12.0.0 (or at any time after b77f22d), the database loaded and transmitted the entire row, but only the first column of the first row was returned. (See Doctrine\DBAL\ForwardCompatibility\Result::fetchOne, which calls fetchNumeric internally.)

Note that loading and transmitting the entire database row (SELECT *) is wasteful and slow.

Therefore, instead of fixing the issue by using fetchAssociative, this PR fixes the issue by keeping fetchOne but selecting only the used field ('publish_date').

If no row is found, fetchOne will return false. A strictly typed check for false is performed before comparing (again, strictly) to 0, in which case the current time will be used.

@benjaminkott
Copy link
Member

@LeoniePhiline thx a lot! Seems to be an oversight of me, I've added some tests to prove its working now.

@benjaminkott benjaminkott merged commit 965792c into TYPO3GmbH:master Jul 20, 2023
@LeoniePhiline
Copy link
Contributor Author

LeoniePhiline commented Jul 20, 2023

Yeah, quite a typical little mistake when similar changes are made in lots of places in a batch. :)

Thanks for your quick response, merge and release!

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 this pull request may close these issues.

not able to alter Publish Date
2 participants