Skip to content

Commit

Permalink
Fix issues with JSession on CLI Scripts fixes #9076
Browse files Browse the repository at this point in the history
  • Loading branch information
@wilsonge
wilsonge committed Mar 22, 2016
1 parent 2163a81 commit 943f2e9
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions libraries/joomla/session/handler/joomla.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,11 @@ public function start()
$session_name = $this->getName();

// Get the JInputCookie object
$cookie = $this->input->cookie;
$cookie = JFactory::getApplication()->input->cookie;

if (is_null($cookie->get($session_name)))
{
$session_clean = $this->input->get($session_name, false, 'string');
$session_clean = JFactory::getApplication()->input->get($session_name, false, 'string');

if ($session_clean)
{
Expand Down

7 comments on commit 943f2e9

@beat
Copy link
Contributor

@beat beat commented on 943f2e9 Mar 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilsonge This might fix the CLI, but it breaks the WEB application completely!

@beat
Copy link
Contributor

@beat beat commented on 943f2e9 Mar 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilsonge also wonder how that got committed directly to staging, which didn't go through automated tests that would have catched that?

@mbabker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't break any unit tests because the session package has next to no coverage and the code here isn't covered by the handful of cases that are indirectly covering code in this class.

@beat
Copy link
Contributor

@beat beat commented on 943f2e9 Mar 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but web app is broken, front-end and admin area...

@mbabker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The apps aren't broken. A circular dependency has been introduced by this commit and somewhat explained in #9520 (I'm writing a more detailed comment right now).

@beat
Copy link
Contributor

@beat beat commented on 943f2e9 Mar 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does break my test-site with Joomla staging including this commit and latest Community Builder. This is a major B/C break. I have opened an issue here #9533 .

Update: Ok, #9520 covers same issue, just says it's at install time only, but actually it's also valid for installed sites.

@mbabker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the grand scheme of things this commit broke the functionality of Joomla. But it did not fatally break the applications or the API. Plus, even with unit tests fully covering the JSession API, this wouldn't have raised a red flag because the needed dependencies would have been mocked and set up before the test run. If the system tests were executable in Travis, those probably would have caught it but honestly I'm not sure of the state of those at this point (whether they're usable or what state the efforts to wire those into the Travis-CI matrix is in).

As my last comment on #9520 pointed out, the reason things stopped working as expected is because this introduced a circular dependency that cannot be correctly resolved given the order of operations. If Joomla applications did not auto-start sessions in the application class constructors and instead used a lazy start method where the session is started at the first point data is needed (which happens a couple of lines into $app->execute()) then this commit wouldn't have broken the web applications (and honestly wouldn't have made the CLI situation any worse; instead of erroring trying to start the session they'd error trying to get the application since most don't assign to JFactory::$application). It's just a simple order of operations issue.

Please sign in to comment.