Bug: Duplicated breadcrumbs

[kelsay]

Thank you for your input!

[zanardi]

Well, i discovered this thread AFTER i submit an official FIX in SVN. My fix was to check if we are in a menu item directly linked to a product id or category id, and in that case NOT to add VM pathway. I guess it could solve the duplicate item issue while at the same time keeping the pathway everywhere its needed.

For discussions and/or critics, my code is:

Code: [Select]

//retrieve menu parameters
$menus = &JSite::getMenu();
$menu  = $menus->getActive();
$menu_params = new JParameter( $menu->params );
$menu_product_id = $menu_params->get('product_id', 0);
$menu_category_id = $menu_params->get('category_id', 0);

// retrieve current VM element id
$vm_product_id = vmRequest::getVar('product_id', 0);
$vm_category_id = vmRequest::getVar('category_id', 0);

// avoid double pathway
if ( ( $menu_product_id != 0 && $menu_product_id == $vm_product_id ) or ( $menu_category_id != 0 && $menu_category_id == $vm_category_id ) ) {
return;
}


[kelsay]

@Zandardi

Can you please tell me where this code needs to go?  Which file?

[zanardi]

@kelsay:
this code, which still needs some testing, goes in the file mainframe.class.php, in the function vmAppendPathway. I report here the whole new function for clarity:

Code: [Select]

function vmAppendPathway( $pathway ) {

global $mainframe;

//retrieve menu parameters
$menus = &JSite::getMenu();
$menu  = $menus->getActive();
$menu_params = new JParameter( $menu->params );
$menu_product_id = $menu_params->get('product_id', 0);
$menu_category_id = $menu_params->get('category_id', 0);

// retrieve current VM element id
$vm_product_id = vmRequest::getVar('product_id', 0);
$vm_category_id = vmRequest::getVar('category_id', 0);

// avoid double pathway
if ( ( $menu_product_id != 0 && $menu_product_id == $vm_product_id ) or ( $menu_category_id != 0 && $menu_category_id == $vm_category_id ) ) {
return;
}

// Remove the link on the last pathway item
$pathway[ count($pathway) - 1 ]->link = '';

if( vmIsJoomla('1.5') ) {
$cmsPathway =& $mainframe->getPathway();
foreach( $pathway AS $item) {
$item->link = str_replace('&', '&', $item->link);
// make sure that ' (apostrophe) is converted to '
$item->name = html_entity_decode( $item->name, ENT_QUOTES );
$cmsPathway->addItem( $item->name, $item->link );
}
} else {
$tpl = vmTemplate::getInstance();
$tpl->set( 'pathway', $pathway );
$vmPathway = $tpl->fetch( 'common/pathway.tpl.php' );
$mainframe->appendPathWay( $vmPathway );
}
}


[franzpeter]

Did implement the code into my page (VM 1.1.5). The double breadcrumbs did disappear but the last entry for the product too. In the VM demo the breadcrumb shows: Home > Hand Tools > Hammer but with the patch class it shows only:

Home > Hand Tools (without Hammer as last entry). So the question is:
Should the last entry i.e. the product name appear or should it not appear in the breadcrumb trail?

[zanardi]

Well, i am not here to say what's right and what's wrong :-)

Obviously this should be fixed, my code should only be invoked if we ARE in a menu item which HAS a product id or category id parameter specified; and it should not change default behaviour. Some more testing is needed... i'll let you know as soon as i have news.

[doorknob]

Should the last entry i.e. the product name appear or should it not appear in the breadcrumb trail?

Today is the first time that I've looked at Zanardi's patch in detail. My original patch and Zanardi's alternative version are both trying to identify where allowing VirtueMart to add to the standard Joomla breadcrumbs is not desirable. Joomla creates breadcrumbs based on the menu structure (which it works out from the current value of Itemid, working backwards through the menu structure). The VM approach to menus is based on the principle that the category menu is provided by a module (which is not really a menu in the Joomla sense). A standard VM implementation has just one menu item (the home page). That's why a range of Joomla control and security features don't work in VM like they do in other components. When VM adds extra items to the breadcrumbs, it is because it is expecting the stub created by Joomla to correspond to just the home page only. VM then expects to add the items that correspond with it's own dummy menu. Those of us that reject the mickey mouse menu are stuck with having to cope with VM assuming that we are using it. My original patch works by checking whether the menu item had a different id from the home page. This approach allows the VM menu module to work in its quaint way but for regular menu items to be treated in the same way as non-VM items. This may be a problem for some pages. There are some pages where I want to add an extra item to the breadcrumbs (e.g. for speshitpillt cross-category searches such 'featured products' or 'on special'). For those, I added a second parameter to the vmAppendPathway() function to permit me to force an extra entry to the breadcrumbs when needed. My site doesn't use the cart and so there may be pages in that area that need special treatment. The alternative patch provided by Zanardi takes a different approach. Instead of a simple generic way to identify the pages where the extra breadcrumbs are not needed, it tries to positively identify those pages using quite complex logic. This approach is always going to need more and more complexity to cope with exceptions. For a start, it can't handle a manufacturer based browse. Looking at the detail, it seems to have other problems

