Fix Language parsing when parse_ini_file disabled #15620

PhilETaylor · 2017-04-27T11:09:36Z

Use sensible code to process ini files when moronic web hosts disable parse_ini_file in php.ini

Please test

closes #15587 Use sensible code available in FOF to process ini files when moronic web hosts disable parse_ini_file in php.ini Please test

dgrammatiko · 2017-04-27T11:13:57Z

@PhilETaylor shouldn't this be conditional, if php_ini_parse not exist then use FOF?

dgrammatiko · 2017-04-27T11:19:41Z

Still, this will be faster (one less file, less memory for the good hosts):

		if (!function_exists('parse_ini_file'))
		{
         		$strings = FOFUtilsIniParser::parse_ini_file($filename, true);
		}
		else
		{
			$strings = @parse_ini_file($filename);
		}

zero-24 · 2017-04-27T11:55:11Z

Should we really create another dependencey to FOF where we try to remove it at other places?

csthomas · 2017-04-27T12:09:14Z

libraries/joomla/language/language.php

-		$strings = @parse_ini_file($filename);
+		if (!function_exists('parse_ini_file'))
+		{
+			return FOFUtilsIniParser::parse_ini_file_php($filename, true);


Why we can not use the old way before #13407?

return @parse_ini_string(file_get_contents($filename));

As long as we restore:

$contents = file_get_contents($filename); $contents = str_replace('_QQ_', '"\""', $contents); $strings = @parse_ini_string($contents);

That should do the trick.

In case the new function is not there.

IMO the idea was improve performance. Please replace above line as I suggested.

No just as fallback when the new code don't work for b/c? (as respose to @PhilETaylor)

This $contents = str_replace('_QQ_', '"\""', $contents); is not needed. I tested on php7.0

Obviously just using parse_ini_file is simpler than using parse_ini_string and file_get_contents. That's probably the main reason it has been changed.
And as you can read in that PR, there is some automatic handling of QQ

Bakual · 2017-04-27T12:12:32Z

Using FOF certainly isn't right.

zero-24 · 2017-04-27T12:25:31Z

@PhilETaylor

remove QQ replacement (apparently its not needed)

why do you think so?

parse_ini_file can do that: http://php.net/manual/en/function.parse-ini-file.php

Constants may also be parsed in the ini file so if you define a constant as an ini value before running parse_ini_file(), it will be integrated into the results.

But i can not find something like that for parse_ini_string: http://php.net/manual/en/function.parse-ini-string.php

I have not tested this yet but maybe you have..

zero-24 · 2017-04-27T12:28:53Z

As i have said i have not checked it yet and i have not saw that comment above. Needs to be tested.

Bakual · 2017-04-27T12:29:03Z

Honestly, just revert #13407.

zero-24 · 2017-04-27T12:30:39Z

@Bakual i think the rest (array + renaming a var) can stay from that PR? Just that str_replace needs to be checked i guess.

brianteeman · 2017-04-27T12:49:48Z

You absolutely can not remove the qq processing!!

mbabker · 2017-04-27T12:57:09Z

Do not revert #13407

The only thing you need to do is basically if parse_ini_file is usable then use it otherwise use our old way which supports broken INI files

We need to stop allowing broken crap to work in our core APIs
The simplest option is to use the simplest function if available, since apparently hosts are 3 steps past moronic we can have a sensible fallback (i.e. the old code which has been proven to work with broken files)
The fact that this project is still so reluctant to accept valid third party solutions is really getting out of hand. We don't have the developers to write everything ourselves because we keep chasing everyone with half an ounce of skill away.

infograf768 · 2017-04-27T14:46:48Z

For testers:
add
disable_functions = parse_ini_file
in your php.ini file
Stop and restart server.

csthomas · 2017-04-27T15:10:12Z

Constants for parse_ini_string() are supported in php version 5.3.2.
I have checked it at https://3v4l.org/#preview

Constants are not supported in hhvm 3.19.1 and lower at all for both (parse_ini_file, parse_ini_string)

I tested with code:

echo phpversion() . "\n";
define('__QQ__', " WORKS ");
print_r(parse_ini_string('A="IT"__QQ__"GREATE!"'));
print_r(parse_ini_file(__DIR__ . '/test.ini'));

For hhvm I got:

5.6.99-hhvm
Array
(
    [A] => IT__QQ__GREATE!
)
Array
(
    [A] => IT__QQ__GREATE!
)

csthomas · 2017-04-27T15:34:28Z

If you will add unittest for __QQ__ then you will have to:)

