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

Link ID refactoring #697

Merged
merged 5 commits into from
Dec 12, 2016
Merged

Conversation

ArthurHoaro
Copy link
Member

@ArthurHoaro ArthurHoaro commented Nov 28, 2016

First of all, it was way harder than what I though, tests appreciated.

Links now use an incremental unique numeric identifier. This ID is persistent and must never change. ArrayAccess is used to match the link ID with the array keys (see the comment at the top of LinkDB for more details).

Key 'created' added for each link, with creation date as a DateTime object. 'updated' is now also a DateTime.

I also added a persistent 'shorturl' key to all links to preserve existing permalinks. "Small hashes" are now generated once using the date (YYYYMMDD_hhmmss) and the ID to avoid collision.

The updater does its work and everything should be transparent to users. Just in case, it makes a backup of the datastore before anything.

Take a look at individual commits for something more readable.

@mro, I've run your tests against this PR, it should be fine.

Fixes #351 and closes v0.8.1.

@ArthurHoaro ArthurHoaro added cleanup code cleanup and refactoring feature in review please test template HTML rendering labels Nov 28, 2016
@ArthurHoaro ArthurHoaro added this to the 0.8.1 milestone Nov 28, 2016
@ArthurHoaro ArthurHoaro self-assigned this Nov 28, 2016
Copy link
Member

@virtualtam virtualtam left a comment

Choose a reason for hiding this comment

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

The updater ran fine on a ~2000-link datastore :)
Mostly minor comments regarding date formatting stuff and typos

* @param DateTime $date Link creation date.
* @param int $id Link ID.
*
* @return string the small hash generating from link data.
Copy link
Member

Choose a reason for hiding this comment

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

*generated

@@ -31,7 +31,11 @@ function logm($logFile, $clientIp, $message)
* - are NOT cryptographically secure (they CAN be forged)
*
* In Shaarli, they are used as a tinyurl-like link to individual entries,
* e.g. smallHash('20111006_131924') --> yZH23w
* build once with the combination of the date and item ID.
Copy link
Member

Choose a reason for hiding this comment

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

*built

* build once with the combination of the date and item ID.
* e.g. smallHash('20111006_131924' . 142) --> eaWxtQ
*
* Note: before v0.8.1, smallhashes were built only with the date,
Copy link
Member

Choose a reason for hiding this comment

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

$order = $order === 'ASC' ? -1 : 1;
// Reorder array by dates.
usort($this->links, function($a, $b) use ($order) {
return $a['created'] < $b['created'] ? 1 * $order : -1 * $order;
Copy link
Member

Choose a reason for hiding this comment

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

cool! this will be easy to modify to add support for the 'updated' link field

*
* @param int $id Persistent ID of a link.
*
* @return int Real offset in local array, or null if doesn't exists.
Copy link
Member

Choose a reason for hiding this comment

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

*exist

@@ -1429,6 +1451,8 @@ function renderPage($conf, $pluginManager)
'tags' => $tags,
'private' => $private
);
} else {
$link['linkdate'] = $link['created']->format('Ymd_His');
Copy link
Member

Choose a reason for hiding this comment

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

LinkDB::LINK_DATE_FORMAT

} else {
$link['updated_timestamp'] = '';
}
$taglist = explode(' ', $link['tags']);
uasort($taglist, 'strcasecmp');
$link['taglist'] = $taglist;
$link['shorturl'] = smallHash($link['linkdate']);
$link['shorturl'] = smallHash($link['created']->format('Ymd_His'));
Copy link
Member

Choose a reason for hiding this comment

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

LinkDB::LINK_DATE_FORMAT


$isso = sprintf($isso_html, $issoUrl, $issoUrl, $link['linkdate'], $link['linkdate']);
// FIXME! KO thread unique si même date
$linkDate = $link['created']->format('Ymd_His');
Copy link
Member

Choose a reason for hiding this comment

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

LinkDB::LINK_DATE_FORMAT


$isso = sprintf($isso_html, $issoUrl, $issoUrl, $link['linkdate'], $link['linkdate']);
// FIXME! KO thread unique si même date
Copy link
Member

Choose a reason for hiding this comment

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

CORRIGEZ-MOI ! Comment in French 😄

{
self::$publicLinkDB->reorder('ASC');
$linkIdToTest = 42;
foreach (self::$publicLinkDB as $key => $value) {
Copy link
Member

Choose a reason for hiding this comment

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

is the test loop breaking after iterating over the first element? if so, it'd be more robust to check the order of keys in the sorted array

    Links now use an incremental unique numeric identifier.
    This ID is persistent and must never change.

    ArrayAccess is used to match the link ID with the array keys (see the comment in LinkDB for more details)

    Key 'created' added, with creation date as a DateTime object. 'updated' is now also a DateTime.
…ry keys.

creation and update dates are now DateTime objects.
Since this update is very sensitve (changing the whole database), the datastore will be automatically backed up into the file datastore.<datetime>.php.
All existing link will keep their permalinks.
New links will have smallhash generated with date+id.

The purpose of this is to avoid collision between links due to their creation date.
@ArthurHoaro
Copy link
Member Author

Thanks for your review! Good advices, as usual. =)

Changes rebased.

@ArthurHoaro ArthurHoaro merged commit 9cf93bc into shaarli:master Dec 12, 2016
@ArthurHoaro ArthurHoaro deleted the feature/ids-bis branch December 12, 2016 02:15
@virtualtam
Copy link
Member

You're welcome, thanks for releasing v0.8.1 ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup code cleanup and refactoring feature template HTML rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LinkDB: do not use raw timestamps to index links
2 participants