[#5105] - Tags: Fixing routing
- Closed
- 30 Dec 2014
- Medium
- Build: staging
- # 5105
- Diff
- Hackwar:com_tags_routing
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
I seriously don't know how this ever could have worked correctly...
We'll start by seeing that all tags that are associated with a language can not be reached, because they are stored in $lookup[$language][$view], but lookup later is only done on $lookup[$view]. So we are correcting this in a way that all tags are first of all associated with a language and then with a view. (See $lookup[$lang][$view])
Then we see that whatever we do, we override the caching of the lookup in line 151 anyway, so we can shorten that code tremendously.
Further we see that in line 159 the $language of the needles is overwritten with the $language from one random menu entry. This is done exactly once and not used thereafter, except that further cache lookups are stored as that $language. Since all this has no use anyway, we can simply remove this. (For reference, that parameter should somehow filter by language, but that is not the task of this class. That filtering has to be done earlier in the model.)
Last but not least, we are doing the lookup by both the view and the language.
This whole PR does not adress the fact that it does not support menu items with more than one tag, that there is only one view that it is checked against, that there is no fallback to the tags view, that _findItem() is only used in one method and thus completely unnecessary and could simply be merged into getTagRoute() and that it is doing A LOT of additional work that simply is not necessary. But at least this way it is a lot more correct and its behavior is more deterministically than before.
No testing instructions from me so far, since I found this during a code review. Asking a maintainer to review this.
Votes
jissues-bot
- | Labels |
Added:
?
|
||
| Status | Pending | ⇒ | Needs Review |
brianteeman
- | Category | ⇒ | SEF Tags |
Although I have not tested this, careful with this one. Until now we have advised users to not assign languages to Tags but rather make sure they are filtered by setting com_tags Options "Language Filter" to "current".
(When using multilanguage)
I just tested, site gets broken with this error.
Parse error: syntax error, unexpected '[', expecting '(' in *********/com_tags/helpers/route.php on line 136.
Joomla 3.3.6 multilanguage - My tags are assosiated with languages.
Sorry, that was a stupid bug. Fixed it.
No worries. 
Tested and it's working properly. The file is up and running live.
We have some SEO issues with tags (like before).
I only have tested tags associated with languages.
1) Deleted tags don't redirect to 404 Error page, but to an empty page with the full template of the site. GWMT gives 404 Soft error for this links (empty page instead of 404 error)
2)Tags associated with languages working for all languages. Example: An english tag can be displayed to the Greek site like normal if you change the language code manually from url.
I have also changed the title of the tag for signle tag view from H2 to H1.
Regarding 1): That is the same behavior as before.
Regarding 2): That is also the same behavior as right now. The tags component currently simply does not seem to support languages at all...
The thing is, that this fixes a bunch of simple programming mistakes... This still needs a code review. 
Yes, tags if used properly, they have a big impact to SEO. Tags should be fixed.
@ManuxGR As you already tested this, can you please detail your testing methodology? Also, it seems that you have an success test, but in http://issues.joomla.org/tracker/joomla-cms/5105 it is still logged as a failure...
@ManuxGR Yes, to fix your "vote" just go to http://issues.joomla.org/tracker/joomla-cms/5105 login with GitHub and change your vote.
Thanks Hannes for your hard work on this important Joomla routing problem..Merry Christmas to you!
Maintainer Review: As there is no b/c being broken here and it's cleaning up the code hugely I'm happy with this in theory (note haven't tested). Please can we have some detailed test instructions tho :)
Fixed your codestyle remark.
As I wrote in my initial comment: I found this during a code review and don't have any idea which bugs I'm fixing with this... I just know that it is wrong...
| Status | Needs Review | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-12-30 02:20:59 |
@test couldn't find any issues with the patch when applied. Tried navigating around various tag views some attached to menu items some not. Merging.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5105.
Moving this to Needs Review. Thanks
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5105.