Inject the JInput dependency into the session handler #15605

Merged
merged 1 commit into from Apr 27, 2017

Conversation

Projects
None yet
@mbabker
Member

mbabker commented Apr 26, 2017

Summary of Changes

Calling JFactory::getSession() without a properly configured session already injected into JFactory::$session results in an invalid JSession object being created in that the internal dependencies are not fully initialized. Namely, the JSessionHandlerJoomla class needs a JInput instance which is not injected at all. The web applications build the JSession object on their own and correctly call JSession::initialise() with the needed dependencies, hence the reason this bug has not been previously reported; it takes running a custom web application not extending JApplicationCms or a CLI application to reproduce the issue.

Testing Instructions

Place the following script in your site's cli directory:

<?php
// We are a valid entry point.
const _JEXEC = 1;

// Load system defines
if (file_exists(dirname(__DIR__) . '/defines.php'))
{
	require_once dirname(__DIR__) . '/defines.php';
}

if (!defined('_JDEFINES'))
{
	define('JPATH_BASE', dirname(__DIR__));
	require_once JPATH_BASE . '/includes/defines.php';
}

// Get the framework.
require_once JPATH_LIBRARIES . '/import.legacy.php';

// Bootstrap the CMS libraries.
require_once JPATH_LIBRARIES . '/cms.php';

// Configure error reporting to maximum for CLI output.
error_reporting(E_ALL);
ini_set('display_errors', 1);

class SessionTestCli extends JApplicationCli
{
	public function doExecute()
	{
		JFactory::getUser();
		$this->out('User retrieved');
	}
}

// Instantiate the application object, passing the class name to JCli::getInstance
// and use chaining to execute the application.
JApplicationCli::getInstance('SessionTestCli')->execute();

Expected result

Michaels-MacBook-Pro:joomla-cms mbabker$ php cli/session_test.php 
User retrieved

Actual result

Michaels-MacBook-Pro:joomla-cms mbabker$ php cli/session_test.php 

Notice: Trying to get property of non-object in /libraries/joomla/session/handler/joomla.php on line 69

Call Stack:
    0.0003     361576   1. {main}() /cli/session_test.php:0
    0.0333    1729656   2. JApplicationCli->execute() /cli/session_test.php:38
    0.0333    1729656   3. SessionTestCli->doExecute() /libraries/joomla/application/cli.php:142
    0.0333    1729656   4. JFactory::getUser() /cli/session_test.php:31
    0.0352    1884160   5. JSession->get() /libraries/joomla/factory.php:236
    0.0352    1884160   6. JSession->start() /libraries/joomla/session/session.php:486
    0.0352    1884160   7. JSession->_start() /libraries/joomla/session/session.php:608
    0.0352    1884160   8. JSessionHandlerJoomla->start() /libraries/joomla/session/session.php:648

Error displaying the error page: Application Instantiation Error: Call to a member function get() on null

@mbabker mbabker added this to the Joomla 3.7.1 milestone Apr 27, 2017

@nzampella

This comment has been minimized.

Show comment
Hide comment
@nzampella

nzampella Apr 27, 2017

Also related to issues with CRON jobs, see issue: 15603

Tested update on my Joomla/CiviCRM test site - CRON job now triggers all CiviCRM scheduled jobs as required.


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

nzampella commented Apr 27, 2017

Also related to issues with CRON jobs, see issue: 15603

Tested update on my Joomla/CiviCRM test site - CRON job now triggers all CiviCRM scheduled jobs as required.


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

@elkuku

This comment has been minimized.

Show comment
Hide comment
@elkuku

elkuku Apr 27, 2017

Member

I have tested this item successfully on 3292c86

Works.


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

Member

elkuku commented Apr 27, 2017

I have tested this item successfully on 3292c86

Works.


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

@mbabker mbabker removed the PR-staging label Apr 27, 2017

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.7.1 milestone Apr 27, 2017

@@ -594,8 +594,13 @@ protected static function createSession(array $options = array())
// Config time is in minutes
$options['expire'] = ($conf->get('lifetime')) ? $conf->get('lifetime') * 60 : 900;
+ // The session handler needs a JInput object, we can inject it without having a hard dependency to an application instance
+ $input = self::$application ? self::getApplication()->input : new JInput;