Bakual · 2017-04-27T15:49:52Z

Afaik currently unit tests run on HHVM. But the CMS itself not necessary. To my knowledge we don't support it yet officially.

wilsonge · 2017-04-27T22:06:46Z

tests/unit/suites/libraries/joomla/language/JLanguageTest.php

-		$strings = $this->inspector->parse(__DIR__ . '/data/bad.ini');
+		/**
+		 * suppressor used as we know this will generate a warning message
+		 * syntax error, unexpected BOOL_TRUE in


in.........

Coluld this https://github.com/PhilETaylor/joomla-cms/pull/1 can be useful?

TBH, I don't think it's even worth the effort to support HHVM anymore. Seems it is losing traction with the PHP community and big players are dropping support for it because apparently it doesn't have feature parity with PHP core starting with 5.6.

I do not care about HVVM support but I want to enforce a unit test for _QQ_.

@mbabker About my proposed changes. Would it be better to remove lines with hhvm support and allow the unit test on hhvm to fail? Or better way is to not add a unit test for _QQ_ now?

rdeutz · 2017-05-02T09:00:22Z

Can some test this so that we can get this in 3.7.1?

csthomas · 2017-05-02T09:13:28Z

libraries/joomla/language/language.php

+
+		if (!is_array($strings))
+		{
+			$strings = array();


@PhilETaylor If you are not convinced about merge my PR can you at least remove a ternary operator from line 857:

return is_array($strings) ? $strings : array();

which is equal to your new code:

if (!is_array($strings)) { $strings = array(); }

infograf768 · 2017-05-02T16:07:21Z

I have tested this item ✅ successfully on 30e69b3

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

AlexRed · 2017-05-02T16:17:16Z

I have tested this item ✅ successfully on 30e69b3

Patch ok for me

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

ghost · 2017-05-02T16:25:14Z

RTC after two successful tests.

MixingOnBeat · 2017-05-12T10:00:40Z

Boy, did I also have a rough time with this update from 3.6.5 to 3.7.0. After the upgrade all the text in the admin panel as well as some text on the public pages were a mess. I also have a host/server that doesn't allow me to alter php.ini files. :(

I had to revert back my changes (glad I made a backup of the files/database). I hope you guys find a common ground for 3.7.1. I read all arguments and though it would be nice that some hosts would not do this, but as long this can be fixed using a bit of code, perhaps this is best for everyone.

wilsonge · 2017-05-12T10:12:02Z

This patch got merged and we hope that 3.7.1 will drop towards the end of next week with the fix for you in :)

* Fix Language parsing when parse_ini_file disabled closes joomla#15587 Use sensible code available in FOF to process ini files when moronic web hosts disable parse_ini_file in php.ini Please test * Only load FOFUtilsIniParser if needed * Tabs not spaces * Unit Testing * tabs not spaces - darn editors * TABS! * Different approach * remove QQ replacement (apparently its not needed) * re-add QQ replacement (apparently its needed) * Revert joomla#13407 almost :) * ok back to this if/else * Update Unit Testing * Update Unit Testing * remove a ternary operator

hakanara · 2017-06-01T14:34:53Z

Hi all, this issue is still existing with a fresh 3.7.2 installation. Shall I open a new issue?

brianteeman · 2017-06-01T14:59:42Z

Yes please

hakanara · 2017-07-21T08:54:34Z

Fresh installation or a 3.7.3 upgrade still shows us that moronic hosting issue interferes Joomla and prevents Joomla from rocking. All Joomla loverz, let's unite and let Joomla rock!

hakanara · 2017-07-21T09:02:51Z

You are all invited to the reopening of #17198

Fix Language parsing when parse_ini_file disabled

a09fbb2

closes #15587 Use sensible code available in FOF to process ini files when moronic web hosts disable parse_ini_file in php.ini Please test

joomla-cms-bot added the PR-staging label Apr 27, 2017

This comment was marked as abuse.

Sign in to view

Phil E. Taylor added 3 commits April 27, 2017 12:25

Only load FOFUtilsIniParser if needed

c7f3b05

Tabs not spaces

bb2aee2

Unit Testing

3af06d1

joomla-cms-bot added the Unit/System Tests label Apr 27, 2017

Phil Taylor added 2 commits April 27, 2017 12:44

tabs not spaces - darn editors

0bc3e1e

TABS!

73785e9

This comment was marked as abuse.

Sign in to view

csthomas reviewed Apr 27, 2017

View reviewed changes

This comment was marked as abuse.

Sign in to view

Phil E. Taylor added 2 commits April 27, 2017 13:19

Different approach

358b81d

remove QQ replacement (apparently its not needed)

f30d514

This comment was marked as abuse.

Sign in to view

re-add QQ replacement (apparently its needed)

db093b3

This comment was marked as abuse.

Sign in to view

Revert #13407 almost :)

1cd95ca

This comment was marked as abuse.

Sign in to view

wilsonge requested changes Apr 27, 2017

View reviewed changes

infograf768 added this to the Joomla 3.7.1 milestone Apr 28, 2017

csthomas reviewed May 2, 2017

View reviewed changes

remove a ternary operator

30e69b3

This comment was marked as abuse.

Sign in to view

infograf768 mentioned this pull request May 2, 2017

Joomla 3.7 Update Error #15722

Closed

joomla-cms-bot removed this from the Joomla 3.7.1 milestone May 2, 2017

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 2, 2017

rdeutz merged commit 9b9c122 into joomla:staging May 3, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 3, 2017

rdeutz added this to the Joomla 3.7.1 milestone May 3, 2017

Bakual mentioned this pull request Jul 21, 2017

Language strings are still not parsed after upgrading to 3.7.3 #17198

Closed

andrepereiradasilva mentioned this pull request Jun 18, 2018

[Deprecate _QQ_ 3] Mark _QQ_ as deprecated and normalize language ini parsing #20321

Merged

Fix Language parsing when parse_ini_file disabled #15620

Fix Language parsing when parse_ini_file disabled #15620

Conversation

PhilETaylor commented Apr 27, 2017 • edited by joomla-cms-bot

dgrammatiko commented Apr 27, 2017

This comment was marked as abuse.

This comment was marked as abuse.

dgrammatiko commented Apr 27, 2017 • edited

zero-24 commented Apr 27, 2017

This comment was marked as abuse.

csthomas Apr 27, 2017 • edited

Choose a reason for hiding this comment

This comment was marked as abuse.

zero-24 Apr 27, 2017 • edited

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

zero-24 Apr 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

Bakual commented Apr 27, 2017

This comment was marked as abuse.

zero-24 commented Apr 27, 2017

This comment was marked as abuse.

zero-24 commented Apr 27, 2017

Bakual commented Apr 27, 2017

This comment was marked as abuse.

zero-24 commented Apr 27, 2017

This comment was marked as abuse.

brianteeman commented Apr 27, 2017

mbabker commented Apr 27, 2017

infograf768 commented Apr 27, 2017 • edited

This comment was marked as abuse.

csthomas commented Apr 27, 2017

This comment was marked as abuse.

csthomas commented Apr 27, 2017

Bakual commented Apr 27, 2017

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csthomas Apr 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

rdeutz commented May 2, 2017

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

This comment was marked as abuse.

infograf768 commented May 2, 2017

AlexRed commented May 2, 2017

ghost commented May 2, 2017

MixingOnBeat commented May 12, 2017

wilsonge commented May 12, 2017

hakanara commented Jun 1, 2017

brianteeman commented Jun 1, 2017

hakanara commented Jul 21, 2017

hakanara commented Jul 21, 2017 • edited

PhilETaylor commented Apr 27, 2017 •

edited by joomla-cms-bot

dgrammatiko commented Apr 27, 2017 •

edited

csthomas Apr 27, 2017 •

edited

zero-24 Apr 27, 2017 •

edited

zero-24 Apr 27, 2017 •

edited

infograf768 commented Apr 27, 2017 •

edited

csthomas Apr 28, 2017 •

edited

hakanara commented Jul 21, 2017 •

edited