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

Regression of non-ANSI strings handing in 6.2.0 #515

Open
LarnuUK opened this issue Nov 20, 2023 · 3 comments · May be fixed by #516
Open

Regression of non-ANSI strings handing in 6.2.0 #515

LarnuUK opened this issue Nov 20, 2023 · 3 comments · May be fixed by #516

Comments

@LarnuUK
Copy link

LarnuUK commented Nov 20, 2023

The merge from 6.2.0 has made a change to the file class-wpdb.php (which replaced the now deprecated file wp-includes/wp-db.php) which removed several lines, including the following on line 1470:

$query = preg_replace( "/(?<!%)(%($allowed_format)?f)/", '%\\2F', $query ); // Force floats to be locale-unaware.
$query = preg_replace( '/(?<!%)%s/', " N'%s'", $query ); // Quote the strings, avoiding escaped strings like %%s.

The N' at the start ensured that strings were passed as an nvarchar, rather than a varchar, which is vital as the columns in the database from Nami are nvarchar values as well; using varchar literals would result in data loss as characters outside of the code page of the database would appear as ?.

This is still the case in the latest version, and so characters outside the code page (likely ANSI) are lost.

As the line that was there before has been completely removed, I'm unsure if it should have been (and was in error), or if the new line is designed to try to address what the prior lines did:

$query = preg_replace( "/%(?:%|$|(?!($allowed_format)?[sdfFi]))/", '%%\\1', $query );

I did a quick test and adding the old preg_replace line breaks the site, this occurs when placing it both before and after the new line.

@srutzky
Copy link
Contributor

srutzky commented May 30, 2024

@LarnuUK Hey there. Do you have any time / ability to test the change I proposed via #516 ?

@srutzky
Copy link
Contributor

srutzky commented Jun 24, 2024

@patrickebates Hey there. Since I haven't heard back from @LarnuUK , I was wondering if you would be able to help test the change I posted to fix this (i.e. #516 ) in the hopes of being able to merge the fix. Thanks!

@LarnuUK
Copy link
Author

LarnuUK commented Jul 11, 2024

Hey @srutzky sorry I didn't get back to you; I'd simply not had the chance (even though this is a simple fix).

I just gave this a go, and seems to work fine. Dumped a few emoticons and other characters into a draft and they weren't lost in the database.

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 a pull request may close this issue.

2 participants