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

[4.0] Finder: Replacing iconv() with mb_convert_encoding() #22042

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Sep 7, 2018

The original issue is this here: https://forum.joomla.org/viewtopic.php?t=860554#p3503601
Looking through the documentation, iconv() indeed has issues: https://secure.php.net/manual/en/function.iconv.php

The more reliable solution as it seems is this code.

Testing Instructions

Apply this patch and run the indexer of Smart Search in the backend. See that it still works.

@csthomas
Copy link
Contributor

csthomas commented Sep 7, 2018

Similar solution is at:

protected function remove($source)
{
// Check for invalid UTF-8 byte sequence
if (!preg_match('//u', $source))
{
// String contains invalid byte sequence, remove it
$source = htmlspecialchars_decode(htmlspecialchars($source, ENT_IGNORE, 'UTF-8'));
}
// Iteration provides nested tag protection
do
{
$temp = $source;
$source = $this->_cleanTags($source);
}
while ($temp !== $source);
return $source;
}

@laoneo
Copy link
Member

laoneo commented Sep 10, 2018

The PHP mb extension is not always installed on hosts. So either way I would add a check or fallback to iconv if it is not available.

@mbabker
Copy link
Contributor

mbabker commented Sep 10, 2018

symfony/polyfill-mbstring covers this if the extension isn't installed and it is already a transient dependency of 4.0 so this is fine as is.

@laoneo
Copy link
Member

laoneo commented Sep 11, 2018

ok

@laoneo laoneo added this to the Joomla 4.0 milestone Sep 19, 2018
@laoneo laoneo merged commit 5fef532 into joomla:4.0-dev Sep 19, 2018
@laoneo
Copy link
Member

laoneo commented Sep 19, 2018

Thanks

@Hackwar Hackwar deleted the patch-31 branch April 27, 2019 08:09
Comment on lines +36 to +37
$oldSetting = ini_get('mbstring.substitute_character');
ini_set('mbstring.substitute_character', 'none');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ini_set returns the old value so this could be one operation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm ok commenting on an 5 year old pr makes no sense... sry for the noise

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

6 participants