Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

slt_cf_field_exists() is unreliable #24

Closed
burabure opened this Issue · 6 comments

2 participants

@burabure

The query on this function isn't working correctly and is unreliable at the least (didn't work at all on my wordpress 3.5.1 site, causing the admin meta boxes to display saved fields on the second query only)

The problem is the LIMIT statement without a preceding ORDER BY statement.

I fixed the error with this:
st-cf-lib.php @ 168

$field = $wpdb->get_results("
SELECT meta_value
FROM $table
WHERE meta_key = '$key'
  AND $id_field = $id
ORDER BY $id_field
  LIMIT 1");
@gyrus
Owner

I can't replicate it not working. Could you detail your test case?

I wonder if this is related to the fact that fields are only created in the DB when a value is entered - and if no value is entered, and a field currently exists, it's deleted. That came in in 0.7, though, a while ago. And within that logic, it seems to work fine.

Also, why would that SQL need an ORDER BY? As it's limited to a max of 1 result, ordering seems to be irrelevant. Let me know if I'm missing something.

Nice avatar BTW :-)

@burabure

Hi Gyrus,
I'm developing in a Ubuntu virtual server with an up to date LAMP stack.

The limit/orderby gotcha is dependent on the database environment (system, config, version...), and while limit alone works sometimes (maybe most of the times in a typical wordpress enviroment) it's actually very unreliable.

Now the thing that bothers me is that I was getting the correct output on fields 2,4,6,8... meaning maybe the query or part of the results weren't evaluated instantly.

Anyway, having and orderby everytime you use limit is good practice and solves the issue =)

@gyrus
Owner

Hmm, still don't understand this "gotcha". Is it just a weird bug in SQL on Linux? Sounds like the kind of thing you end up doing for Internet Explorer in CSS, not doing DB queries on Linux!

I guess since the ORDER BY is technically irrelevant I could just drop it in. I'm just wary of sticking code that I don't understand into the plugin, for an issue I can't replicate :-/

Regarding the "test case", I was more interested in the specific steps you take to create this error. I've tried all sorts of combinations: old post, new field; new post, show preview just after first draft save, etc... No luck.

@burabure

test case:
https://gist.github.com/3amsleep/5086276
create a post (post or custom post type, doesn't matter), enter data into the custom fields, publish/update. only custom field #2 shows its saved data in the editor. Note: the data is saved, the problem is that slt_cf_field_exists() returns false and the field is populated with the default value.

i dont think the problem is UNIX/ubuntu related, it's probably a mysql related issue. But like i said, sql servers can basically order by randomly if you dont specify, and sql interpreters can interpret a groupby-less LIMIT unreliably, so it's always good practice to use limit and group by together

@burabure

i've tested some more and it seems that the problem is caused by the WPMUDEV events+ plugin.

seems that this is the problem

/**
     * Late-bound `$wpdb` query filter.
     */
    function _eab_wpdb_filter_postmeta_query ($q) {
        global $wpdb;
        if (!preg_match('/^\s*SELECT/i', $q)) return $q;
        $postmeta = preg_quote($wpdb->postmeta, '/');
        if (preg_match("/\b{$postmeta}\b/", $q) && !preg_match('/\bORDER BY\b/i', $q)) $q .= " ORDER BY {$wpdb->postmeta}.meta_id";
        remove_filter('query', '_eab_wpdb_filter_postmeta_query'); // Clean up
        return $q;
    }

    /**
     * Late binding filter for forced query ordering on postmeta requests.
     */
    function _eab_filter_meta_query ($check) {
        add_filter('query', '_eab_wpdb_filter_postmeta_query');
        return $check;
    }
    add_filter('get_post_metadata', '_eab_filter_meta_query');

Some quality code there eh?... gosh...XD

@burabure burabure closed this
@gyrus
Owner

Thanks for looking deeper! There was another recent clash with that plugin: #23

I'll add a note about this in the readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.