Mssql - fix escape() and add support for unicode chars by csthomas · Pull Request #13585 · joomla/joomla-cms

csthomas · 2017-01-13T18:03:12Z

Pull Request for better version of #13534

Summary of Changes

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291
Repair $db->escape()
stripslashes() added in Add stripslashes to mssql result columns #13534 is now not needed
In $db->quote() add prefix N, means instead column = 'text' there will be column = N'text' which add support for unicode, international chars (ex. Polish chars ąęćółńżź)

Testing Instructions

Install fresh joomla staging, (if you have installed joomla with some modification in database then JSON Decode error may occur).
Edit or add some article:

change Meta Description field to (add some national chars like ąęśżźół):

xęą
\
\
x

change Title to "TEST 1 - ale[x]"

Save and check result.
You should see that Meta Description field is not saved properly (new line has been removed, national chars disapears).
Edit template file at templates/protostar/index.php
Add php code at the bottom of the file (before </body>):

<?php
$db = JFactory::getDbo();
$query = $db->getQuery(true)
	->select('title')
	->from('#__content')
	->where('title LIKE ' . $db->quote('%' . $db->escape('ale[x]', true) . '%', false));

$title = $db->setQuery($query)->loadResult() ?: 'NOT FOUND';
echo '<p>Result 1: "' . $title . '"</p>';

$query = $db->getQuery(true)
	->select('COUNT(*)')
	->from('#__content')
	->where('title LIKE ' . $db->quote('%' . $db->escape('%%%', true) . '%', false));

echo '<p>Result 2: "' . $db->setQuery($query)->loadResult() . '"</p>';
?>

Go to front page and check results:

Result 1: "NOT FOUND"
Result 2: "67"
or similar (number bigger than 0) - sample testing data installed

Apply current PR by PatchTester.
Repeat point 2,3,4. Now Meta Description is saved OK.
Set Content Rights field (json column) to:

Author\"\
rights.

Note: If you added above text before applying patch you get Json error on joomla with applied patch. Before adding patch please reset that field.
11. Save and check whether content rights is correct.
12. Go to front page and check results:

Result 1: "TEST 1 - ale[x]"
Result 2: "0"

Explanation for tests:

%%% - there is no article with text '%%%' and should not found any articles. If found then escape does not work if correct way.
ale[x] - should find one article because there is one title with these chars. But before patch it treats brackets as a special chars and try to find only "alex".
backslash at the end of line test fix for https://support.microsoft.com/en-us/kb/164291
Content Rights - is stored, encoded to json. This test check whether json which contains backslashes or quotes works correctly.

Documentation Changes Required

N/A

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291 joomla/joomla-cms#13585

waader · 2017-01-27T16:52:15Z

I have different results for step 7 and step 12:

Step 7:
Result 1: "TEST 1 - ale[x]"
"67"

Step12:
Result 1: "NOT FOUND"
Result 2: "0"

The rest works as described.

csthomas · 2017-01-27T16:54:54Z

This is OK, you have in total 67 articles, I had 75 articles.
sql LIKE '%%%' found al your articles.

csthomas · 2017-01-27T16:55:42Z

wait a moment

csthomas · 2017-01-27T16:57:17Z

You talk about Result 2: "67" where I got 75? Right?

waader · 2017-01-27T17:00:21Z

No, I talk about Result 1. My results a different then yours.

csthomas · 2017-01-27T17:03:26Z

Last time I added unicode support, it may change that. I'm checking it.

csthomas · 2017-01-27T17:17:46Z

Do you have title of article "TEST 1 - ale[x]" <- brackets are important to test it.
Result as you get - I think that you may forget add brackets in article title. If not I will search more.

waader · 2017-01-27T17:46:55Z

I do have the brackets in the title.

csthomas · 2017-01-27T18:08:00Z

which database do you use: local SQL Server or Azure? If SQL server then what version?

waader · 2017-01-27T20:08:30Z

SQL Server 2008 R2

See https://support.microsoft.com/en-us/kb/164291

csthomas · 2017-01-27T21:09:08Z

I have rebased PR and improved a test instruction.
But there is no changes that could return your result.

I based on https://msdn.microsoft.com/library/ms179859.aspx where [ has to be escaped like [[].
The same is for % and _.

I will wait for next test. May it resolve something.

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291 joomla/joomla-cms#13585

photodude · 2017-01-29T17:58:24Z

It would be a good idea to rebase this now that AppVeyor is CI testing our unit test suite with MSSQL

photodude · 2017-01-29T18:12:27Z

@csthomas can you comment on the unit test failure on the Framework version?
https://ci.appveyor.com/project/photodude/database/build/1.0.67/job/l64qi7ws1j9ghi8b#L898
Do we just need to change the unit test to have brackets around the % char? % vs [%]

csthomas · 2017-01-29T21:14:42Z

