[Plugin: Events Manager] Pagination of search results has SQL bug

  • Resolved webaware

    (@webaware)


    12 years, 10 months ago

    G’day, using EM 5.0.50 and have found a problem with the SQL used to count search results, and pagination in the events-list.php template.

    Firstly, the events-list.php template does not pass the search arguments onto em_paginate() so the search form is cleared when you click through to a page from pagination. When $args is supplied, the page number isn’t passed through properly, due to simply adding the search arguments array to another array. An array merge will ensure the second array’s values override the first array:

    @28:
    $search_args = array_merge(EM_Events::get_post_search($args), array('page'=>'%PAGE%','action'=>'search_events'));

    Secondly, class EM_Events will never count rows correctly when there is an offset specified, because the result set has only one row (the count) and an offset scrolls that out of bounds. Thus, when ::get is called with $count=true you need to set $limit and $offset to empty strings:

    @58:

    $limit = $offset = '';
if (!$count) {
    $limit = ( $args['limit'] && is_numeric($args['limit'])) ? "LIMIT {$args['limit']}" : '';
    $offset = ( $limit != "" && is_numeric($args['offset']) ) ? "OFFSET {$args['offset']}" : '';
}

    https://www.ads-software.com/extend/plugins/events-manager/

Viewing 9 replies - 1 through 9 (of 9 total)
  • Plugin Author Marcus (aka @msykes)

    (@netweblogic)

    will check this out

    Plugin Author Marcus (aka @msykes)

    (@netweblogic)

    I’m inclined to disagree on both your fixes here, on what instances/situations does this actually cause a bug for you?

    esp on the second fix, there are various instances where we still want a count for events on page x, so it shouldn’t force counts to have no limits. by default if a limit or offset isn’t passed, it should calculate without using limits/offsets

    Thread Starter webaware

    (@webaware)

    G’day Marcus,

    This happens when you do a search, then click through on the numbered pagination links. Without the fixes, if I type in something in the search field it does not carry through to the pagination links; print_r($search_args) just before calling em_add_get_params() gives this (NB: no search/em_search argument):

    Array
(
    [page] => %PAGE%
    [action] => search_events
)

    and $page_link_template returned is this:

    /event-search?country=AU&page=%25PAGE%25&action=search_events

    I figured it was because EM_Events::get_post_search() was being called with no $args, which then defaults to an empty array, and no search is performed. Thus, the pagination count is for a query with no search criteria (so all events), and the page number doesn’t make it into the search criteria so there is no offset (or limit) when counting.

    When I put $args into EM_Events::get_post_search() it did the count correctly, but because the arrays were added instead of merged, the page number was always 1 in all of the pagination links; using array_merge() allowed the second array to overwrite the array element for ‘page’ with ‘%PAGE%’ and the links had the correct page numbers again.

    With this all working, the count broke when page > 1. The count should be for all rows in the search, to get total rows, which you then break up into pages in the events-list.php template. Instead, the query selected one row (with the count, which is correct) and then applied a limit and offset to it, so for page 2+ this meant the result set was empty because (with a limit of 10) the offset was 10 which is > 1 thus no rows selected. Hence the patch to only provide a limit and offset when not counting.

    Maybe there’s a better way of doing this, but the fixes above got me going again at least.

    Thread Starter webaware

    (@webaware)

    G’day Marcus,

    Looking further, my fix for the search terms added ‘limit’ into the arguments passed in the pagination links, which broke the count query. Your code doesn’t add limit and offset if limit is missing, so adding limit was the problem. adding the following after my array_merge() stop the count from breaking:

    unset($search_args['limit']);

    And empty arguments can be removed with this:

    $search_args = array_filter($search_args, 'strlen');

    So, no need to patch EM_Events::get() after all.

    Plugin Author Marcus (aka @msykes)

    (@netweblogic)

    Hi, I’ll certainly fix the bug (you may be right), but I still can’t actually reproduce an instance where this happens. For example:

    visit https://demo.wp-events-plugin.com/events/ and filter category Jazz events. Try the pagination there and you’ll see it’s fine.

    That page %PAGE% value is important and shouldn’t be overriden by an actual page value, because the pagination script uses that as a link template and replaces that with the page number for each link.

    Thread Starter webaware

    (@webaware)

    G’day Marcus, that demonstrates the problem nicely. Search on young, and select page two: results of page two are of all events, not just of young, because the search term has been lost.

    Plugin Author Marcus (aka @msykes)

    (@netweblogic)

    thanks for working with me on that, it explains it very nicely ??

    Will have another look at this and sort it out

    Hi webaware, just had a look at this (thx for your help debugging), the fix for this was to modify the get_post_search() function, first line, along with that array merge to remove the ‘search’ attribute. The demo works fine now, and I’ve committed the changes just now to the dev

    This is to do with the fact that the form uses em_search instead of ‘search’, like normal EM queries do, to prevent conflicts with plugins. Since the search results are generated only on events-list.php I’ve made sure em_search is used in the link generation.

    Now… onto the other issue. How do I reproduce a problem which is fixed by your second snippet on your first post (r.e. limits/offsets)?

    Thread Starter webaware

    (@webaware)

    G’day Marcus,

    That’s good news. Of course it was the change from search to em_search, why didn’t I think of that?

    Regarding the limits/offsets bit, no fix required per my comment above. It was only an issue because of what I had done to the search_args array.

    cheers,
    Ross

    [edit to fix comment link]

Viewing 9 replies - 1 through 9 (of 9 total)
  • The topic ‘[Plugin: Events Manager] Pagination of search results has SQL bug’ is closed to new replies.