Tags: Fixing routing #5105

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
8 participants
@Hackwar
Member

Hackwar commented Nov 14, 2014

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.

components/com_tags/helpers/route.php
@@ -131,32 +131,23 @@ protected static function _findItem($needles = null)
{
if (isset($item->query) && isset($item->query['view']))
{
+ $lang = ($item->language != '' ? $item->language : '*');
+ if (!isset(self::lookup[$lang]))

This comment has been minimized.

@zero-24

zero-24 Nov 14, 2014

Contributor

@Hackwar can you add a clear line bevor the if statement

@zero-24

zero-24 Nov 14, 2014

Contributor

@Hackwar can you add a clear line bevor the if statement

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Nov 14, 2014

Contributor

No testing instructions from me so far, since I found this during a code review. Asking a maintainer to review this.

Moving this to Needs Review. Thanks

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5105.

Contributor

zero-24 commented Nov 14, 2014

No testing instructions from me so far, since I found this during a code review. Asking a maintainer to review this.

Moving this to Needs Review. Thanks

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5105.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 20, 2014

Member

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)

Member

infograf768 commented Nov 20, 2014

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)

@ManuxGR

This comment has been minimized.

Show comment
Hide comment
@ManuxGR

ManuxGR Nov 20, 2014

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.

ManuxGR commented Nov 20, 2014

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.

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 20, 2014

Member

Sorry, that was a stupid bug. Fixed it.

Member

Hackwar commented Nov 20, 2014

Sorry, that was a stupid bug. Fixed it.

@ManuxGR

This comment has been minimized.

Show comment
Hide comment
@ManuxGR

ManuxGR Nov 20, 2014

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.

ManuxGR commented Nov 20, 2014

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.

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 20, 2014

Member

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. 😄

Member

Hackwar commented Nov 20, 2014

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. 😄

@ManuxGR

This comment has been minimized.

Show comment
Hide comment
@ManuxGR

ManuxGR Nov 20, 2014

Yes, tags if used properly, they have a big impact to SEO. Tags should be fixed.

ManuxGR commented Nov 20, 2014

Yes, tags if used properly, they have a big impact to SEO. Tags should be fixed.

@smanzi

This comment has been minimized.

Show comment
Hide comment
@smanzi

smanzi Dec 1, 2014

@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...

smanzi commented Dec 1, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@ManuxGR

ManuxGR Dec 1, 2014

@smanzi the failure was before the fix. Is there a way to change it? And this doesn't fix nothing. It's just the same code updated - modernized.

ManuxGR commented Dec 1, 2014

@smanzi the failure was before the fix. Is there a way to change it? And this doesn't fix nothing. It's just the same code updated - modernized.

@smanzi

This comment has been minimized.

Show comment
Hide comment
@smanzi

smanzi Dec 1, 2014

@ManuxGR Yes, to fix your "vote" just go to http://issues.joomla.org/tracker/joomla-cms/5105 login with GitHub and change your vote.

smanzi commented Dec 1, 2014

@ManuxGR Yes, to fix your "vote" just go to http://issues.joomla.org/tracker/joomla-cms/5105 login with GitHub and change your vote.

@zero-24

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 Dec 1, 2014

Contributor

@ManuxGR

the failure was before the fix. Is there a way to change it?

alter your test as "Not tested" and after this as "tested successfully"

Contributor

zero-24 commented Dec 1, 2014

@ManuxGR

the failure was before the fix. Is there a way to change it?

alter your test as "Not tested" and after this as "tested successfully"

@Hackwar Hackwar referenced this pull request Dec 9, 2014

Merged

Fixing tag routing #5347

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 9, 2014

Member

While this fixes the TagsHelperRoute class up, the router is fixed in #5347. So please have a look there, too. Apply both PRs and it should make a world of difference. 😉

Member

Hackwar commented Dec 9, 2014

While this fixes the TagsHelperRoute class up, the router is fixed in #5347. So please have a look there, too. Apply both PRs and it should make a world of difference. 😉

@ssnobben

This comment has been minimized.

Show comment
Hide comment
@ssnobben

ssnobben Dec 11, 2014

Thanks Hannes for your hard work on this important Joomla routing problem..Merry Christmas to you!

Thanks Hannes for your hard work on this important Joomla routing problem..Merry Christmas to you!

components/com_tags/helpers/route.php
@@ -88,14 +88,10 @@ public static function getTagRoute($id)
}
else
{
- if (!empty($needles) && $item = self::_findItem($needles))
+ $link = 'index.php?option=com_tags&view=tag&id=' . $id;

This comment has been minimized.

@wilsonge

wilsonge Dec 14, 2014

Contributor

Cause I'm an arse can we add a new line after above the if() statement please?

@wilsonge

wilsonge Dec 14, 2014

Contributor

Cause I'm an arse can we add a new line after above the if() statement please?

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Dec 14, 2014

Contributor

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 :)

Contributor

wilsonge commented Dec 14, 2014

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 :)

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Dec 14, 2014

Member

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...

Member

Hackwar commented Dec 14, 2014

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...

components/com_tags/helpers/route.php
@@ -131,32 +128,23 @@ protected static function _findItem($needles = null)
{
if (isset($item->query) && isset($item->query['view']))
{
+ $lang = ($item->language != '' ? $item->language : '*');

This comment has been minimized.

@wilsonge

wilsonge Dec 29, 2014

Contributor

Can we have a new line in between this line and the if statement please for code style

@wilsonge

wilsonge Dec 29, 2014

Contributor

Can we have a new line in between this line and the if statement please for code style

This comment has been minimized.

@Hackwar

Hackwar Dec 29, 2014

Member

For you, I'd even add 2. 😉

@Hackwar

Hackwar Dec 29, 2014

Member

For you, I'd even add 2. 😉

@wilsonge wilsonge closed this in 183173c Dec 30, 2014

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Dec 30, 2014

Contributor

@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.

Contributor

wilsonge commented Dec 30, 2014

@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.

@Hackwar Hackwar deleted the Hackwar:com_tags_routing branch Dec 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment