code needs some serious cleanup

_ck_
(@_ck_)

12 years, 8 months ago
I’ll be the first to admit my own code is not great at times and I often go back to rewrite and fix things. However the code in this plugin seems to be full of sub-optimal processes.
```
$post = $wpdb->get_results("SELECT * FROM {$wpdb->posts} WHERE id = $post_id");
```
All you are using out of that fetch is post_content, so only fetch post_content. Also you can use get_var to get just one element. Also use LIMIT
```
$post_content = $wpdb->get_var("SELECT post_content FROM {$wpdb->posts} WHERE id = $post_id LIMIT 1");
```
(then you don’t have to use $post[0]->post_content)

then there is
```
$matches = array();
    preg_match_all('/<\s*img [^\>]*src\s*=\s*[\""\']?([^\""\'>]*)/i', $post[0]->post_content, $matches);
    if (count($matches)) {
```
This is not how PHP works. That $matches is going to be set as false if there is no match, it won’t be an array at all. Do something like:
```
if (preg_match_all('/<\s*img [^\>]*src\s*=\s*[\""\']?([^\""\'>]*)/i', $post[0]->post_content, $matches)) {
```
then there is
```
$image = substr($image, strpos($image, '"')+1);
 $result = $wpdb->get_results("SELECT ID FROM {$wpdb->posts} WHERE guid = '".$image."'");
 $thumb_id = $result[0]->ID;

if (!$thumb_id) {
```
Seriously? WTF – What if there is no result? What if there are special characters in the image url? etc. etc.

Try this instead (replacing the first few lines
```
$thumb_id=$wpdb->get_var("SELECT ID FROM {$wpdb->posts} WHERE guid = '".mysql_real_escape_string(trim(trim(trim($image),"'\"")))."' LIMIT 1");
```
That way get_var will return false if none and the three trims tries to clean up the url.

Also, the reason why this plugin fails on some installations is because users are unaware images are fetched EXTERNALLY not through the local storage but via the web, even if local.

The allow_url_fopen detection code is not good enough, file_get_contents is not a robust way of getting data from urls and can fail – doesn’t WordPress have a built-in fetch now? Anyway, give novice users the ability to force curl.

As a plugin creator I realize there is virtual no payout for making plugins like these but if you want to learn how to code better, you should go through every single line of this plugin and clean it up.

Viewing 2 replies - 1 through 2 (of 2 total)

Thread Starter _ck_
(@_ck_)

12 years, 8 months ago
Are you certain this mysql query is not causing a sub-query ON EVERY POST ROW? Because this is a known mysql bug that it most likely will:
```
$query = "SELECT * FROM {$wpdb->posts} p where p.post_status = 'publish' AND p.ID NOT IN (
                        SELECT DISTINCT post_id FROM {$wpdb->postmeta} WHERE meta_key IN ('_thumbnail_id', 'skip_post_thumb')
                      )";
```
Most people think that means mysql will do the subquery first, then do the outer select. Except it doesn’t. It does the outer select first and then does the subquery over and over and over again for each post. Also, why are you fetching the entire row (*) when you only need the ID.

Please read up on the different $wpdb functions, get_results is not the only one, you can get_col (and get_row).
iceq
(@iceq)

12 years, 5 months ago

Hi CK.. I am no code savvy, but if it is that serious, would you please consider to email the author directly.. he may have missed this topic.

Viewing 2 replies - 1 through 2 (of 2 total)

The topic ‘code needs some serious cleanup’ is closed to new replies.