Wrong album number in search results when requested from a search linked context

[bic-ed]

Perhaps there is no need for checking not visible album too, as they are not in the search results. Plus, this is breaking any attempt to get the proper number of albums in a search linked context, because also not visible albums are included. https://github.com/bic-ed/zenphoto/blob/9a80714f351c9e6899c8465681ad171ed76a0ab6/zp-core/functions/functions.php#L932

Am I missing any side effect of removing the $_zp_loggedin manipulation from there? From

//  see if the album is within the search context. NB for these purposes we need to look at all albums!
$save_logon = $_zp_loggedin;
$_zp_loggedin = $_zp_loggedin | VIEW_ALL_RIGHTS;
$search_album_list = $_zp_current_search->getAlbums(0);
$_zp_loggedin = $save_logon;

to

$search_album_list = $_zp_current_search->getAlbums(0);

[bic-ed]

Hi again Malte, I opened this on my fork because I can see that you are overwhelmed whit many things to fix elsewhere and I don't want to put any more pressure on you, but at the same time I don't want to forget this, as it toke a really huge effort to me for finding where the heck was the issue which was preventing getting the correct number of searched albums when in a search linked context. Whenever you have any time, please take a look at this.

[bic-ed]

@acrylian, I added you to this fork because otherwise I couldn't even tag you. Now you should get al the notifications as well. Anyway, like I said above, no pressure at all, I just needed to be sure you will read this sooner or later.

[acrylian]

All right, probably so you cannot "spam" random users ;-) Not really familiar offhand with all this and I have no idea what $_zp_loggeind contains or where it is set and why not our general function zp_loggedin(() is used here. Curse of globals as usual. Generally lines like that should make sure users with rights get what fits their level of course (the | operator is the special here). The whole search linked stuff is not really easy.

[bic-ed]

I investigated this for a long time, because I was trying to implement a system to get the right link to the search page containing the image when browsing through the images resulting from the search.

The way it works now, if you go from the search page to an image, the link in the breadcrumb contains the right search page you had started from, but if you then navigate through the images ending up at an image that belongs to another page in the search results, the link does not change, so it still leads to the search page you had started from, no matter where the image belongs.

The problem: in image.php it is impossible to get the correct number of albums from search to calculate the page to which an image belongs, because even albums that are not visible are included in the album count, regardless of the system used. For the Multiverse theme, I ended up using a cookie, set in search.php with the correct number of albums and read in image.php to build the correct page number for the image. Easy solution but exaggerated means.

Now I've finally found why the number of albums is returned wrong, that is the code I posted, which apparently is totally wrong, as the comment says:

// see if the album is within the search context. NB for these purposes we need to look at all albums!

that is not true at the moment. Perhaps it has been in the past, since the last modification of that code was 13 years ago! In the mean time the way protected albums are returned in search results is changed more than once.

Plus, if you look at what the code does after that, it just sets the value of the cookie zpcms_search_parent to _searchresultsalbum if current album is in search results, while does nothing but messing up the albums count when in an image page and search linked context.

Now, a non-visible album, with the actual policies, is never in the search results, so there is no point in forcing a fake $_zp_loggeind to include non-visible albums in that check.

[acrylian]

Now, a non-visible album, with the actual policies, is never in the search results, so there is no point in forcing a fake $_zp_loggeind to include non-visible albums in that check.

Not exactly true because if you are a loggedin user with rights to those they are! So the check in the current form maybe wrong but it is necessary. Regardless good find because I never paid attention (and given missing reports generally not many others) to that and our search context - which is also cookie based of course - is quite a speciality of ZP no other system has to my knowledge at least.

Anyway we get to this later you already mentioned. First I need to fix the metadata bug that we caused ourselves basically…

[bic-ed]

Not exactly true because if you are a loggedin user with rights to those they are! So the check in the current form maybe wrong but it is necessary.

Of course the check has to be done, but I forgot to mention that a proper check, without the need for "faking" $_zp_loggedin, is done later, using the new function isVisible(). Precisely here: https://github.com/zenphoto/zenphoto/blob/2114037ab0eb192ba835c23dd15f188a98dc6ad5/zp-core/classes/class-searchengine.php#L1438

[bic-ed]

Adding this here as somehow related with the above. At this point of the code, getAlbumList() is empty for some reasons: https://github.com/bic-ed/zenphoto/blob/2114037ab0eb192ba835c23dd15f188a98dc6ad5/zp-core/template-functions.php#L1486 so the parents albums are always removed from the breadcrumb on this line: https://github.com/bic-ed/zenphoto/blob/2114037ab0eb192ba835c23dd15f188a98dc6ad5/zp-core/template-functions.php#L1514 I made some tests using getAlbums() instead and that seems to work.

[acrylian]

All that search context is probably a bit overcomplicated and we should have just presented results without any later context as everyone else does. Would have saved us some cookie ;-)

