[4.0] Optional session metadata, misc. session tweaks by mbabker · Pull Request #13322 · joomla/joomla-cms

mbabker · 2016-12-21T21:03:23Z

Summary of Changes

This should in full decouple the requirement for session metadata. It's done by adding a new global configuration switch (default on because B/C) that when switched off features dependent on this data will need to check. Plus, some other tweaks squeezed in while testing stuff.

Summary of change:

New config field, if switching off metadata then the config model will also purge what data we have
Features dependent on session metadata adjusted:
- Joomla\CMS\Application\CMSApplication::afterSessionStart() does not run the code to write or cleanup metadata when disabled
- Admin mod_logged module shows a message if metadata is disabled versus a list of users
- Admin mod_status module hides site/admin user counts if metadata is disabled
- Site mod_whosonline module shows a message if metadata is disabled versus a count of users
- PlgUserJoomla only interacts with the session data when the environment is set for it
- Joomla\CMS\Table\Table::isCheckedOut() modified to account for the metadata tracking
Added fields to configure Redis session handler
Removed fields to configure the now unsupported Memcache session handler
Joomla\CMS\Application\CMSApplication::afterSessionStart() will no longer cause an application fatal error if writing the session metadata fails; either it'll fail again when writing the at the end of the request (if using the database handler) and the session fails to persist or the user isn't using the database handler and it really doesn't matter anyway

Testing Instructions

With this patch applied over the 4.0 branch, it should be possible to turn off writing session related metadata to the database. When using a non-database handler, the session table should always stay empty. Switching this off when enabled should clear existing metadata.

Documentation Changes Required

Note that it'll be possible for data to not always be persisted into the database, namely the session ID, client, user ID, and username. When enabled, this will cause the checked out checks to not be able to determine if a user is still logged in.

mbabker · 2017-09-27T23:13:21Z

Is this worth my time to pursue rebasing?

brianteeman · 2017-09-27T23:28:34Z

Sorry i didnt test this as i didnt understand it and others have repeatedly stated that only experienced developers should test library changes. if only those same people would actually do that :(

mbabker · 2017-09-28T00:07:53Z

Well it is kind of technical in nature, and it also affects user facing features. So a mix of both would be helpful. But I'm really not interested in fighting with 9 months of merge conflicts if it's going to sit around.

joomdonation · 2017-12-26T09:44:09Z

Just want to add that if people agree with the missing features when session meta data is turned off, I can test and review this PR once conflict resolved

brianteeman · 2018-01-04T18:36:38Z

@mbabker looks like if you resolve the cconflicts there are at least two of us prepared to test

mbabker · 2018-01-05T01:04:46Z

OK this should be good to go.

brianteeman · 2018-01-05T10:29:34Z

When using a non-database handler, the session table should always stay empty. Switching this off when enabled should clear existing metadata.

It clears the metadata field but leaves the record with the session_id - is that correct?

brianteeman · 2018-01-05T10:31:16Z

I have tested this item ✅ successfully on b2dab59

Other than my question above everything still works as i would expect it to work and the module changes work as described etc

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13322.}

mbabker · 2018-01-05T12:57:40Z

It clears the metadata field but leaves the record with the session_id - is that correct?

Correct. The session related fields that are tracked are the session ID, time, and data.

brianteeman · 2018-01-05T13:00:29Z

All seems good to me then

joomdonation · 2018-01-05T14:43:47Z

It works for me, too. Just an issue with Session Save Path setting:

Might be a problem with path filter but when I enter a windows path like D:\www\sessiondata , it could not be saved
I guess you need to add some validation to the path to make sure it exists (like other paths). When a wrong path is entered, the application could not be started, an error like this displayed:

Warning: mkdir(): No such file or directory in D:\www\joomla4\libraries\vendor\joomla\session\src\Handler\FilesystemHandler.php on line 55
Error displaying the error page: A \Joomla\Session\SessionInterface object has not been set.: Could not create session directory "/home/abc"

joomdonation · 2018-01-05T14:45:39Z

libraries/src/Application/CMSApplication.php

+			 * 1) The database handler is in use and the session is new
+			 * 2) The database handler is not in use and the time is an even numbered second or the session is new
+			 */
+			if (($handler !== 'database' && ($time % 2 || $session->isNew())) || ($handler === 'database' && $session->isNew()))


Just want to be sure, when $handler !== 'database' and Track Session Metadata is off, do we still need to call $this->checkSession();?

This is already inside a conditional checking if the metadata is being tracked (the if statement starting at line 166)

Ah, OK. Thanks, see it now

brianteeman · 2018-01-05T14:46:54Z

@joomdonation - enter the wrong data and you get an error message telling you that - seems fine to me.
Pretty sure that this is nothing to do with this PR though and as such is out of scope here.

joomdonation · 2018-01-05T14:48:18Z

I'm not getting error. The path is saved to configuration.php and then the application could not be stared like I said

brianteeman · 2018-01-05T14:50:59Z

thats what I meant. You are getting the error (just not immediately).

BUT if this happened before this PR then it is out of scope and you should create a new issue if you feel it is wrong

joomdonation · 2018-01-05T14:51:12Z

