Skip to content

Initial codereview of languagefilter plugin#5135

Merged
infograf768 merged 4 commits intojoomla:stagingfrom
Hackwar:languagefilter
Nov 20, 2014
Merged

Initial codereview of languagefilter plugin#5135
infograf768 merged 4 commits intojoomla:stagingfrom
Hackwar:languagefilter

Conversation

@Hackwar
Copy link
Copy Markdown
Member

@Hackwar Hackwar commented Nov 18, 2014

This is an initial code review of the languagefilter plugin. There are several sins in this plugin and I tried to simplify and improve the code where possible.

Testing instructions are simple:

  1. Set up a full fledged multi-lingual installation of Joomla with lots of different content and associations, etc.
  2. Check the current behavior.
  3. Apply the change
  4. See that the behavior did not change 😉

@smanzi
Copy link
Copy Markdown

smanzi commented Nov 18, 2014

@Hackwar So far it seems to work nicely!
I noticed that redirect for logging-in users are now through POST and not GET...
With this too I have spurious 303 (I can show you, if you wish), and I tested that on two different PCs (Win 8.1 and Win 7)... Maybe 'cause my code base is 3.3.6 and not staging?

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Nov 18, 2014

This code review does not include the changes from #5129 yet.

@smanzi
Copy link
Copy Markdown

smanzi commented Nov 18, 2014

OK, got it: your 303 are those that are now 301 in #5129.
With this PR I never get the double redirection

@smanzi
Copy link
Copy Markdown

smanzi commented Nov 18, 2014

Hannes, when you have time, give a look at #5129 (comment) and tell me if it is just a load of bullshit of mine or if there is any sense in it...

@smanzi
Copy link
Copy Markdown

smanzi commented Nov 18, 2014

Yeah, I know... Travis is a pain in the back... 😀

@infograf768
Copy link
Copy Markdown
Member

I combined this with #5129 and I looks like working

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Nov 19, 2014

Added #5129 to this PR.

@infograf768
Copy link
Copy Markdown
Member

OK for me.
@smanzi ?

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Nov 19, 2014

Further changes are proposed in #5140

@smanzi
Copy link
Copy Markdown

smanzi commented Nov 19, 2014

@infograf768 Thanks for asking, appreciated!
I would say that I'm not "against" but I'm not totally "pro" either. After all this modification introduces some inconsistency whose nature and mechanism is not yet clear. I agree that a 301 is more correct, but as I said before I also have concerns about having 301 in such sheer amount. This should be test thoroughly "in the field".

I propose to implement the modification but at the same time have an option "Legacy redirect mode" to revert it in case anything "backfires". It should be plain easy: not even an "if" required, just use the option as the last parameter to $app->redirect()

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Nov 19, 2014

Sorry, but no. We are not going to introduce another parameter for this. This already will make no really measurable difference for Google. If you are concerned about your search engine ranking, then you have lots of other areas where you can work on. I doubt that the redirect type for this redirect makes more than 0.001% difference for Google. It will make more of a difference for Google that Joomla overall will become faster by an improved languagefilter plugin and that itself is only measurable in 2 digit milliseconds.

@smanzi
Copy link
Copy Markdown

smanzi commented Nov 19, 2014

OK, then let's cross fingers and hope it will not backfire in any way.

@smanzi
Copy link
Copy Markdown

smanzi commented Nov 19, 2014

Of course the best solution would be not having redirects at all, but probably this is not achievable with the current architecture.

infograf768 added a commit that referenced this pull request Nov 20, 2014
Initial codereview of languagefilter plugin
@infograf768 infograf768 merged commit 1466854 into joomla:staging Nov 20, 2014
@infograf768
Copy link
Copy Markdown
Member

Merging and closing #5129

@Bakual Bakual added this to the Joomla! 3.4.0 milestone Nov 21, 2014
infograf768 referenced this pull request Dec 2, 2014
Change language string names

Add extra large class to field

Don't check for empty new url if in advanced mode

Remove not null constraint in db

Remove not null constraint in db (2/2)

Add extra check for a 3xx code

Port to all databases

Add missing update files
@Hackwar Hackwar deleted the languagefilter branch December 10, 2014 09:29
@infograf768
Copy link
Copy Markdown
Member

@Hackwar

Please see http://forum.joomla.org/viewtopic.php?f=711&t=882488&p=3292887#p3292887
It seems we may need a "true" added on line 371 (staging)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants