PR-staging

Success

User tests: Successful: Unsuccessful:

avatar demis-palma
demis-palma
19 Apr 2016

Summary of Changes

A wrong path for the main configuration file prevents the database from open a connection under CLI scripts.

Testing Instructions

Create and run a typical CLI script from scratch as described in the documentation
Example 1: Hello World Command Line Interface (CLI) App

In JFilterInput::__construct() an attempt to connect to the database with $db->connect();
fails because the configuration files has not been read yet
(it will be read afterwards, but it is too late and the invalid database connection is cached and served to subsequent calls)

No matter whether you have a valid configuration.php or not, this always raises an exception and makes
absolutely unreachable the recent code added to handle UTF8mb4
// And now we can decide if we should strip USCs
$this->stripUSC = $db->hasUTF8mb4Support() ? 0 : 1;

the instruction within the catch block will be always executed instead
// Could not connect to MySQL. Strip USC to be on the safe side.
$this->stripUSC = 1;

Since the exception is caught and the program flux continues, I think that the only way to inspect this behaviour is using a breakpoint to follow the execution.

avatar demis-palma demis-palma - open - 19 Apr 2016
avatar demis-palma demis-palma - change - 19 Apr 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 19 Apr 2016
Labels Added: PR-staging
avatar wilsonge
wilsonge - comment - 19 Apr 2016

Hey. I'm pretty sure that to get things to work all you need to do is require the configuration file like in this script https://github.com/joomla/joomla-cms/blob/staging/cli/finder_indexer.php#L51

Note that I agree with your change and support us merging it - as this has been an issue for a long long time...

avatar brianteeman brianteeman - change - 20 Apr 2016
Category CLI
avatar demis-palma
demis-palma - comment - 20 Apr 2016

Thanks @wilsonge I confirm that including the configuration file makes the things work. This is the workaround I used in my own scripts. Nevertheless I thought to propose a PR.
Since the Database Object relies in the configuration, I think that it should be able to load the config file by itself, if needed.

avatar mbabker
mbabker - comment - 8 May 2016

My two cents...

JFactory::createConfig() has had this default since it was introduced, which has always gone against the CMS' default config path. Technically it's probably the stupidest B/C break that could be introduced but it is one, and dammit it needs to be made because the fact that JFactory can't by default load the application's configuration is probably one of the dumbest things the API does right now.

avatar wilsonge wilsonge - change - 8 May 2016
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-05-08 07:50:44
Closed_By wilsonge
avatar wilsonge wilsonge - close - 8 May 2016
avatar wilsonge wilsonge - merge - 8 May 2016
avatar wilsonge wilsonge - reference | 28ab509 - 8 May 16
avatar wilsonge wilsonge - merge - 8 May 2016
avatar wilsonge wilsonge - close - 8 May 2016
avatar demis-palma
demis-palma - comment - 30 Jun 2016

Just a question about Github behaviour: since this PR has been already merged, can I safely delete this branch from my own repository?

avatar andrepereiradasilva
andrepereiradasilva - comment - 30 Jun 2016

yes

Add a Comment

Login with GitHub to post a comment