Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some improvements #14

Closed
wants to merge 3 commits into from
Closed

Some improvements #14

wants to merge 3 commits into from

Conversation

vlad-ghita
Copy link
Contributor

  • after installing from ensemble, if enabled from Extensions list, it will update .htaccess rules (no more manul editing needed).
  • removed redirect mechanism. Continue reading for discussion.

About redirect:

I noticed a performance hit on my site: www.xanderadvertising.com with the previous version of Language Redirect. The redirect requires the Symphony Engine to initialize and execute the Events attached to a Page before the effective redirect takes place. Adding this to some other factors, there was a penalty of at least 2-3 seconds in page load time.

The changes in this commit will remove the redirection mechanism and instead detect the language as follows (if not found at current point, proceed to next point):

  1. On LanguageRedirect initialization, get from $_REQUEST.
  2. try to get from Cookie
  3. try to get from Geolocation Service extension (very useful)
  4. try to get from browser's languages
  5. try english (this is good in my case. Perhaps you would like to remove it)
  6. defaults to first supported language-code.

This will comply very well with Search Engines who will index only www.xanderadvertising.com instead of www.xanderadvertising.com/ro/ or www.xanderadvertising.com/en/.

@klaftertief
Copy link
Collaborator

Hi Vlad,

thanks for improving the extension. I haven't found the time to implement some of the older pull requests and kind of forgot those. I'll have the time to got through my list of extensions in february, but I can add you as an collaborator to the repo right now if you want to.

Regards
Jonas

Am 31.01.2012 um 12:32 schrieb Vlad Ghita:

  • after installing from ensemble, if enabled from Extensions list, it will update .htaccess rules.
  • removed redirect mechanism. Continue reading for discussion.

About redirect:

I noticed a performance hit on my site: www.xanderadvertising.com with the previous version of Language Redirect. The redirect requires the Symphony Engine to initialize and execute the Events attached to a Page before the effective redirect takes place. Adding this to some other factors, there was a penalty of at least 2-3 seconds in page load time.

The changes in this commit will remove the redirection mechanism and instead detect the language as follows (if not found at current point, proceed to next point):

  1. On LanguageRedirect initialization, get from $_REQUEST.
  2. try to get from Cookie
  3. try to get from Geolocation Service extension (very useful)
  4. try to get from browser's languages
  5. try english (this is good in my case. Perhaps you would like to remove it)
  6. defaults to first supported language-code.

This will comply very well with Search Engines who will index only www.xanderadvertising.com instead of www.xanderadvertising.com/ro/ or www.xanderadvertising.com/en/.

You can merge this Pull Request by running:

git pull https://github.com/vlad-ghita/language_redirect master

Or you can view, comment on it, or merge it online at:

#14

-- Commit Summary --

  • Fixed a redirect loop when removing language from preferences will be matched by cookie reference.
  • Update to 1.0.4
  • Language detection w/o redirect.

-- File Changes --

M README.md (9)
M events/event.language_redirect.php (201)
M extension.driver.php (57)
M lib/class.languageredirect.php (125)

-- Patch Links --

https://github.com/klaftertief/language_redirect/pull/14.patch
https://github.com/klaftertief/language_redirect/pull/14.diff


Reply to this email directly or view it on GitHub:
#14

@vlad-ghita
Copy link
Contributor Author

I haven't found the time to implement some of the older pull requests and kind of forgot those.

No prob, just keep up the good work.

I can add you as an collaborator to the repo

This could be an idea, but you should review & agree with the changes I made. This time they're quite radical ;)

@klaftertief
Copy link
Collaborator

I like the changes :-)
But I think they can be even more radical. Maybe some people don't like the URL scheme the extension sets in the .htaccess. Writing to the .htaccess feels and is dirty and the samesimilar can be achieved with the URL router extension. So I think I will remove the .htaccess rules and let people add those manually or use the URL router. (@designermonkey will probably like it. He suggested to deprecate the language redirect because of the redirect and the .htaccess rule.)

What do you think?

@klaftertief
Copy link
Collaborator

Ah, and the extension should be renamed then. There is no redirect anymore. Maybe "Frontend Languages" or similar? But this will break with many existing extensions that require the language redirect.

@designermonkey
Copy link

(@designermonkey will probably like it. He suggested to deprecate the language redirect because of the redirect and the .htaccess rule.)

Heheh.

I wouldn't worry too much about breaking dependencies, S2.3 is going to break a hell of a lot more than that ;o)

I'm sure we could all bang heads together to find a way of ensuring legacy support until those extensions are also updated.

I'll do some reading through this extension a little more and see what I can think of.

@designermonkey
Copy link

You've also reminded me I need to write a tutorial for using the URL rRouter correctly.

@klaftertief
Copy link
Collaborator

I'm currently working on it... Will push something later today.

@klaftertief
Copy link
Collaborator

Well, not on the tTutorial ;-)

@vlad-ghita
Copy link
Contributor Author

Is it safe to just use $_SERVER['REMOTE_ADDR']? The IP seems a bit hardcoded ;-)

Well, this is there for the developers working on localhost. In this case the $_SERVER's variables hold 127.0.0.1, which is pretty useless. Couldn't find a better solution.

Ah, and the extension should be renamed then. There is no redirect anymore. Maybe "Frontend Languages" or similar? But this will break with many existing extensions that require the language redirect.

What about merging current changes and let them live like this for Sym 2.2.x and starting with Symphony 2.3 rename the extension to Frontend Languages or something like that? Doing so, in my Frontend Localisation I'll drop the multiple frontend language driver support and stick only with this new & improved version.

I recommend postponing this until S2.3 because there are lots of changes to be done, especially in all my extensions (they all depend on language info retrieved from Frontend Localisation). With S2.3 I will have to edit them all (there are some breaking changes) and I'll update the Language references as well.

But I think they can be even more radical. Maybe some people don't like the URL scheme the extension sets in the .htaccess. Writing to the .htaccess feels and is dirty and the samesimilar can be achieved with the URL router extension. So I think I will remove the .htaccess rules and let people add those manually or use the URL router. (@designermonkey will probably like it. He suggested to deprecate the language redirect because of the redirect and the .htaccess rule.)

Hmm ... And the first rule in the URL Router rules should be this one?

\/([a-z]{2})-([a-z]{2})(/[a-zA-Z0-9]+/?)?
/$3/?language=$1&region=$2

This feels a bit incomplete. It doesn't allow these cases:

/ro/
/ro-RO/articles/article-with-hyphen

@klaftertief
Copy link
Collaborator

To wait for the renaming for S2.3 sounds good.

I thought about appending example URL Router and .htacces rules to the language settings in the preferences. So developers can update the .htacces or add routes manually. Languages won't be changed that often.

@designermonkey
Copy link

We need to look at those rules, as also there seems to be an issue with query strings in the Router itself. I'm thinking about it at the moment...

I would suggest a rule like:

\/([a-zA-Z]{2})(-([a-zA-Z]{2}))?/(.*)?/\
/$4/?language=$1&region=$3

Also, the plan is to get @nils-werner to finish his pages branch to remove the rules from Preferences to a separate page, so we could look for the existence of this page and append a 'Languages Redirects' section to it? Just an idea...

@vlad-ghita
Copy link
Contributor Author

I thought about appending example URL Router and .htacces rules to the language settings in the preferences. So developers can update the .htacces or add routes manually. Languages won't be changed that often.

This is a very good idea. .htaccess rules should be preserved for anyone who wants them. Here's a use case:

Say you want to localise the Page Titles and Handles. Page LHandles extension deals with this:

_1. .htaccess rules manipulate the URL
_2. PLH translates the URL (FrontendPrePageResolve)
_3. URL Router does it's magic (FrontendPrePageResolve)

If language is determined by URL Router, this will happen:

_1. URL Router determines language (FrontendPrePageResolve)
_2. PLH translates the URL (FrontendPrePageResolve)
_3. URL Router ?fires again? on FrontendPrePageResolve delegate and does it's magic with the localised URL? or ...

Let's bring @designermonkey here. John, your opinion is most welcome in this one.

Edit: Uhh, you're here :)

P.S. Why did you said that Writing to the .htaccess feels and is dirty ?

@klaftertief
Copy link
Collaborator

.htaccess rules should be preserved for anyone who wants them.

Yep. I wouldn't want to drop support for .htaccess rules. I just want to get rid of writing to it. This way, you don't depend on Apache and could use the URL Router (or even just set the language and region parameters [need to be prefixed then] in the extension itself).

@vlad-ghita
Copy link
Contributor Author

Duhh ... Why shouldn't these rules

\/([a-zA-Z]{2})(-([a-zA-Z]{2}))?/(.*)?/\
/$4/?language=$1&region=$3

live in Language Redirect directly? + some logic change here and voila!

This way, FrontendPrePageResolve from Language Redirect should fire first, LR does it's logic (determines language, region, cleans the URL from them) and serves the new URL for further processing.

EDIT:

Here's the fixed rule. With this we can extract the language and solve it:

\/([a-z]{2})(-[A-Z]{2})?/(.*/)?\

new url is /$3 ?

AFAIK: standard is language = lower case; region = upper case

@klaftertief
Copy link
Collaborator

I don't want to hardcode the regexes anywhere anymor I think. So the developer has the control about the URL scheme again.

@designermonkey
Copy link

Talking of URL schemes, I've been reading about that today...

It's best practice to put the language code at the beginning of the URL like you have been doing. I reckon we should at least still advocate that behaviour, even if it's not hardcoded.

@klaftertief
Copy link
Collaborator

Agree. This is where the example rules will come in handy.

@vlad-ghita
Copy link
Contributor Author

Symphony 2.3 is around the corner. How are we dealing with this?


1. Take note that as @designermonkey mentioned before

It's best practice to put the language code at the beginning of the URL like you have been doing.

we should help the frontend developer in setting up the links. The recommended way to setup a multilingual site is (using gTLDS):

www.site.com - main language (eg: en). Defaults to the main/reference language
www.site.com/fr - french
www.site.com/de - deutsch
....

2. Regarding .htaccess rules, @klaftertief said

I don't want to hardcode the regexes anywhere anymor I think. So the developer has the control about the URL scheme again.

@see my comment above. The rules should stay in my opinion.

Let this extension use .htacces rules and URLs as stated above. If someone has an outstanding requirement for another way of language identification (eg www.site.com/?lang=XX), then writing an extension for that particular case is trivial and very easy to integrate with Frontend Localisation.


3. What this extension should do: - offer a way to input desired frontend languages (exists) - select a reference language (Frontend Localisation does this atm = not a good thing) - I think this extension could properly be renamed to `frontend_languages`
I can deal with implementing this stuff. What do you think?

@vlad-ghita
Copy link
Contributor Author

Update: Forget about this post. Read next one.

Here's Frontend Languages. It respects the bullets from point 3 in previous post.<

I'll integrate it with Frontend Localisation and proceed. Until Language Redirect is updated to Symphony 2.3, I'll remove it from supported Language drivers in Frontend Localisation.

@vlad-ghita vlad-ghita closed this May 7, 2012
@vlad-ghita
Copy link
Contributor Author

Last night a new idea struck me and here's the result:

Frontend Localisation deals with Frontend language management (adding language codes in Backend and selecting main language of the site).

The frontend language information is sent by third-party extensions. It must receive 2 GET parameters: fl-language and fl-region: www.site.com/?fl-language=XX&fl-region=XX.

One extension the implements the routing logic is FLang detection gTLDs (I just created it). It implements slighlty modified .htaccess rules of language redirect. This way, the Frontend Language information sits in one place (Frontend Localisation) and everybody is free to chose a way to implement the URL structure as long as behind the scenes the GET params are received for processing.

What do you think?

@klaftertief
Copy link
Collaborator

Thanks again for thinking and working hard on this one. And sorry again for slow answers.

I don't have a strong opinion at the moment. Maybe it really is time to hand the extension over to you, because you are building extensions on top of it. I only used the old language field and the multilingual field with it.

I'll have some thinking and extension-updating time at the weekend. So you can expect a mor detailed answer then.

@vlad-ghita
Copy link
Contributor Author

And sorry again for slow answers.

No prob. I was expressing the ideas loud because these decisions will impact the future really hard and I wanted at least someone else's opinion in this matter.

Maybe it really is time to hand the extension over to you, because you are building extensions on top of it.

Well, I'm building for some time now and that's why I needed some solid ground => Frontend Localisation :) In current state of things, there is no need of Language Redirect any longer since the needed functionality (language detection and language management) are handled separately in dedicated places.

Here's how I see it atm. Can you drop your 2 cents please?

@klaftertief
Copy link
Collaborator

I couldn't install the extension to have a look at them.

I like the idea that the Frontend Localisation doesn't care where the language information comes from. It gives you the freedom to set the parameters the way you like, e.g. via .htaccess-rules or the URL Router extension. I'm not sure if the language information should only be passed via URL parameters. Maybe we should define other ways as well, like normal page parameters or event parameters.

What I generally don't like are your sometimes cryptic naming conventions :-) But maybe that's just me.

What I didn't like in the Language Redirect changes in this pull request is the following:

  • The defined languages are en, de.
  • You visit the frontend and set a secondary language, e.g. by openening an URL like /de/page/param/.
  • You want to change to the standard language en, which should be possible by ommiting the language parameter.
  • You visit /page/param/, but the language is still de and not en, because it's saved in the cookie.

The automatic redirect took care of this. But I think that's just a question of the implementation and not a desgn choice. Is it?

(Maybe we should discuss further somewhere else. Though the multilingual stuff is not my main building lot at the moment.)

@vlad-ghita
Copy link
Contributor Author

I'm not sure if the language information should only be passed via URL parameters. Maybe we should define other ways as well, like normal page parameters or event parameters.

Indeed. Frontend Localisation should provide these ways, but I'll implement them when are needed. ATM, I didn't see any request in multilingual related threads for something specific (except the brief ideas from this discussion = URL Router)

What I generally don't like are your sometimes cryptic naming conventions :-) But maybe that's just me.

Yeah, well, it's a battle between conciseness and explicitness :) It seems I'm changing sides on-the-fly.

What I didn't like in the Language Redirect changes in this pull request is the following:

Well, this pull request is ancient history at this moment since the main ideas from it have been split between (Frontend Localisation) & (FLang detection gTLDs).

The important thing now is you to decide what to do with Language Redirect, if there is still need for it. The language management and a language detection mechanism are taken care of in other places. Language redirection must remain here.

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.

None yet

3 participants