-
Notifications
You must be signed in to change notification settings - Fork 203
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
EZP-29279: Script converting ezdate & ezdatetime timestamps to UTC #2499
EZP-29279: Script converting ezdate & ezdatetime timestamps to UTC #2499
Conversation
{ | ||
$fields = []; | ||
|
||
if ($this->mode == 'date' || $this->mode == 'all') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and use these constants here afterward 😄
->addArgument( | ||
'timezone', | ||
InputArgument::OPTIONAL, | ||
'Original timestamp TimeZone', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
Original timestamp TimeZone
-> Original timestamp's TimeZone
$progressBar->start(); | ||
|
||
for ($offset = 0; $offset < $count; $offset += $iterationCount) { | ||
$processBuilder = new ProcessBuilder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be aware this is deprecated in 3.4: https://api.symfony.com/3.4/Symfony/Component/Process/ProcessBuilder.html
*/ | ||
protected function processTimestamps($offset, $limit) | ||
{ | ||
$ezDateObjectAttributes = $this->getEzDateObjectAttributes($offset, $limit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
terminology: Fields is the word for Attributes in eZ Platform
Also you can drop the use of "Ez" in methods, looks strange, and can be omitted if we are specific as this is anyway inside eZ Platform.
example $dateFields = $this->getDateFields($offset, $limit);
And then adapt the rest of the method and other places for the simpler naming ;)
|
||
$progressBar->finish(); | ||
$output->writeln( | ||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes for several places here, but you can safely place the array start on line of the call when it's the only argument.
Double check with php-cs-fixer but at least before I don't think it minds.
so:
$output->writeln([
'',// Empty newline
sprintf('Done: %d', $this->done),
]);
/** | ||
* @return int | ||
*/ | ||
protected function countEzDateObjectAttributes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* Counts affected date fields using captured "mode", "from" and "to" command options.
*
* @return int
*/
private function countDateFields()
'only-datetime', | ||
null, | ||
InputOption::VALUE_NONE, | ||
'Only ezdatetime fields will be converted' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this should be removed as it's not used and is covered by mode
The command <info>%command.name%</info> updates field | ||
data_int in configured Legacy Storage database for a given field type. | ||
|
||
Fields will be checked and updated only if not already in UTC timezone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will it detect this by itself? :)
Also, as a second sentence here maybe say something like:
This is to be used either when migrating from eZ Publish to eZ Platform (when using platform backend instead of legacy), or when upgrading legacy to v2019.03 which has been adapted to use UTC.
} | ||
|
||
/** | ||
* @return string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string[]
const DEFAULT_ITERATION_COUNT = 100; | ||
const EZDATE_ONLY_MODE = 'date'; | ||
const EZDATETIME_ONLY_MODE = 'datetime'; | ||
const ALL_MODE = 'all'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do the following:
const MODES = [
'date' => ['ezdate'],
'datetime' => ['ezdatetime'],
'all' => ['ezdate', 'ezdatetime'],
];
Simplifies validation of option, and simplifies mapping in getField[TypeIdentifier]s()
.
Can even use this as source for help text on the option to solve all open comments ;)
e8df7b7
to
171410c
Compare
171410c
to
3cc691f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big 👍
Tested last version?
|
||
This is to be used either when migrating from eZ Publish to eZ Platform | ||
(when using platform backend instead of legacy), or when upgrading legacy | ||
to v2019.03 which has been adapted to use UTC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note on MERGE: Depending on when this is merged and when patch releases comes out, we can change this to v2018.12 / 2019.01 / 2019.02 or whatever :)
Smooth as Butter 😄 |
6.13
This PR is followup for ezsystems/ezpublish-legacy#1401 and implements migration script for the legacy to convert stored ezdate & ezdatetime timestamps into UTC.