[#12315] - Shared session without another new parameter in config
- Closed
- 10 Aug 2017
- Medium
- Build: staging
- # 12315
- Diff
- izharaazmi:session-name
- Success continuous-integration/drone the build was successful Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Summary of Changes
This is in continuation with @mbabker's #12068. For more details see that PR.
The addition of this in the global configuration page is a nice improvement. The same effect can be achieved without introducing an new parameter shared_session. Having two parameters for the same purpose is not a great idea. However we cannot get rid of existing session_name parameter due to our B/C promise. Hence this PR, and this is valid only before the first public release of J3.7.
With or without this patch:
// Not shared session, empty 'session_name' in configuration.php
JFactory::getConfig()->get('session_name'); // ''
JFactory::getApplication()->get('session_name'); // 'site' or 'administrator'
// Shared session, pre-defined 'session_name' in configuration.php, e.g. 'abcde'
JFactory::getConfig()->get('session_name'); // 'abcde'
JFactory::getApplication()->get('session_name'); // 'abcde'Database changes in the other PR can also be avoided but it doesn't harm in any way, so I have skipped them. I can do that if advised so.
Testing Instructions
- This should behave exactly as described in #12068.
- Add
public $session_name = 'something';to yourconfiguration.phpand login to backend. You should be automatically logged in to front-end as well. The reverse is also true, provided you have enough access rights. - Without this patch if you open and save global configuration your setting will be lost, as mbabker said.
- Apply this patch, open and save global configuration. Everything should be same as in 2 above, the value in the configuration may change to 'shared' but the behaviour will be same.
Documentation Changes Required
None, however any references to the new parameter shared_session should be removed if already added.
PS: It is also possible to avoid logout when changing this setting from backend by using 'administrator' as the shared session_name, however I am not sure if this is a good idea.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Category | ⇒ | Administration Components Modules Installation Libraries Front End Plugins |
Well. I understand that the two ways of reading the parameter return different values. That is perfectly fine. JFactory::getConfig()->get() is supposed to return the set configuration values, and the JFactory::getApplication()->get() is for the current environment values for them which may be set by various defaults or even a plugin or whoever is concerned.
This is true for various parameters in configuration such as language etc. We just need to be aware of the differences and use what we mean to.
And I'd call that inconsistent behavior. If I have to be aware of whether each parameter is a runtime parameter or something static and access it appropriately, that seems wrong; the parameters in configuration.php should match whether you read the config through the application or the global factory.
... the parameters in
configuration.phpshould match whether you read the config through the application or the global factory.
IMHO, That is not true.
Application does manipulates the values inside $this->config and it may be different during runtime than the stored config... and that is correct behaviour.
Had it been the motive like what you said, they should not ever modify the values at runtime and not update the other instance. (Refer to JUser where the several instances are updated whenever needed coz they must remain in sync)
I'll leave it this way. When I fetch a configuration value, regardless of source, it should match. If I can go through the application and the factory to get two different values for a single configuration parameter, we have created a massive developer experience fail and a massive inconsistency in our API.
Yes, I get there are some runtime configuration parameters; values that get set after the initial read from JConfig based on whether a parameter is configured, or parameters that are request specific (i.e. with JApplicationWeb reading $this->get('uri');). But there should be very few instances where these values differ.
In either case relying on whether session_name has a value is too flaky to be used reliably. It may not be set in JConfig (the data object returned from the factory) but will have a value in the application configuration. On that argument alone I find this pull request invalid because in some places you are reading this from one source and in some places the other source; for this parameter the only valid source is the application config (runtime). Whereas shared_session is a static value and consistent regardless of anything else; IMO it is much better to use a tracking parameter and have our API programmatically take care of the underlying behaviors for the user than relying on something which as demonstrated is inconsistent and easily manipulated and make it easier for the user to screw something up.
I find this pull request invalid because in some places you are reading this from one source and in some places the other source;
My bad, I just replaced usage with very little brain application, that was a mistake
brianteeman
- | Labels |
Added:
?
Removed: ? |
||
Should this be closed as the other PR was merged?
| Category | Administration Components Modules Installation Libraries Front End Plugins | ⇒ | Administration com_config Modules Installation Libraries Front End Plugins Components |
Closing as no longer valid.
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-08-10 07:47:31 |
| Closed_By | ⇒ | izharaazmi |
The entire reason I went the way I did was to keep the user from inputting their own value and force the system to generate a random value.
Also,
JFactory::getApplication()->get('session_name')will always return a value because theJApplicationCmsconstructor will set the value to the current app name if a value hasn't been injected into the configuration when instantiated. This will causeJFactory::getApplication()->get('session_name')andJFactory::getConfig()->get('session_name')to return different values.So the
session_nameparameter remains as it has been implemented previously, only a parameter name affecting the generation of the session name, and the newshared_sessionparameter acts as a tracking parameter. I would suggest not accepting this PR.