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

Fixes for a regression introduced by #4221 #4502

Merged
merged 1 commit into from Oct 10, 2014
Merged

Fixes for a regression introduced by #4221 #4502

merged 1 commit into from Oct 10, 2014

Conversation

smanzi
Copy link

@smanzi smanzi commented Oct 10, 2014

#4221 was incorrectly using str_replace() to remove the default language code when required. As str_replace() acts globally on the subject, the language code was removed also out of context (e.g. when part of an alias).

With this PR I use instead preg_replace() with the limit of one substitution. As the /language-code/ string to be replaced appears between two slashes and just after the host name (eg. http://example.com/en/) this should guarantee the correct replacements of the language code only.

I took the occasion to make a bit of clean-up by aggregating some redundantly duplicated code

#4221 was incorrectly using str_replace() to remove the default language
code when needed. As str_replace acts globally on the subject, the
language code was removed also out of context (e.g. when part of an
alias).

With this PR I use instead preg_replace with the limit of one
substitution. due to the fact that the /language-code string to be
replaced appears just after the host name and cannot be part of it (due
to the/), this should guarantee the correct replacements of the language
code only.

I took the occasion to make a bit of clean-up by aggregating some
redundantly duplicated code
@infograf768
Copy link
Member

@test
Fine here.

@smanzi
Copy link
Author

smanzi commented Oct 10, 2014

@test
Good here too, on a production server
Does my word count?

@smanzi
Copy link
Author

smanzi commented Oct 10, 2014

@infograf768

I just noticed my local copy of languagefilter.php is CR-LF terminated (à-la-windows...)

Is this a problem? Can be fixed?

@infograf768
Copy link
Member

It is Unix in core

infograf768 added a commit that referenced this pull request Oct 10, 2014
Fixes for a regression introduced by #4221
@infograf768 infograf768 merged commit 89675ff into joomla:staging Oct 10, 2014
@infograf768
Copy link
Member

Thanks for the fix. Merged.
please test #4425 for 2.5.x

@smanzi smanzi deleted the fix-#4421 branch October 10, 2014 18:43
@smanzi
Copy link
Author

smanzi commented Oct 10, 2014

You're welcome!
Tested #4425 with success.

@mbabker mbabker added this to the Joomla! 3.3.7 milestone Oct 11, 2014
@mbabker mbabker modified the milestones: Joomla! 3.3.7, Joomla! 3.4.0 Nov 22, 2014
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

5 participants