Is this perhaps for albums that are not actaully search results itself? For example sub albums whose top level was search result? Then it at least technically would be correct even if unexpected.

[bic-ed]

Is this perhaps for albums that are not actaully search results itself? For example sub albums whose top level was search result? Then it at least technically would be correct even if unexpected.

Is for that case indeed, but is not working as expected, because even if a parent album is present in the search results its crumb gets unset as $search_album_list is always empty. For instance if you have album_a and album_b both in search results, being b children of a (album_a/album_b), you get a breadcrumb like gallery>search>album_b while expecting gallery>search>album_a>album_b when browsing album_b I agree that all of this stuff is overcomplicated though.

[acrylian]

For instance if you have album_a and album_b both in search results, being b children of a (album_a/album_b), you get a breadcrumb like gallery>search>album_b while expecting gallery>search>album_a>album_b when browsing album_b

Okay if both are search results perhaps, but you could also see it this way: Both are search results and therefore in search context their direct parent is the search results and not any real parent…

I tend to always open results in a new tab as I am so used to from everywhere else… Btw, we don't have any search context for articles or pages!

[bic-ed]

Ok, then we might just remove the not working code to leave just the last children crumb as it is erroneously now.

[bic-ed]

Or we could just fix the first issue of this topic (I've done more testing and I'm pretty sure that the manipulation of zp_logeddin is non needed at all anymore) and leave the breadcrumb as it is now. Then if someone wants to reconsider and recode the search linked stuff it can be done anytime in the future.

[acrylian]

I really haven't taken any look at this as the meta data stuff kept me busy - I rarely looked at it and it is not yet ready (please test that, too - see the ticket please).

I will try to take a look at the breadcrumb stuff to see how off it feels and if we perhaps should fix it. Is the numbering perhaps even related to that as well? But first the metadata stuff as that is far more important for us as a photo gallery CMS:

[bic-ed]

I really haven't taken any look at this as the meta data stuff kept me busy - I rarely looked at it and it is not yet ready (please test that, too - see the ticket please).

I have seen the commit, just need some time to pull it locally cause I'm experimenting changes in functions.php as well and at the moment I have a different version.

I will try to take a look at the breadcrumb stuff to see how off it feels and if we perhaps should fix it. Is the numbering perhaps even related to that as well?

