?
avatar mbabker
mbabker
22 Mar 2016

Steps to reproduce the issue

As of 943f2e9 the install app can't be run because of a circular reference to JFactory::getApplication()

Expected result

Joomla can be installed

Actual result

Error displaying the error page: Application Instantiation Error: Application Instantiation Error

Stack trace:
screencapture-jcms-installation-index-php-1458655317653

System information (as much as possible)

Staging checked out as of 943f2e9

avatar mbabker mbabker - open - 22 Mar 2016
avatar MATsxm
MATsxm - comment - 22 Mar 2016

Confirmed

avatar mbabker mbabker - change - 22 Mar 2016
Labels Added: ?
avatar mbabker mbabker - change - 22 Mar 2016
Category Installation Libraries
avatar mbabker mbabker - change - 22 Mar 2016
Priority Medium Critical
avatar brianteeman brianteeman - change - 22 Mar 2016
Milestone Added:
avatar brianteeman brianteeman - change - 22 Mar 2016
Milestone Added:
avatar wilsonge
wilsonge - comment - 22 Mar 2016

We somehow need to expose the input object in the session then :(

avatar mbabker
mbabker - comment - 22 Mar 2016

You already have it by injecting it as part of JSession::initialise(). Anyone not doing that isn't getting the event dispatching mechanism either.

You can't make JSession dependent on JFactory::getApplication() before the session is started (which happens in the application constructors and before JFactory::$application is assigned) because it introduces circular dependencies. I hate to say it, but for me the only fix (which you can argue B/C about all day long) is scripts HAVE to call JSession::initialise() and inject the JInput object in before starting sessions.

avatar mbabker mbabker - reference | 943f2e9 - 22 Mar 16
avatar mbabker
mbabker - comment - 22 Mar 2016

Just some clarity on why things worked on 3.4.8 and not with 3.5.0.

First, backtracking a little further (probably 3.2, I don't remember clearly anymore). In the JApplicationCms instances (site/admin & install apps) a JSession object is created as part of the application constructor, the application's JInput and JEventDispatcher instances are injected into that JSession object via JSession::intialise(), THEN the session is started. In the case of the install app, if doesn't use JFactory to create the JSession object but rather directly uses JSession::getInstance() then assigns to JFactory::$session to make the API valid. These routes have worked fine from 3.2 up to the referenced commit.

In 3.4.8 and earlier, scripts could rely on a session being internally started without issue because of the manner in which JFactory::createSession() operates. Specifically, if it detects that the session is in the expired state, it will trigger JSession::restart() and through that code path it will trigger JSession::_start() in a way that would never require the JInput object to be injected. Under the hood everything would seemingly just work.

In 3.5, the session API is refactored a bit to split off some handling to a new API layer instead of directly in the JSession class. Through this change, the JSessionHandlerJoomla object that gets used by default has a dependency on a JInput object being injected into it before JSession::start() or JSession::restart() are called. Because Joomla doesn't have a proper service layer, the hack with this was to use JSession::initialise() to push it into the handler. Also, now JSession::_start() does not have different code paths for if the state is "restart" or not. Now all code paths have a dependency when the JSessionHandlerJoomla handler is in use to have an injected JInput object before the session starts.

JFactory::getApplication() cannot be relied on for this. As explained above, sessions in Joomla's web applications are instantiated and started in the application's constructors, before JFactory::$application is assigned. So the commit that broke things now creates a circular dependency because the handler will try to access JFactory::$application before it is assigned and JFactory::getApplication() throws an Exception if you call it without a name parameter and the object isn't set in the static store.

The quick and dirty hack around this for JApplicationCms instances is to assign JFactory::$application = $this in the constructor before loading the session, but that's no fix for the long term. JFactory::createSession() can't reliably use JFactory::getApplication() to inject a JInput object through JSession::initialise() because it may not be assigned. One possible workaround is to call $session->initialise(new JInput, JEventDispatcher::getInstance()) in JFactory::createSession() and that will ensure a JInput instance is always injected in, but this means the session and application objects are now using different JInput instances and that is not reliable given JInput's internals (the default constructor creates a reference to the $_REQUEST superglobal but when you call something like $input->cookie it creates a new JInput object using the appropriate superglobal but it is not a reference, so now the application and session JInput objects would have internal references to different JInputCookie objects with different data stores).

avatar Kubik-Rubik
Kubik-Rubik - comment - 22 Mar 2016

@mbabker Thank you for the explanation! In one of my installation script I also did the dirty hack / workaround (JFactory::$application = $this)... :-)

We need to fix this issue asap since Staging state is broken (also tests are all failing...).

avatar wilsonge
wilsonge - comment - 23 Mar 2016

So on top of the straight initialise is now required thing looks like i rebuild the regenerate behaviour but something is now a bit weird - it calls session_start in the JSessionHandlerNative::regenerate() function - which can lead it to try and start the session twice (e.g. if you call restart) as it then calls the main handler start straight afterwards and because regenerate doesn't set the JHandlerNative::$started flag to true throws a RuntimeException in the started method before it starts the session twice. That's actually the high priority bug

Then we work out how we want to deal with the initialise bug. This only affects custom scripts - most of who call JFactory::getSession and then do shit without ever calling initialise as michael stated - so it's not actually QUITE as bad as I suspected.

As for the initialise our abstraction of $_COOKIE is already pretty buggered - when we destroy the session we run

We can do an isset() check on the cookie call as well. I mean how many CLI scripts are actually going to depend on the state of the session in the cookie (that's almost certainly not set!) and if there are a few scripts left then I'm sure it's not beyond them to add the initialise call?? (though really far from ideal)

avatar brianteeman
brianteeman - comment - 26 Mar 2016

Just tested and the current staging now installs - I assume it was fixed elsewhere and am closing this. (If i'm wrong it can always be re-opened)


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

avatar brianteeman brianteeman - change - 26 Mar 2016
Status New Closed
Closed_Date 0000-00-00 00:00:00 2016-03-26 10:37:16
Closed_By brianteeman
avatar brianteeman brianteeman - close - 26 Mar 2016
avatar joomla-cms-bot joomla-cms-bot - change - 26 Mar 2016
Status Closed New
Closed_Date 2016-03-26 10:37:16
Closed_By brianteeman
avatar joomla-cms-bot joomla-cms-bot - reopen - 26 Mar 2016
avatar brianteeman brianteeman - close - 26 Mar 2016
avatar brianteeman
brianteeman - comment - 26 Mar 2016

dont know why the bot re-opened it - closed again

avatar brianteeman brianteeman - change - 26 Mar 2016
Status New Closed
Closed_Date 0000-00-00 00:00:00 2016-03-26 10:38:38
Closed_By brianteeman
avatar brianteeman brianteeman - close - 26 Mar 2016
avatar brianteeman brianteeman - close - 26 Mar 2016
avatar wilsonge wilsonge - close - 26 Mar 2016
avatar brianteeman brianteeman - close - 26 Mar 2016
avatar wilsonge wilsonge - change - 9 Apr 2016
Labels Removed: ?
avatar brianteeman brianteeman - change - 22 Apr 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 22 Apr 2016
Labels Removed: ?
avatar brianteeman brianteeman - change - 22 Apr 2016
Labels Added: ?

Add a Comment

Login with GitHub to post a comment