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.

Sign up for GitHub

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

Jump to bottom

Unify and cleanup HTML signature #109

Merged
merged 4 commits into from
Jul 3, 2017
Merged

Unify and cleanup HTML signature #109

merged 4 commits into from
Jul 3, 2017

Conversation

stklcode
Copy link
Contributor

@stklcode stklcode commented Jun 27, 2017 •
edited
Loading

Addresses issue #108

Unifies signature in HTML comment and provides the ability to remove additional data.

Signature with details (old behavior)

<!-- Cachify | http://cachify.de
{METHOD} @ {DATE TIME}
{DETAILS}
-->

Cleaned signature (default)

<!-- Cachify | http://cachify.de
Generated @ {DATE TIME} -->

@krafit krafit requested review from websupporter and removed request for bueltge and swissspidy July 3, 2017 06:13
websupporter
websupporter reviewed Jul 3, 2017
View reviewed changes
inc/cachify.class.php Outdated
* @return bool Show details in signature.
*/
private static function _signature_details() {
return isset(self::$options['sig_detail']) ? self::$options['sig_detail'] == 1 : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, this could be a bit more readable. Isn't isset(self::$options['sig_detail']) always true?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also about Yoda you must think and strict comparison you must use :)

websupporter
websupporter reviewed Jul 3, 2017
View reviewed changes
inc/cachify.class.php
@@ -1290,7 +1302,8 @@ public static function set_cache( $data ) {
),
self::_cache_hash(),
self::_minify_cache( $data ),
self::_cache_expires()
self::_cache_expires(),
self::_signature_details()
Copy link
Contributor

@websupporter websupporter Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why is it needed here? Cachify_DB::store_item does not expect this information.
Cachify_APC is using it.

websupporter
websupporter reviewed Jul 3, 2017
View reviewed changes
inc/cachify_db.class.php Outdated
@@ -131,7 +132,7 @@ public static function print_cache( $cache ) {

/* Signature */
if ( isset( $cache['meta'] ) ) {
echo self::_cache_signature( $cache['meta'] );
echo self::_cache_signature( $sigDetail, $cache['meta'] );
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason to store the signature in APC/HDD/Memcache in the cached page itself while DB is generating the signature during the output process? I think, this would make sense to unify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The DB cache signature can provide additional information only available at runtime (e.g. number of actual DB queries). There might be any reason why one would like to see this...

Only possibility to unify call of the "basic" signature would be to add a second comment at runtime or drop the additional data completely (remove $sig_detail).

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for clarification :)

websupporter
websupporter reviewed Jul 3, 2017
View reviewed changes
inc/cachify_apc.class.php Outdated
* @param string $hash Hash of the entry.
* @param string $data Content of the entry.
* @param integer $lifetime Lifetime of the entry.
* @param bool $sigDetail Show details in signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use snake_case format: $sig_detail

websupporter
websupporter reviewed Jul 3, 2017
View reviewed changes
inc/cachify_db.class.php Outdated
*
* @param array $meta Content of metadata.
* @return string Signature string
* @param bool $detail Show details in signature
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameter comment must end with a full stop.

websupporter
websupporter reviewed Jul 3, 2017
View reviewed changes
inc/cachify_db.class.php Outdated
human_time_diff( $meta['time'], current_time( 'timestamp' ) )
)
);
if ($detail) {
Copy link
Contributor

Choose a reason for hiding this comment

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

space after opening bracket and before closing bracket are missing.

websupporter
websupporter reviewed Jul 3, 2017
View reviewed changes
inc/cachify_hdd.class.php Outdated
*
* @return string Signature string
* @param bool $detail Show details in signature
Copy link
Contributor

Choose a reason for hiding this comment

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

fullstop.

@websupporter
Copy link
Contributor

Hi @stklcode,
thanks a lot for your PR 🎉 I have just some minor nitpicks about it (see above). Most of them just so we stay on track with our goal to align the code with WordPress standards.

@krafit krafit added this to the 2.3.0 milestone Jul 3, 2017
websupporter
websupporter approved these changes Jul 3, 2017
View reviewed changes
Copy link
Contributor

@websupporter websupporter left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks a lot @stklcode ❤️

@websupporter websupporter merged commit 7118206 into pluginkollektiv:master Jul 3, 2017
@websupporter websupporter mentioned this pull request Jul 3, 2017
Closed
@stklcode stklcode deleted the ftUnifySignature branch July 3, 2017 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@websupporter websupporter websupporter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@stklcode @websupporter @krafit