Code: [Select]

$menu_product_id != 0 && $menu_product_id == $vm_product_idThis identifies menu items for a specific product and suppresses VM breadcrumbs. For products not linked to the current menu item, the breadcrumbs will have the full VM category structure and the product name added. It's not clear to me why this would be desirable.

Code: [Select]

$menu_category_id != 0 && $menu_category_id == $vm_category_idPresumably this is intended to apply to browse pages for categories but will also usually apply to product detail pages (which may be unintended). When menu items for specific products are correctly configured, they should have both a product_id and a category_id value. Even if that were not the case, the logic should allow for the possibility of users setting them up that way. To prevent this applying to product detail pages it would be necessary to add '&& $menu_product_id == 0'.
A better approach is to keep it simple and use the original patch. If the patch causes truncation of the breadcrumb for specific pages then it would be better to test the value of $page and allow exceptions on that basis. The original patch takes the premise that Joomla gets the breadcrumbs right and VM messes it up. The alternative patch takes the opposite view that VM's interference in the breadcrumbs is acceptable except under specific circumstances. You choose.

[zanardi]

Most of what you say is correct and your technical analysis is good. The addition to category_id check is a correct suggestion.

But I never meant to change the way VM produces is pathway. I started from a much simpler request: remove VM appended pathway whenever i manually create a menu item directly related to a specific product or category.

Example.

I have a shop which has a "BIKES" category and a "THUNDER" product. This is my menu:

HOME
SHOP (menu type: VirtueMart, no parameters)

I'll click on shop, pathway is Home -> Shop
I see categories listing, i click on BIKES, patwhay is Home -> Shop -> Bikes
I see product listing, i click on THUNDER, patwhay is Home -> Shop -> Bikes -> Thunder

Now i create another couple menu items, and my menu becomes:

HOME
SHOP   (menu type: VirtueMart, no parameters)
BIKES   (menu type: VirtueMart, category_id = 4)
THUNDER   (menu type: VirtueMart, product_id = 12)

With VM 1.1.5:
- if i click on BIKES menu item, pathway is Home -> Shop -> Bikes -> Bikes
- if i click on THUNDER menu item, pathway is Home -> Shop -> Thunder -> Bikes -> Thunder

This is what i am trying to address, because many people asked to do that. I'll stick with that for 1.1.6, with all aforementioned corrections.

[doorknob]

You haven't said why the more generic solution is not satisfactory

[aravot]

The following post http://forum.virtuemart.net/index.php?topic=77191.0 has a video of breadcrumb issue.

[doorknob]

All that video does is support the point I made that a generic solution such as the one I use will be more robust. I've been using that solution since the original post on this thread was made and it works in every situation that I've found including navigating to next and prior products from a detail page. Making the logic more complex is fraught with dangers. When I looked at what you proposed, I could see that manufacturer_id was not covered, the logic also assumed that on a detail page the category_id is zero. Going that way just digs a bigger and bigger hole.

[zanardi]

@aravot: thank you, but we have no problem understanding what the issue is: we are now discussing what's the best solution, so your video is mis-timed.

@doorknob: you are answering to Peter Osipof (aravot), not to me. I am Francesco Abeni (zanardi) and i was going to answer that your solution does in fact look more solid. As i said, i discovered this thread AFTER i made my modifications; i was going to have another look at it soon and i think i am going to use what you propose.

At my first look, i guessed that you were solving a different problem; but i can see i was wrong. So be happy and peaceful: i WILL use your solution and there's no need to argue anymore :-)

[aravot]

I know you understand the issue, I posted the video for those users who view the topic to understand it better, visual is always better.

[doorknob]

i was going to have another look at it soon and i think i am going to use what you propose.

You might also need to look at the getShopItemid function because the 'where' clause is so vague that it just picks the first vm menu item that it finds that is published. Usually that's ok because the 'home' page is the first one that was set up but it won't be for everybody. I made a separate post about that some time ago (suggested adding "AND params LIKE '%page=shop.index\\n%'" into the selection criteria).
Phil

[simply123]

great fix doorknob, simple and perfect