[#11351] - Break and deprecate internal dependency to JFactory::createSession()
- Closed
- 12 Sep 2016
- Medium
- Build: staging
- # 11351
- Diff
- mbabker:JSession-set-to-factory
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Summary of Changes
There is a rather difficult to deal with dependency to JFactory::createSession() to actually create a JSession instance and have it function correctly. In a move that's somewhat similar to how the install application deals with this, I've refactored the CMS web application class to avoid using JFactory::createSession() to create the session and inlined all appropriate logic into the application's loadSession() method.
I also propose deprecating JFactory::createSession() as this is now duplicated logic and we should begin working out how to transition the CMS to proper service injection and retrieval. The application classes are creating their own session objects and storing them internally at this point, we should point JFactory::getSession() to that reference when possible. JFactory::getSession() should only return a null session instance if one cannot be created from the application object as of 4.0.
Also fixed is checking in JFactory::createSession() if an expire time was injected via the options array and not re-calculating that if so; in custom applications this could cause unwanted behavior but for the default CMS behaviors it is calculating from the same data sources.
Testing Instructions
Sessions still function correctly with this patch applied.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Labels |
Added:
?
|
||
Well either inline creating a null JSession object into getSession() or leave createSession() to do all the stuff I commented right into the code.
Also, this is kind of a mid-point between what we have now and what's in your 4.0 branch, so it's kinda jumping the gun on moving toward a state that's being built from there.
I'm just considering the number of people who in CLI apps that require some sort of session state to exist who just call JFactory::getSession()->restart(). Is anything here going to break that workflow. I don't think so right?
Right now no. Nothing has changed at all in the existing workflow for how JFactory creates the session. What has changed is JApplicationCms using JFactory to create it and basically just creates the session itself and direct assigns to JFactory, similar to what's happening in the install app.
And what's the recommended flow for those building CLI apps that require/use sessions now that createSession is deprecated?
Like I said, we can make JSession in 4.0 build a null session. For 3.x,
there's no change.
On Saturday, July 30, 2016, George Wilson notifications@github.com wrote:
And what's the recommended flow for those building CLI apps that
require/use sessions now that createSession is deprecated?—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#11351 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfofBeNm_mk3FtjpLBcfWQHgvhWxRoks5qa9YogaJpZM4JYKT6
.
I have tested this item
Session still works correctly.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/11351.
Nevermind.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-09-12 20:52:47 |
| Closed_By | ⇒ | mbabker |
createSessionis going to be called by a number of CLI scripts I guess still?