For mssql LIKE clause you have to escape 3 chars: _,% and [ because it is a default escape char.
% -> [%]
_ -> [_]
[ -> [[]

csthomas · 2017-01-29T21:36:53Z

Now AppVeyor is happy:)

photodude · 2017-01-30T16:42:23Z

libraries/joomla/database/driver/sqlsrv.php

-			$result = str_replace('_', '[_]', $result);
+			// Escape special chars
+			$result = str_replace(
+				array('[',   '_',   '%'),


do we also need ] to be escaped?

array('[', ']', '_', '%'), array('[[]', '[]]''[_]', '[%]'),

No.
According to doc https://msdn.microsoft.com/library/ms179859.aspx#Anchor_7
and my own tests.

Thanks for the clarification

photodude

Looks good on code review

csthomas · 2017-01-31T22:18:08Z

@alikon You can test and confirm or not waader results?

waader · 2017-02-01T12:34:33Z

I have tested this item ✅ successfully on 8d12824

I retested your patch with a new checkout and now I get your results. One thing that´s obviously different to your environment is, that I get a 404 component not found error in frontend with no testing data installed. So I am curious: what database version are you using?

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13585.}

csthomas · 2017-02-01T12:53:30Z

Not installed sample data probably is a reason
but IIRC every article without set up publish_down is treats on mssql as expired (datetime type should be change to datetime2 - for now joomla is waiting for that)

My system is linux with installed mssql on it.

waader · 2017-02-01T13:08:24Z

As I have no sample data installed the difference most likely is the runtime environment. I will dig into it when I have more time.

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291 joomla/joomla-cms#13585

wilsonge · 2017-02-05T19:40:56Z

Given this only affects SQLServer I'm going to merge this with the code review and the one good test

csthomas · 2017-02-05T19:42:25Z

Thanks now I can go forward and complete next one #13262 as I mentioned.

joomla-cms-bot added the PR-staging label Jan 13, 2017

csthomas force-pushed the fixmssql3 branch 2 times, most recently from 9ab3048 to b35f3ce Compare January 13, 2017 22:53

csthomas mentioned this pull request Jan 14, 2017

Add stripslashes to mssql result columns #13534

Merged

csthomas force-pushed the fixmssql3 branch from b35f3ce to fb2d842 Compare January 14, 2017 15:18

photodude added a commit to photodude/database that referenced this pull request Jan 15, 2017

fix [Mssql] escape() - stripslashes for column is not needed

687f43e

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291 joomla/joomla-cms#13585

photodude mentioned this pull request Jan 15, 2017

fix [Mssql] escape() - stripslashes for column is not needed joomla-framework/database#68

Merged

photodude added a commit to photodude/database that referenced this pull request Jan 17, 2017

fix [Mssql] escape() - stripslashes for column is not needed

8e096ad

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291 joomla/joomla-cms#13585

csthomas force-pushed the fixmssql3 branch from fb2d842 to af80a2c Compare January 26, 2017 11:16

csthomas changed the title ~~Mssql - fix escape() - stripslashes for column is not needed~~ Mssql - fix escape() and add support for unicode chars Jan 26, 2017

csthomas mentioned this pull request Jan 27, 2017

Can not install joomla on MSSQL - remove semicolon before UNION ALL #13767

Merged

Mssql - save in correct way '\<CR><LF>', '\<LF>' and fix $db->escape()

d1a4fdb

See https://support.microsoft.com/en-us/kb/164291

csthomas force-pushed the fixmssql3 branch from af80a2c to 1913049 Compare January 27, 2017 20:31

photodude added a commit to photodude/database that referenced this pull request Jan 29, 2017

fix [Mssql] escape() - stripslashes for column is not needed

33509c8

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291 joomla/joomla-cms#13585

Add support for unicode/nchar characters

7073792

csthomas force-pushed the fixmssql3 branch from 1913049 to 7073792 Compare January 29, 2017 21:17

Fix tests

8d12824

joomla-cms-bot added the Unit/System Tests label Jan 29, 2017

photodude reviewed Jan 30, 2017

View reviewed changes

photodude approved these changes Jan 30, 2017

View reviewed changes

photodude added a commit to photodude/database that referenced this pull request Feb 3, 2017

fix [Mssql] escape() - stripslashes for column is not needed

04d78f9

Fix escape sequences - see https://support.microsoft.com/en-us/kb/164291 joomla/joomla-cms#13585

csthomas mentioned this pull request Feb 5, 2017

Advanced MSSQL query workaround for limit, offset and for sql with aggregate functions #13262

Merged

wilsonge merged commit f1221bb into joomla:staging Feb 5, 2017

wilsonge added this to the Joomla 3.7.0 milestone Feb 5, 2017

csthomas deleted the fixmssql3 branch February 5, 2017 19:43

This was referenced Feb 28, 2017

Typo in comment #14276

Closed

typo in comments #14277

Merged

Uh oh!

Conversation

csthomas commented Jan 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Explanation for tests:

Documentation Changes Required

Uh oh!

waader commented Jan 27, 2017

Uh oh!

csthomas commented Jan 27, 2017

Uh oh!

csthomas commented Jan 27, 2017

Uh oh!

csthomas commented Jan 27, 2017

Uh oh!

waader commented Jan 27, 2017

Uh oh!

csthomas commented Jan 27, 2017

Uh oh!

csthomas commented Jan 27, 2017

Uh oh!

waader commented Jan 27, 2017

Uh oh!

csthomas commented Jan 27, 2017

Uh oh!

waader commented Jan 27, 2017

Uh oh!

csthomas commented Jan 27, 2017

Uh oh!

photodude commented Jan 29, 2017

Uh oh!

photodude commented Jan 29, 2017

Uh oh!

csthomas commented Jan 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csthomas commented Jan 29, 2017

Uh oh!

photodude Jan 30, 2017

Choose a reason for hiding this comment

Uh oh!

csthomas Jan 30, 2017

Choose a reason for hiding this comment

Uh oh!

photodude Jan 30, 2017

Choose a reason for hiding this comment

Uh oh!

photodude left a comment

Choose a reason for hiding this comment

Uh oh!

csthomas commented Jan 31, 2017

Uh oh!

waader commented Feb 1, 2017

Uh oh!

csthomas commented Feb 1, 2017

Uh oh!

waader commented Feb 1, 2017

Uh oh!

wilsonge commented Feb 5, 2017

Uh oh!

csthomas commented Feb 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

csthomas commented Jan 13, 2017 •

edited

Loading

csthomas commented Jan 29, 2017 •

edited

Loading