This comment has been minimized.

@laoneo

laoneo Apr 27, 2017

Member

Should we not use here the namespaced input class?

@laoneo

laoneo Apr 27, 2017

Member

Should we not use here the namespaced input class?

This comment has been minimized.

@mbabker

mbabker Apr 27, 2017

Member

No. 3.x isn't in a spot to use the Framework's Input class and 3.7 doesn't have namespaced classes.

@mbabker

mbabker Apr 27, 2017

Member

No. 3.x isn't in a spot to use the Framework's Input class and 3.7 doesn't have namespaced classes.

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Apr 27, 2017

Contributor

I have tested this item successfully on 3292c86


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

Contributor

alikon commented Apr 27, 2017

I have tested this item successfully on 3292c86


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

@wilsonge wilsonge merged commit c230491 into joomla:staging Apr 27, 2017

4 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wilsonge wilsonge added this to the Joomla 3.7.1 milestone Apr 27, 2017

@mbabker mbabker deleted the mbabker:factory-session branch Apr 27, 2017

@photodude photodude referenced this pull request in photodude/AssetFix Apr 28, 2017

Open

Session handler causes errors #1

rdeutz added a commit to joomlajenkins/joomla-cms that referenced this pull request May 1, 2017

@alex-equities

This comment has been minimized.

Show comment
Hide comment
@alex-equities

alex-equities May 3, 2017

Contributor

that fixed it, thanks

Contributor

alex-equities commented May 3, 2017

that fixed it, thanks

@tso2085

This comment has been minimized.

Show comment
Hide comment
@tso2085

tso2085 May 8, 2017

I was receiving this error when running a CRON job for CiviCRM. I made the changes to the factory.php file, but now I get a different error when the CRON job executes. I verified (3 times) that the changes were made correctly. The error now is:

Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /../../public_html/../ccprod/libraries/joomla/session/handler/joomla.php on line 46
Error displaying the error page: Application Instantiation Error: Failed to start the session because headers have already been sent by "/../../public_html/../ccprod/libraries/joomla/session/handler/joomla.php" at line 46.

tso2085 commented May 8, 2017

I was receiving this error when running a CRON job for CiviCRM. I made the changes to the factory.php file, but now I get a different error when the CRON job executes. I verified (3 times) that the changes were made correctly. The error now is:

Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /../../public_html/../ccprod/libraries/joomla/session/handler/joomla.php on line 46
Error displaying the error page: Application Instantiation Error: Failed to start the session because headers have already been sent by "/../../public_html/../ccprod/libraries/joomla/session/handler/joomla.php" at line 46.

@vishalCTUK

This comment has been minimized.

Show comment
Hide comment
@vishalCTUK

vishalCTUK Jun 23, 2017

I receive the same error as tso2085, with my cron job from CiviCRM

Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /home/xxx/public_html/libraries/joomla/session/handler/joomla.php on line 46, I have error with the settings below
CivivCRM 4.7.20
Joomla 3.7.2
PHP 5.6

I receive the same error as tso2085, with my cron job from CiviCRM

Warning: ini_set(): A session is active. You cannot change the session module's ini settings at this time in /home/xxx/public_html/libraries/joomla/session/handler/joomla.php on line 46, I have error with the settings below
CivivCRM 4.7.20
Joomla 3.7.2
PHP 5.6

@brianteeman

This comment has been minimized.

Show comment
Hide comment
@brianteeman

brianteeman Jun 23, 2017

Contributor

commenting on a closed issue will not be seen

Contributor

brianteeman commented Jun 23, 2017

commenting on a closed issue will not be seen

@tso2085

This comment has been minimized.

Show comment
Hide comment
@tso2085

tso2085 Jun 23, 2017

vishalCTUK,

I created an item in the Joomla forum also. Unfortunately there have been no updates or suggestions to it. Feel free to add to the forum item if you wish:
https://forum.joomla.org/viewtopic.php?f=706&t=950332

This is still an issue for me as well.

tso2085 commented Jun 23, 2017

vishalCTUK,

I created an item in the Joomla forum also. Unfortunately there have been no updates or suggestions to it. Feel free to add to the forum item if you wish:
https://forum.joomla.org/viewtopic.php?f=706&t=950332

This is still an issue for me as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment