PR-staging

Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
14 Nov 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.

Votes

# of Users Experiencing Issue
1/2
Average Importance Score
4.00

avatar Hackwar Hackwar - open - 14 Nov 2014
avatar jissues-bot jissues-bot - change - 14 Nov 2014
Labels Added: PR-staging
avatar zero-24 zero-24 - change - 14 Nov 2014
Status Pending Needs Review
avatar zero-24
zero-24 - comment - 14 Nov 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.

avatar brianteeman brianteeman - change - 14 Nov 2014
Category SEF Tags
avatar ManuxGR ManuxGR - test_item - 16 Nov 2014 - Tested unsuccessfully
avatar infograf768
infograf768 - comment - 20 Nov 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)

avatar ManuxGR
ManuxGR - comment - 20 Nov 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.

avatar Hackwar
Hackwar - comment - 20 Nov 2014

Sorry, that was a stupid bug. Fixed it.

avatar ManuxGR
ManuxGR - comment - 20 Nov 2014

No worries. :+1: 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.

avatar Hackwar
Hackwar - comment - 20 Nov 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. :smile:

avatar ManuxGR
ManuxGR - comment - 20 Nov 2014

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

avatar smanzi
smanzi - comment - 1 Dec 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...

avatar ManuxGR
ManuxGR - comment - 1 Dec 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.

avatar smanzi
smanzi - comment - 1 Dec 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.

avatar zero-24
zero-24 - comment - 1 Dec 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"

avatar ManuxGR ManuxGR - test_item - 1 Dec 2014 - Not tested
avatar ManuxGR ManuxGR - test_item - 1 Dec 2014 - Tested successfully
avatar Hackwar
Hackwar - comment - 9 Dec 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. :wink:

avatar ssnobben
ssnobben - comment - 11 Dec 2014

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

avatar wilsonge
wilsonge - comment - 14 Dec 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 :)

avatar Hackwar
Hackwar - comment - 14 Dec 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...

avatar wilsonge wilsonge - close - 30 Dec 2014
avatar wilsonge wilsonge - close - 30 Dec 2014
avatar wilsonge wilsonge - change - 30 Dec 2014
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2014-12-30 02:20:59
avatar wilsonge
wilsonge - comment - 30 Dec 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.

avatar wilsonge wilsonge - test_item - 30 Dec 2014 - Tested successfully
avatar Hackwar Hackwar - head_ref_deleted - 30 Dec 2014

Add a Comment

Login with GitHub to post a comment