Here is the screenshot after I entered a wrong path and save configuration, the site is completely broken:

joomdonation · 2018-01-05T14:54:08Z

I read description of the PR and thought this is included:

Added field to configure the file path if using the file session handler

But you are correct, look like that setting was introduced before this PR

mbabker · 2018-01-05T14:58:34Z

Adding that setting was once in scope of this PR but at some point it got added to the repo separately, removed that note from the main description as the diff clearly shows that setting isn't being touched here now.

joomdonation · 2018-01-05T15:01:07Z

I have tested this item ✅ successfully on b2dab59

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13322.}

ghost · 2018-01-05T15:19:25Z

Ready to Commit after two successful tests.

mbabker · 2018-02-14T14:06:29Z

No longer interested in pursuing correct session architecture in Joomla. Bye.

tonypartridge · 2018-04-05T01:06:36Z

Where are we with this? I see we got two successful tests but no merge and now conflicts?

…he session table

…atus

…ath for session data

…bled layout markup for updated BS card structure

mbabker · 2018-04-10T13:46:39Z

Rebased. See y'all at the next merge conflict 😉

laoneo · 2018-04-11T06:08:27Z

administrator/components/com_config/forms/application.xml

 		label="CONFIG_SESSION_SETTINGS_LABEL">

-		<field
-			name="shared_session"


Shared sessions between front end will not be possible anymore, even when session meta data is enabled?

No, it's a duplicate field definition. Still exists at (new) line 936. Just with all the rebasing and merging and whatnot at this point somehow it's in the form twice.

Ah ok, then all is fine here.

laoneo · 2018-04-11T06:09:13Z

administrator/components/com_config/forms/application.xml

-			type="radio"
-			class="switcher"
-			default="0"
-			label="COM_CONFIG_FIELD_SHARED_SESSION_LABEL"


Strings should also be removed here.

joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Dec 21, 2016

brianteeman modified the milestone: Joomla 4.0 Jun 8, 2017

joomdonation reviewed Jan 5, 2018

View reviewed changes

joomla-cms-bot removed this from the Joomla 4.0 milestone Jan 5, 2018

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 5, 2018

brianteeman added this to the Joomla 4.0 milestone Jan 12, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 14, 2018

Michael Babker and others added 13 commits April 10, 2018 08:42

Add a config switch to disable session metadata

4301438

If disabling session metadata and not using database handler, purge t…

cf1aee4

…he session table

Check in extensions depending on session metadata for the tracking st…

4fa9828

…atus

Check in JTable::isCheckedOut for the metadata tracking status

905a13c

Purge the metadata if disabling it regardless of handler

18d9ebb

Lowercase the handler names in the form field, add configurable filep…

652039c

…ath for session data

This wasn't actually a DBO, oops

08b57dd

Hello Chicken, I'm Egg

e2189c9

Need the user var to actually be a user

0d386c0

Duplicated field, move field to end of set

67a0362

Fix a couple bad merges from project repo

9c27151

Fix issues from rebase

a75591b

Remove duplicate config fields, remove dead Memcache fields, fix disa…

9f737e7

…bled layout markup for updated BS card structure

laoneo reviewed Apr 11, 2018

View reviewed changes

Merge branch '4.0-dev' into 4.0-no-session-metadata

f980649

laoneo merged commit ec78aae into joomla:4.0-dev Apr 11, 2018

Uh oh!

Conversation

mbabker commented Dec 21, 2016 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Documentation Changes Required

Uh oh!

mbabker commented Sep 27, 2017

Uh oh!

brianteeman commented Sep 27, 2017

Uh oh!

mbabker commented Sep 28, 2017

Uh oh!

joomdonation commented Dec 26, 2017

Uh oh!

brianteeman commented Jan 4, 2018

Uh oh!

mbabker commented Jan 5, 2018

Uh oh!

brianteeman commented Jan 5, 2018

Uh oh!

brianteeman commented Jan 5, 2018

Uh oh!

mbabker commented Jan 5, 2018

Uh oh!

brianteeman commented Jan 5, 2018

Uh oh!

joomdonation commented Jan 5, 2018

Uh oh!

joomdonation Jan 5, 2018

Choose a reason for hiding this comment

Uh oh!

mbabker Jan 5, 2018

Choose a reason for hiding this comment

Uh oh!

joomdonation Jan 5, 2018

Choose a reason for hiding this comment

Uh oh!

brianteeman commented Jan 5, 2018

Uh oh!

joomdonation commented Jan 5, 2018

Uh oh!

brianteeman commented Jan 5, 2018

Uh oh!

joomdonation commented Jan 5, 2018

Uh oh!

joomdonation commented Jan 5, 2018

Uh oh!

mbabker commented Jan 5, 2018

Uh oh!

joomdonation commented Jan 5, 2018

Uh oh!

ghost commented Jan 5, 2018

Uh oh!

mbabker commented Feb 14, 2018

Uh oh!

tonypartridge commented Apr 5, 2018

Uh oh!

mbabker commented Apr 10, 2018

Uh oh!

laoneo Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

mbabker Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

laoneo Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

laoneo Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mbabker commented Dec 21, 2016 •

edited by joomla-cms-bot

Loading