-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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] Fixing smartsearch issue with some multibytes characters (alternative proposal) #28592
[4.0] Fixing smartsearch issue with some multibytes characters (alternative proposal) #28592
Conversation
This works also. |
…arch-binary-collation-1
Testing InstructionsThanks to @infograf768 for the testing instructions. I've added the update test to make sure I haven't made a mistake in the update sql script. Test 1: New installationStep 1: Patch and make a clean install. Step 2: Create and publish an article which contains
Step 3: Create a smartsearch module in frontend. Step 4: In frontend, search for any Chinese character or group of characters, but specially for the 4 bytes Result: OK and hightlighting is correct. Test 2: UpdateStep 1: Update a 3.10 to 4.0-dev plus the patch of this PR applied, using the update package built for this PR or the corresponding custom update URL. Packages and update URL can be found here: https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/28592/downloads/31062/. Step 2: Repeat steps 2 to 4 of the previous "Test 1: New installation". Result: Same as for "Test 1: New installation". |
I would prefer this over #28587. |
I have tested this item ✅ successfully on 0b20612 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28592. |
I have tested this item ✅ successfully on 0b20612 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28592. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28592. |
@alikon @infograf768 I hope you have also done "Test 2: Update". |
my bad and laziness no.... i'll do sorry |
Update test is important to check that I haven't made a mistake in the update SQL and that the order of processing is correct, i.e. at the end collations are like they should be for the tables and columns handled by this PR. |
…arch-binary-collation-1
@alikon @infograf768 wait with the update test. I've just updated to latest 4.0-dev and so new update package will be built by drone. |
New update package and custom URL have been built by drone. I've updated the link in the testing instructions. @alikon Ready for the update test now. @infograf768 If you have done both tests before, installation and update, please just mark your test result again. Otherwise, if you haven't done the update test: Could you do that test, too? It is important to check there is no mistake in the update sql. |
I have tested this item ✅ successfully on 938edd1 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28592. |
Not sure if related but Joomla 3x table fields such as images in #__content, params in #__modules and #__menu are converting unicode characters to 6-bytes. They appear unicode in the backend but you see multi-byte in db. |
Looks like you are confusing bytes and bits. Unicode UTF8 is max 4 bytes. |
Sorry if it is the case. |
These are JSON encoded and it is not related to the issue here with finder. |
Thanks for the clarification. |
Thanks! |
Pull Request for remaining part of Issue #28493 .
Alternative to PR #28587 .
Summary of Changes
Changing collation of columns
term
,stem
andsoundex
in table#__finder_terms
and columnsterm
andstem
in tables#__finder_tokens
and#__finder_tokens_aggregates
toutf8mb4_bin
.This is the same as done in PR #28587 , except here it is done only for columns mentioned above and not for the complete tables.
Changing table
#__finder_terms_common
so that only columnterm
hasutf8mb4_bin
collation, so it fits to how we do it with other tables havingutf8mb4_bin
collated columns.Testing Instructions
Thanks to @infograf768 for the testing instructions.
Moving testing instructions to a comment below because Drone seems to have problems with certain unicode characters in the description of a PR.
Documentation Changes Required
None.