[#18924] - [4.0] Fix build SEF URL for component view without own menu item
- Fixed in Code Base
- 2 Feb 2018
- Medium
- Build: 4.0-dev
- # 18924
- Diff
- csthomas:siteroute_build_sef
User tests: Successful: Unsuccessful:
Pull Request for Issue #18923
Summary of Changes
- Always add prefix
/component/[COMPONENT_NAME]/if menu item component is not equal toquery[option]. - Optimise code, remove a code that set again
$query['Itemid'].
I removed a way when the build rule can change menu route by changing value inquery['Itemid'].
This example describe old way that I removed.
// Store Itemid in other variable
$itemID = empty($query['Itemid']) ? null : $query['Itemid'];
$crouter = $this->getComponentRouter($component);
// Now build rule can change query['Itemid']
$parts = $crouter->build($query);
// Restore Itemid if deleted
if (empty($query['Itemid']) && $itemID)
{
$query['Itemid'] = $itemID;
}
// After that we load menu item
$item = empty($query['Itemid']) ? null : $this->menu->getItem($query['Itemid']);Testing Instructions
See issue and code review.
Expected result
SEF links is build in correct way.
Note:
IMO similar PR also can be applied to J.3.x
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
Thanks, I will change one more line, replace isset() to empty(), then almost everything will be done, there is still problem with unit tests.
| Labels |
Added:
?
|
||
Could you please explain why you get $item from $query['Itemid'] before component router build process? Look like this change the original logic of the routing
As I understand, the original code give component router a chance to find correct Itemid (logical to me) for the routing, so we should try to get $item after component build process rater than before. So the change like this 4.0-dev...joomdonation:patch-2 seems more safe?
Could you please explain why you get $item from $query['Itemid'] before component router build process? Look like this change the original logic of the routing
In general, build URL process is split into 3 stages: (preprocess, build and postprecess).
Itemid is set up in preprocess stage, ex: MenuRules::preprocess()
SiteRouter::buildSefRoute() is called in build stage and calls StandardRules::build().
Maybe you would agree with me that StandardRules::build() or NomenuRules::build() should not change $query[Itemid] to different valid value.
Non SEF process does not call build stage.
IMO allowed option to change Itemid in build stage is wrong.
I have tested this item
This solves my issues reported in #18952.
Imho this is a very important fix as otherwise we will see issues arise with a lot of sites where there are no specific menuitems for a given component.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18924.
I have not tested this item.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18924.
I have tested this item
The PR solves the reported issue. I also did a code review, too
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18924.
| Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
I have tested this item
With PR . I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18924.
The code change looks good and the logic is correct. I would sign off on this.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-02-02 01:04:20 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
|
||
Thanks :)
@joeforjoomla Yes, this PR breaks it.
When you build a URL you can add your own Itemid to the query, otherwise core rules MenuRules will do it for you based on match to menu items.
Before (3.x) you could re-change Itemid again in SEF build method of your component router.
My purpose was:
- SEF and non SEF build should return the same
Itemid - the component router rule
buildis to build path in component, not to repair menu item id.
Tested this patch, it solves the issue of creating a new account from the module when we do not have a Registration Menu item.