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

Introduce setting for controlling thread sort order #3

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Introduce setting for controlling thread sort order #3

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5c3b35f
added admin option to change post order ASC or DESC
chiranjitm-cn May 5, 2016
5a3edb6
Updated metavalue
chiranjitm-cn May 5, 2016
cf585df
Updated as per the comments in PR
chiranjitm-cn May 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions threads.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,14 @@ function cfth_thread_notice($posts, $query) {

function cfth_timeline_posts($term_id) {
$term = get_term_by('id', $term_id, 'threads');
$term_meta = get_term_meta( $term_id, 'threads_meta', true );
Copy link

Choose a reason for hiding this comment

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

This function was introduced as of 4.4.0. Using it here means that the plugin's required minimum WP version will be 4.4.0. That means an update to the README and the plugin's heading docblock.

We should also, for good measure, confirm this function exists using if ( function_exists( 'get_term_meta' ) ) simply because it is so new

$order = ( isset( $term_meta['order_term_meta'] )) ? $term_meta['order_term_meta'] : 'ASC';
Copy link

Choose a reason for hiding this comment

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

there is an extra set of parenthesis around the first item in your ternary operation here, you can remove these

if ($term) {
$query_params = apply_filters('threads_timeline_posts_query', array(
'posts_per_page' => -1,
'taxonomy' => 'threads',
'term' => $term->slug,
'order' => 'ASC',
'order' => $order,
));
$query = new WP_Query($query_params);
return $query->posts;
Expand All @@ -149,6 +151,9 @@ function cfth_timeline_posts($term_id) {
}

function cfth_timeline_content($term_id) {
$term = get_term_by('id', $term_id, 'threads');
$term_meta = get_term_meta( $term_id, 'threads_meta', true );
$order = ( isset( $term_meta['order_term_meta'] )) ? $term_meta['order_term_meta'] : 'ASC';
$posts = cfth_timeline_posts($term_id);
if (!count($posts)) {
return array();
Expand All @@ -170,7 +175,7 @@ function cfth_timeline_content($term_id) {
if ($prev) {
$prev_timestamp = strtotime($prev->post_date_gmt);
$this_timestamp = strtotime($_post->post_date_gmt);
$prev->threads_data['time_offset'] = $this_timestamp - $prev_timestamp;
$prev->threads_data['time_offset'] = ( isset( $term_meta['order_term_meta'] ) && 'DESC' === $term_meta['order_term_meta']) ? $prev_timestamp - $this_timestamp : $this_timestamp - $prev_timestamp;
Copy link

Choose a reason for hiding this comment

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

This is a confusing way to handle this logic. I would separate the steps and include some documentation to explain why this is being handled in this way. That will make the code easier to maintain long-term.

}
$prev = $_post;
}
Expand Down Expand Up @@ -375,3 +380,58 @@ function cfth_timeline_css() {
<?php
}

// Meta field markup for add thread page
function threads_taxonomy_add_new_meta_field() {
Copy link

Choose a reason for hiding this comment

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

Pay attention to your function prefixes. This doesn't match the prefix used by all legacy functions.

?>
<div class="form-field">
<label for="term_meta[order_term_meta]"><?php _e( 'Post order', 'threads' ); ?></label>
<select name="term_meta[order_term_meta]" id="term_meta[order_term_meta]">
<option value="ASC"><?php _e( 'ASC', 'threads' ); ?></option>
Copy link

Choose a reason for hiding this comment

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

Needs escaping,esc_html_e() instead of _e()

<option value="DESC"><?php _e( 'DESC', 'threads' ); ?></option>
</select>
<p class="description"><?php _e( 'Enter a value for this field','threads' ); ?></p>
</div>
<?php
}

add_action( 'threads_add_form_fields', 'threads_taxonomy_add_new_meta_field', 10, 2 );
Copy link

Choose a reason for hiding this comment

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

In this action you've specified that our callback will accept 2 arguments but I see zero being captured in the function definition.


// Meta field markup for edit thread page
function threads_taxonomy_edit_meta_field( $term ) {
Copy link

Choose a reason for hiding this comment

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

Pay attention to your function prefixes. This doesn't match the prefix used by all legacy functions.

$term_meta = get_term_meta( $term->term_id, 'threads_meta', true );
?>

<tr class="form-field">
<th scope="row" valign="top"><label for="term_meta[order_term_meta]"><?php _e( 'Post order', 'threads' ); ?></label></th>
Copy link

Choose a reason for hiding this comment

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

Needs escaping, esc_html_e() instead of _e()

<td>
<select name="term_meta[order_term_meta]" id="term_meta[order_term_meta]">
<option value="ASC" <?php echo ( isset( $term_meta['order_term_meta'] ) && 'ASC' === $term_meta['order_term_meta'] )? 'selected' : ''; ?> ><?php _e( 'ASC', 'threads' ); ?></option>
<option value="DESC" <?php echo ( isset( $term_meta['order_term_meta'] ) && 'DESC' === $term_meta['order_term_meta'] )? 'selected' : ''; ?>><?php _e( 'DESC', 'threads' ); ?></option>
Copy link

Choose a reason for hiding this comment

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

WP has a helper function named selected() that you can use for both this line and the previous:
This entire piece:

<?php echo ( isset( $term_meta['order_term_meta'] ) && 'DESC' === $term_meta['order_term_meta'] )? 'selected' : ''; ?>

can become this:

<?php selected( $term_meta['order_term_meta'], 'DESC' ); ?>

Copy link

Choose a reason for hiding this comment

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

Needs escaping, esc_html_e() instead of _e()

</select>
<p class="description"><?php _e( 'Enter a value for this field','threads' ); ?></p>
Copy link

Choose a reason for hiding this comment

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

Needs escaping, esc_html_e() instead of _e()

Copy link

@brichards brichards May 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This description is also very meaningless. Instead, it should probably read "Sort threaded posts with newest first (DESC), or oldest first (ASC)."

You could also probably change the select input option names to be more humanized as well:
"Oldest to Newest (ASC)" and "Newest to Oldest (DESC)"

</td>
</tr>
<?php
}

add_action( 'threads_edit_form_fields', 'threads_taxonomy_edit_meta_field', 10, 2 );
Copy link

Choose a reason for hiding this comment

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

On this action you've specified the callback expects 2 parameters but you're only using 1 in the function definition.


// Save meta field for thread taxonomy.
function save_threads_custom_meta( $term_id ) {
Copy link

Choose a reason for hiding this comment

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

Pay attention to your function prefixes. This function doesn't have a prefix at all.

if ( isset( $_POST['term_meta'] ) ) {
$term_meta = get_term_meta( $term_id, 'threads_meta', true );
$cat_keys = array_keys( $_POST['term_meta'] );
Copy link

Choose a reason for hiding this comment

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

A variable named $cat_keys doesn't describe our data in any meaningful way. What are cat keys? I assume it's short for "category keys" but we're not dealing with categories here, we're ealing with threads and thread meta.


foreach ( $cat_keys as $key ) {
if ( isset ( $_POST['term_meta'][$key] ) ) {
$term_meta[$key] = $_POST['term_meta'][$key];
Copy link

Choose a reason for hiding this comment

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

There is absolutely zero sanitization happening here and there definitely should be. What if someone hits the admin area with a POST request that contains malicious injection code?

Copy link

Choose a reason for hiding this comment

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

Do we need to iterate over a theoretical loop of term meta options? We've only defined a single option thus far. Instead of using a loop we could just work directly with that one single piece of meta and be done.

}
}

update_term_meta( $term_id, 'threads_meta', $term_meta );
}
}

add_action( 'edited_threads', 'save_threads_custom_meta', 10, 2 );
add_action( 'create_threads', 'save_threads_custom_meta', 10, 2 );
Copy link

Choose a reason for hiding this comment

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

You've declared here, twice, that you're passing 2 arguments to the callback function. However, the function only makes use of a single parameter