Initial codereview of languagefilter plugin #5135

Merged
merged 4 commits into from Nov 20, 2014

Conversation

Projects
None yet
5 participants
@Hackwar
Member

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

This comment has been minimized.

Show comment
Hide comment
@smanzi

smanzi 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?

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

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 18, 2014

Member

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

Member

Hackwar commented Nov 18, 2014

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

@smanzi

This comment has been minimized.

Show comment
Hide comment
@smanzi

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

This comment has been minimized.

Show comment
Hide comment
@smanzi

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

This comment has been minimized.

Show comment
Hide comment
@smanzi

smanzi Nov 18, 2014

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

smanzi commented Nov 18, 2014

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

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 19, 2014

Member

I combined this with #5129 and I looks like working

Member

infograf768 commented Nov 19, 2014

I combined this with #5129 and I looks like working

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 19, 2014

Member

Added #5129 to this PR.

Member

Hackwar commented Nov 19, 2014

Added #5129 to this PR.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 19, 2014

Member

OK for me.
@smanzi ?

Member

infograf768 commented Nov 19, 2014

OK for me.
@smanzi ?

@Hackwar

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 19, 2014

Member

Further changes are proposed in #5140

Member

Hackwar commented Nov 19, 2014

Further changes are proposed in #5140

@smanzi

This comment has been minimized.

Show comment
Hide comment
@smanzi

smanzi 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()

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

This comment has been minimized.

Show comment
Hide comment
@Hackwar

Hackwar Nov 19, 2014

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@smanzi

smanzi Nov 19, 2014

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

smanzi commented Nov 19, 2014

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

@smanzi

This comment has been minimized.

Show comment
Hide comment
@smanzi

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

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

Merge pull request #5135 from Hackwar/languagefilter
Initial codereview of languagefilter plugin

@infograf768 infograf768 merged commit 1466854 into joomla:staging Nov 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 20, 2014

Member

Merging and closing #5129

Member

infograf768 commented Nov 20, 2014

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

[imp] Add ability to set redirect headers in com_redirect. Fixes #4292
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 Hackwar:languagefilter branch Dec 10, 2014

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Apr 18, 2015

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)

Member

infograf768 commented Apr 18, 2015

@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