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

vote from comments: fix a notice #106

Open
wants to merge 32 commits into
base: dev
Choose a base branch
from
Open

vote from comments: fix a notice #106

wants to merge 32 commits into from

Conversation

drzraf
Copy link
Contributor

@drzraf drzraf commented Sep 16, 2017

insert_comment hooks may come withouut $_POST being defined

@@ -6,6 +6,10 @@

add_filter( 'wp_insert_comment', 'process_ratings_from_comment' );
function process_ratings_from_comment($comment_id) {
if ( !isset($_POST['comment_post_ID']) || !isset($_POST["wp_postrating_form_value_$post_id"]) ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Use 'wp_postrating_form_value_' . $post_id instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return;
}

$post_id = intval($_POST['comment_post_ID']);
Copy link
Owner

Choose a reason for hiding this comment

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

$post_id = isset( $_POST['comment_post_ID'] ) ? (int) $_POST['comment_post_ID'] : 0;
$rate = isset( $_POST['wp_postrating_form_value_' . $post_id]; ) ? (int) $_POST['wp_postrating_form_value_' . $post_id ] : 0;

if( 0 === $post_id  || 0 === $rate ) {
    return;
}

I might be nitpicking and double standard, but going forward, I will be using (int) than intval() as (int) is much faster. I recommend coding using PHPStorm and https://plugins.jetbrains.com/plugin/7622-php-inspections-ea-extended-

Been using it for my workplace code and it is amazing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

intval(q) => (int) : done
About strict type checking I'm not totally fond of it here because not-set/empty/FALSE/0 are semantically identical in this case and I'd prefer to globally catch and reject any user-provided edge-case.
I've also the feeling that giving a default value (0) that we're going to reject a couple of lines after makes code understanding a bit more difficult.

NB: Emacs is a nice code-editor BTW

// Output AJAX Result
return the_ratings_results($post_id, $post_ratings_users, $post_ratings_score, $post_ratings_average);

return compact( "post_ratings_users", "post_ratings_score", "post_ratings_average", "post_ratings_rating");
Copy link
Owner

Choose a reason for hiding this comment

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

Strings should be using single quotes.

@lesterchan
Copy link
Owner

I know this plugin is too dated, if I have to redo it, I will follow the WP Coding Standards https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/

It includes strict typing as well. In the company I work for, we use WPCS https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards to make sure our code conform to standards. This plugin will probably break most of it. But if you are touching the code, might be a good idea to start it.

Raphaël Droz added 23 commits October 24, 2017 23:24
* avoid code/routine duplication
  especially for postratings_page_admin_most_stats() and get_ratings_images_vote()
* use ternary operator where it improve readability
* use here-doc where possible
* move wp_localize_script() where the_rating() actually display voting form
  and move part of its logic client-side => code simplification
* use document.getElementById() where it makes things simpler
  and alias jQuery to $j
* move some "static" file_exists() outside of loops and use a cleaner
  way to declare/implode() complex markup
* avoid double-escaping, let this task to wpdb() + use wpdb->insert()
If the option is enabled, voting will NOT be submitted through Ajax.
Instead one simply have to show the voting stars near the comment form submission, like with

```
// Notice the 4th arg = FALSE (== no Ajax submission)
function bind_rate_comments() { the_ratings('div', 0, true, false); }
add_action( 'comment_form' , 'bind_rate_comments' , 5 );
```

Voting will then happen in the comment insertion hook.
…the REST API). Less nested if's and more detailled error strings
…aluated high-noted items to overcome vastly-evaluated items. Ex with (0,2,3,11,5) vs (0,0,3,1,3): Traditional scoring: 2.9 vs 3.2. Bayesian scoring with 90% confidence: 3.4 vs 3.0
…ing data-* attribute and do the event binding later client-side

(rewrite and vastly improve javascript logic)
[documentation](https://codex.wordpress.org/Creating_Tables_with_Plugins)
> You must put each field on its own line in your SQL statement.

Indeed this is the condition for dbDelta() to work.
Current buggy and unique ALTER statement generated:
> ALTER TABLE wp_ratings CHANGE COLUMN `rating_id` rating_id INT(11) NOT
NULL auto_increment, rating_postid INT(11) NOT NULL, ...

With the fix:
> ALTER TABLE wp_ratings CHANGE COLUMN `rating_id` rating_id INT(11) NOT
NULL auto_increment
> ALTER TABLE wp_ratings CHANGE COLUMN `rating_postid` rating_postid INT(11)
NOT NULL
> ...
Raphaël Droz added 5 commits November 2, 2017 20:40
…ice of wp_postratings_check_rated()). It should now be possible to conditionnally enable/disable validation method according to the way the rate was submitted (comment/ajax/REST API/...)
Instead, themes may use things like
```
 echo str_repeat(sprintf('<img src="%s/rating_on.gif" alt="star" class="post-ratings-image" />',
                         plugins_url( 'wp-postratings/images/stars' ) ),
                 $comment->postrating_rate);
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants