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

Add `--preserve-filetime` argument to support persisting file modification time #42

Merged
merged 9 commits into from Oct 12, 2017

Conversation

3 participants
@desrosj
Contributor

desrosj commented Sep 11, 2017

As detailed in #41, preserving the file time when importing local files could be very useful.

This PR introduces the --preserve-filetime option to use the file modified time instead of the default current time.

This is missing tests as I have not written Behat tests before. I will add those shortly as soon as I can read up a bit and get them running locally.

Fixes #41

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Sep 12, 2017

Member

As discussed in Slack, please use both creation and modification dates, and make sure to calculate GMT difference correctly.

Member

schlessera commented Sep 12, 2017

As discussed in Slack, please use both creation and modification dates, and make sure to calculate GMT difference correctly.

Show outdated Hide outdated src/Media_Command.php Outdated

desrosj added some commits Sep 19, 2017

Update modified and publish dates when using file time.
This also properly sets the GMT dates on the post using the site's current timezone offset.
Show outdated Hide outdated src/Media_Command.php Outdated
Show outdated Hide outdated src/Media_Command.php Outdated
@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Sep 21, 2017

Member

Small nitpick regarding constants vs magic numbers.

Are you up to writing tests for this as well?

Member

schlessera commented Sep 21, 2017

Small nitpick regarding constants vs magic numbers.

Are you up to writing tests for this as well?

@desrosj

This comment has been minimized.

Show comment
Hide comment
@desrosj

desrosj Sep 22, 2017

Contributor

Updated to use HOUR_IN_SECONDS instead.

I added what I have so far for tests. I am missing something in the touch step causing the file modified time to not be correct when tested. Also, I'm not sure if I have included more And steps then accepted in my test.

I looked into the file created and modified times. The times should all be returned as Unix timestamps that can be converted with data(). Also, there is no created time for files in most Unix filesystems.

filemtime() will return the the time when the data blocks of a file were being written to, that is, the time when the content of the file was changed while filectime() returns when the inode data is changed. filectime() will most likely return a newer date than filemtime() in most cases because of that difference. My patch is using the filemtime() for published and modified dates. If there is a better method, I can update accordingly.

Contributor

desrosj commented Sep 22, 2017

Updated to use HOUR_IN_SECONDS instead.

I added what I have so far for tests. I am missing something in the touch step causing the file modified time to not be correct when tested. Also, I'm not sure if I have included more And steps then accepted in my test.

I looked into the file created and modified times. The times should all be returned as Unix timestamps that can be converted with data(). Also, there is no created time for files in most Unix filesystems.

filemtime() will return the the time when the data blocks of a file were being written to, that is, the time when the content of the file was changed while filectime() returns when the inode data is changed. filectime() will most likely return a newer date than filemtime() in most cases because of that difference. My patch is using the filemtime() for published and modified dates. If there is a better method, I can update accordingly.

Show outdated Hide outdated features/media-import.feature Outdated
Show outdated Hide outdated features/media-import.feature Outdated
Show outdated Hide outdated features/media-import.feature Outdated
Show outdated Hide outdated src/Media_Command.php Outdated
@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Oct 10, 2017

Member

@desrosj Do you need assistance finishing these changes up?

Member

danielbachhuber commented Oct 10, 2017

@desrosj Do you need assistance finishing these changes up?

@danielbachhuber danielbachhuber changed the title from Preserve file time when importing files to Add `--preserve-filetime` argument to support persisting file modification time Oct 12, 2017

@danielbachhuber danielbachhuber added this to the 1.1.1 milestone Oct 12, 2017

I've cleaned things up

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Oct 12, 2017

Member

@desrosj ^ worth taking a look through the cleanup I did when you have a moment.

Member

danielbachhuber commented Oct 12, 2017

@desrosj ^ worth taking a look through the cleanup I did when you have a moment.

@danielbachhuber danielbachhuber merged commit 3ad78de into wp-cli:master Oct 12, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@desrosj

This comment has been minimized.

Show comment
Hide comment
@desrosj

desrosj Oct 13, 2017

Contributor

@danielbachhuber Thanks! Checked out your updates. Just didn't have the time the last week to follow up with this.

Contributor

desrosj commented Oct 13, 2017

@danielbachhuber Thanks! Checked out your updates. Just didn't have the time the last week to follow up with this.

@desrosj desrosj deleted the desrosj:file-time-option-on-import branch Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment