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

[Deprecate _QQ_ 2] Save language overrides with escaped double quotes #13427

Merged
merged 3 commits into from Jan 4, 2017

Conversation

andrepereiradasilva
Copy link
Contributor

@andrepereiradasilva andrepereiradasilva commented Dec 30, 2016

Summary of Changes

As discussed in #13421
This PR changes language overrides save to ini method so we use escaped double quotes (\") instead of "_QQ_".
Also adds a new method to save the language strings to a ini file so it can be easily used when needed.

Testing Instructions

  • Apply patch
  • Create a new language override from Extensions -> Languages -> Overrides for the en-GB admin language with some double quotes on it ("). Save it, load it again and confirm if ok.
  • Confirm it's saved as escaped double quote in /administrator/language/overrides/en-GB.override.ini. Example: A test with <a href=\"https://joomla.org\" target=\"_blank\">Joomla</a>
  • Manually add an override in /administrator/language/overrides/en-GB.override.ini with _QQ_ and escaped quotes. Example:
JLIB_DATABASE_ERROR_DATABASE_UPGRADE_FAILED="A second test with <a href="_QQ_"https://joomla.org"_QQ_" target=\"_blank\">Joomla</a>"
  • Confirm language override works
    Ex: add to template index.php
// Server side message system
$app->enqueueMessage(JText::_('JLIB_DATABASE_ERROR_DATABASE_UPGRADE_FAILED'), 'warning');

// Client side message system (click on the page to show the javascript message)
JText::script('JLIB_DATABASE_ERROR_DATABASE_UPGRADE_FAILED');
$this->addScriptDeclaration("document.addEventListener('click', function () { Joomla.renderMessages({'error': [Joomla.JText._('JLIB_DATABASE_ERROR_DATABASE_UPGRADE_FAILED')]}) });");

Documentation Changes Required

None.

@ralain
Copy link
Contributor

ralain commented Dec 30, 2016

I have tested this item ✅ successfully on 56d1bc0

Both "_QQ_" and \" are being translated as expected and quotes in overrides save as \".


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

@frankmayer
Copy link
Contributor

I have tested this item ✅ successfully on 56d1bc0

Code review OK


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

@jeckodevelopment
Copy link
Member

RTC


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

@infograf768
Copy link
Member

infograf768 commented Dec 31, 2016

warning concerning strings that would be used through js. see my comment in the specific pr.
let's make sure that one is fully solved all over core before merging this
#13425

@infograf768
Copy link
Member

In any case, can you explain the reason for using addcslashes instead of addslashes ?

@andrepereiradasilva
Copy link
Contributor Author

warning concerning strings that would be used through js. see my comment in the specific pr.

Is not related to this PR.

In any case, can you explain the reason for using addcslashes instead of addslashes ?

because the only thing to escape are the double quotes ("), ie, the ini string delimiters, the rest AFAIK is allowed in the ini file.

@infograf768
Copy link
Member

@rdeutz
Please merge.
Indeed we have to solve the js strings issue in the 2 existing PRs.

@rdeutz rdeutz added this to the Joomla 3.7.0 milestone Jan 4, 2017
@rdeutz rdeutz merged commit d189cb1 into joomla:staging Jan 4, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 4, 2017
@andrepereiradasilva andrepereiradasilva deleted the no-qq-override branch January 4, 2017 08:01
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

7 participants