The numbering relies on the zp_current_search->page, which is defined elsewhere though, and is never updated while browsing images in search results. It could be updated properly (I'm doing it in my theme) with the changes proposed in the first comment of this issue.

[bic-ed]

I've found another page number issue here: https://github.com/bic-ed/zenphoto/blob/a6bfef0f35566300f9b5e80ddacdded52edb3f08/zp-core/classes/class-albumbase.php#L666 That function is called often passing the page as parameter, but if not, such as when called from getAlbumBreadcrumb(), uses the global $_zp_page to get the page number, and $_zp_page in an image page is always 1. So it would be better to use the getAlbumPage() function instead of $_zp_page.

I suggest to fix both the opening issue of this topic and this last one, while leaving the rest as is for the moment, as it is probably useless adding to much context to search results and is a big head hake to do it properly.

[acrylian]

Have to think about that. getAlbumPage() is a template function and should not be used in the core classes as it might not be available. Since I plan some restructuring especially of the image class by moving various image size calculation logic that is currently also template-functions only to the class itself where it should be (e.g. we have to included template functions on some admin pages to be able to calculate resized sizes!)

[bic-ed]

I see, than it could be used in getAlbumBreadcrumb() to pass the proper page as a parameter. Not a final solution but fixes the breadcrumb at least. If you wish, I can search for any other call to album->getLink to be sure the page number is always given as parameter.

[acrylian]

If it can be fixed in the breadcrumb functions itself, sure. The page number must not alway be passed as you sometimes just want the plain link to an album of course.

But please do some research. But remember the double work any small change causes for me. Also let's first again be sure the metadata stuff is correctly working. Fred reported something so I need to change a few things again.

[bic-ed]

Ok, saving as a reminder for later...

Yes, it can be fixed in the breadcrumb by changing https://github.com/bic-ed/zenphoto/blob/a6bfef0f35566300f9b5e80ddacdded52edb3f08/zp-core/template-functions.php#L1389 with

return array('link' => $album->getLink(getAlbumPage()), 'text' => $title, 'title' => getBare($title));

And updating printAlbumBreadcrumb() as well if you want the page number also printed in the breadcrumb, which is not that relevant as for image page the bc is unique anyway..

[acrylian]

Ok,sounds great. As soon as the metadata ticket is closed we can do that. It's just that I just recently forgot to port things and only noticed accidentally…

[bic-ed]

I have a proposal for handling consistently all this breadcrumb staff, but it needs a slightly different approach.
I'm putting it down here as a reminder, also because next week, from 14 to 21, I'll be away for a short vacation.

1. Fix the $search_album_list as proposed on the first comment of this issue
2. Once the above is done, it is possible to get the proper search page while browsing images in search results. I have already tested the code for that
3. Fix the page number using getAlbumPage() in getAlbumBreadcrumb(), as proposed in my last comment before this one
4. This is the different approach part. Skip completely the unset code in getParentBreadcrumb(), as sub-albums of an album which is in the search result are actually in the search results, even if not directly, since they are reachable in the search context along with their images, so they appear when browsing the search results and their missing crumbs in the breadcrumb are somehow disorienting

I place here the proposed recode for getParentBreadcrumb(), so you can test it anytime, involving Fred if you wish

function getParentBreadcrumb() {
  global $_zp_current_search, $_zp_current_album;
  $parents = $output = array();
  if (in_context(ZP_SEARCH_LINKED)) {
    if (in_context(ZP_IMAGE) && !in_context(ZP_ALBUM_LINKED)) {
      $alb_pages = ceil($_zp_current_search->getNumAlbums() / max(1, getOption('albums_per_page')));
      $img_pages = ceil((imageNumber() - getFirstPageImages()) / max(1, getOption('images_per_page')));
      $page = $alb_pages + $img_pages;
    } else {
      $page = $_zp_current_search->page;
    }
    $searchwords = $_zp_current_search->getSearchWords();
    $searchdate = $_zp_current_search->getSearchDate();
    $searchfields = $_zp_current_search->getSearchFields(true);
    $searchpagepath = SearchEngine::getSearchURL($searchwords, $searchdate, $searchfields, $page);
    $dynamic_album = $_zp_current_search->getDynamicAlbum();
    if (empty($dynamic_album)) {
      if (empty($searchdate)) {
        $output[] = array('link' => $searchpagepath, 'title' => gettext("Return to search"), 'text' => gettext("Search"));
        if (is_null($_zp_current_album)) {
          return $output;
        } else {
          $parents = getParentAlbums();
        }
      } else {
        return array(array('link' => $searchpagepath, 'title' => gettext("Return to archive"), 'text' => gettext("Archive")));
      }
    } else {
      $album = $dynamic_album;
      $parents = getParentAlbums($album);
      if (in_context(ZP_ALBUM_LINKED)) {
        array_push($parents, $album);
      }
    }
  } else {
    $parents = getParentAlbums();
  }
  if ($parents) {
    array_push($parents, $_zp_current_album);
    $index = -1;
    foreach ($parents as $parent) {
      $index++;
      if ($index != 0) {
        $parentparent = $parents[$index - 1];
        $page = $parent->getGalleryPage();
        $url = $parentparent->getLink($page);
        $output[] = array('link' => html_encode($url), 'title' => $parentparent->getTitle(), 'text' => $parentparent->getTitle());
      }
    }
  }
  return $output;
}

[acrylian]

Okay, will test it and the current behaviour. I just checked, if it is just that function I can even directly port it.

[bic-ed]

I just checked, if it is just that function I can even directly port it.

Unfortunately not, there are also those in steps 1 and 3, namely handleSearchParms() and getAlbumBreadcrumb()

[acrylian]

I think we had breadcrumbs already aligned (returning arrays) so we can port them more easily. We'll see. Seems we are not yet finished with meta data.