Skip to content

Add stripslashes to mssql result columns#13534

Merged
rdeutz merged 1 commit intojoomla:stagingfrom
csthomas:fixmssql2
Jan 11, 2017
Merged

Add stripslashes to mssql result columns#13534
rdeutz merged 1 commit intojoomla:stagingfrom
csthomas:fixmssql2

Conversation

@csthomas
Copy link
Copy Markdown
Contributor

@csthomas csthomas commented Jan 9, 2017 •
edited by jeckodevelopment
Loading

Pull Request for Issue #13507 .

Summary of Changes

SQL Server database always escape backslash in result.
This PR stripslashes for each column from result query.

Testing Instructions

TEST 1

Before patch

  1. Go to article, change some text or nothing, save article a few times.
  2. Go to "Versions" (button at the top), there should open a modal window with list of dates and versions.
  3. Click on some date row which open popup window.
  4. You should see warnings and no data information about changes between versions.
Warning: Invalid argument supplied for foreach() in .../administrator/components/com_contenthistory/helpers/contenthistory.php on line 32

Warning: Invalid argument supplied for foreach() in .../administrator/components/com_contenthistory/helpers/contenthistory.php on line 295

After patch the warning does not appears and you see some information.

TEST 2

Before patch

  1. Edit new article, and write two chars in full text: \"
  2. Re-save a few times without changing anything more.
  3. You should see more and more backslashes, ignore html tags in ex. \\", next \\\\", etc.

After patch the number of backslashes are not increased on each save.

TEST 3

Before patch:

  1. Edit article and add to Link A Text (Images and Links Tab): A""B.
  2. After you save article you get Error decoding JSON data: Syntax error.

After patch

  1. Do the same and save and load article works as expected.

Documentation Changes Required

N/A

@RonakParmar
Copy link
Copy Markdown

RonakParmar commented Jan 10, 2017

Missing testing instruction.

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@zero-24 zero-24 mentioned this pull request Jan 10, 2017
Closed
@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Jan 10, 2017

Test instruction added.

There is a lots of problems with mssql. This PR only fix a few of them.
There is a lots of places where something does not work this not means this PR is buggy.

"This PR fix only SELECT queries".

@waader
Copy link
Copy Markdown
Contributor

waader commented Jan 10, 2017

I have tested this item ✅ successfully on f57b5ff

Thanks!

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@alikon
Copy link
Copy Markdown
Contributor

alikon commented Jan 11, 2017

I have tested this item ✅ successfully on f57b5ff

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@jeckodevelopment
Copy link
Copy Markdown
Member

jeckodevelopment commented Jan 11, 2017

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13534.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 11, 2017
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Jan 11, 2017
@rdeutz
Copy link
Copy Markdown
Contributor

rdeutz commented Jan 11, 2017

Just one question, you removed loadResult from the class because the JDatabaseDriver class does the job ok?

@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Jan 11, 2017

yes

@rdeutz rdeutz merged commit e3d8732 into joomla:staging Jan 11, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 11, 2017
@csthomas csthomas deleted the fixmssql2 branch January 11, 2017 19:45
@dgrammatiko
Copy link
Copy Markdown
Contributor

dgrammatiko commented Jan 11, 2017

@rdeutz isn't the removal of a public function a b/c break?

@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Jan 11, 2017

It is not removed because it is inherited from JDatabaseDriver

@dgrammatiko
Copy link
Copy Markdown
Contributor

dgrammatiko commented Jan 11, 2017

@csthomas in such case ignore my comment

andrepereiradasilva pushed a commit to andrepereiradasilva/joomla-cms that referenced this pull request Jan 12, 2017
@csthomas @andrepereiradasilva
Add stripslashes to mssql result columns. (joomla#13534)
725755d
roland-d added a commit to roland-d/joomla-cms that referenced this pull request Jan 13, 2017
@roland-d
Merge branch 'staging' of github.com:roland-d/joomla-cms into trackas…
aa225cb 
…sets-option

* 'staging' of github.com:roland-d/joomla-cms: (136 commits)
  Clean up old code in cache.php file (joomla#12183)
  Fixing search for MySQL (joomla#13571)
  Unnecessary double quotes in  /libraries/joomla (joomla#13372)
  Some improvements in tests #3: (joomla#13402)
  It's 2017. Happy New Year
  Fixing a typo in gallery plugin language files
  Add stripslashes to mssql result columns. (joomla#13534)
  remove unneeded space from btn-group/radio/checkboxes (joomla#12003)
  typo (joomla#13563)
  Remove default value from the field params to inherit from plugin
  Remove multiple parameter from user field
  Fix name of component helper in fieldshelper (joomla#13539)
  remove duplicated code (joomla#13550)
  Fix invalid string that causes the ini file not to load (joomla#13544)
  Catch "expects parameter 2 to be string" error
  Take complete context for group lookup (joomla#13538)
  [Mssql] Fix syntax error when installing a language in backend (joomla#13512)
  Normalize #__categories table across 3 db systems and add default values (joomla#13514)
  Normalize #__ucm_content table across 3 db systems and add default values (joomla#13513)
  Update config.xml (joomla#13503)
  ...
@csthomas csthomas mentioned this pull request Jan 13, 2017
Merged
@csthomas
Copy link
Copy Markdown
Contributor Author

csthomas commented Jan 14, 2017 •
edited
Loading

After more deep searching I have found this PR fixes errors but does not do it in the right way.
It was weird to me that I have to use stripslashes() but it worked.

At the end I have found that $db->quote() does not work correctly and generate additional backslashes that are saved to database (specially to columns with encoded json). After that stripslashes() is needed.

After I changed $db->quote() code then stripslashes() is not needed.
Take a look at PR #13585

@csthomas csthomas mentioned this pull request Jan 16, 2017
Merged
rdeutz pushed a commit to rdeutz/joomla-cms that referenced this pull request Jan 17, 2017
@csthomas @rdeutz
Add stripslashes to mssql result columns. (joomla#13534)
eae7293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla 3.7.0

Development

Successfully merging this pull request may close these issues.

8 participants

@csthomas @RonakParmar @waader @alikon @jeckodevelopment @rdeutz @dgrammatiko @joomla-cms-bot