Remove extra User::__wakeup() on session write

[Denitz]

Summary of Changes

Once the session is closed (before write), Joomla\CMS\Session\Storage\JoomlaStorage::close() creates the base64-decoded serialized session registry within cloning the session registry object ($this->data):

	public function close(): void
	{
		// Before storing data to the session, we serialize and encode the Registry
		$_SESSION['joomla'] = base64_encode(serialize(clone $this->data));

		parent::close();
	}

Joomla\Registry\Registry object's magic clone unserializes serialized data:

	public function __clone()
	{
		$this->data = unserialize(serialize($this->data));
	}

This unserialize results in magic wakeup of all objects in the session registry.

Joomla\CMS\User instance in session has the magic wakeup which results in additional load of data.

Basically, we have a chain of User-related magic sleep/wakeup:

1. User::__sleep() - on Joomla\Registry\Registry::__clone() due to serialize()
2. User::__wakeup() - on Joomla\Registry\Registry::__clone() due to unserialize()
3. User::__sleep() - on Joomla\CMS\Session\Storage\JoomlaStorage::close() due to serialize()

This 2nd step is actually an extra wakeup which results in two extra DB queries which don't have any logic because the saved session contains only user ID (see User::__sleep()) and the session read re-loads the data via User::__wakeup().

I don't see any purpose of cloning the session registry before serializing it, any thoughts?

Testing Instructions

1. Log in Joomla.
2. Add full logging of SQL queries. Edit libraries/vendor/joomla/database/src/DatabaseDriver.php and add after line 1872:

		file_put_contents(
			JPATH_SITE.'/tmp/'.microtime(true).'.txt',
			$query .
			"\r\n" .
			print_r($query->getBounded(), true) .
			"\r\n" .
			print_r(debug_backtrace(2), true)
		);

3. Load Joomla page
4. See bunch of files in /tmp folder
5. Search for these two extra queries in the final files, note the :userid bound param as your user ID:

SELECT *
FROM `#__users`
WHERE `id` = :userid

SELECT `g`.`id`,`g`.`title`
FROM `#__usergroups` AS `g`
INNER JOIN `#__user_usergroup_map` AS `m` ON `m`.`group_id` = `g`.`id`
WHERE `m`.`user_id` = :muserid

6. See the trace in files, these queries are the result of clone/sleep/wakeup described above.
7. Apply patch
8. Delete debug files
9. Re-load Joomla page
10. See less files (-2) - means no extra queries

Actual result BEFORE applying this Pull Request

Joomla executes extra two database queries on each page load for logged user.

Expected result AFTER applying this Pull Request

No extra queries. Joomla works as before.

Documentation Changes Required

No.

[prakhar3062]

@test

patch works

---

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

[HLeithner]

This pull request has automatically rebased to 4.2-dev.

[joomla-bot]

This pull requests has been automatically converted to the PSR-12 coding standard.

[Denitz]

Somebody, please restart drone.

[ceford]

Testing this - I must have done something wrong because I ran into a 500 error and reverting the patch manually did not fix it. After much messing about I now have to re-install my test site. In the logging code above $query does not exist in 5.0.0-beta2-dev. My current error starts with:

Exception:
Failed to start application

  at /Users/ceford/Sites/joomla-cms/libraries/src/Factory.php:158
  at Joomla\CMS\Factory::getApplication()
     (/Users/ceford/Sites/joomla-cms/libraries/src/Session/Storage/JoomlaStorage.php:240)

Test this at your peril!

---

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

[Denitz]

@ceford Please advise where did you get 5.0.0-beta2-dev? Can't find this branch.

[Quy]

@Denitz https://developer.joomla.org/nightly-builds.html

[ceford]

@ceford Please advise where did you get 5.0.0-beta2-dev? Can't find this branch.

It is what you see in the Admin Title bar when you clone the repo, checkout the 5 branch and follow the install procedure.

[Denitz]

Sorry, can't find the issue, merged 5.0-dev into my branch which is X years old.

[Denitz]

@ceford Please sorry for inconvenience, I have found the issue and updated the testing instructions.

[ceford]

I have tested this item :white_check_mark: successfully on 763261873b8606cd7499757be6ab98008ad4e38f

I see two fewer queries in the debug bar but four fewer files in my tmp folder. I guess that is OK.

---

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

[Denitz]

Yes, we have minus two useless SQL queries per each page load. The files in tmp folder are just for our testing purposes to see the real number of queries because Joomla debug is not able to log all database queries.

[HLeithner]

@wilsonge @laoneo do you object or have any input on this? (Database queries are expensive so if we can save one it would be good) but I'm not sure about other implications.

[Denitz]

@HLeithner We can save two queries.

[wilsonge]

Needs to be checked from the security implication. This comes from the 3.4.6 security patch when we were trying to protect against malicious session data being used through an old PHP vulnerability (given our minimum version being 7.x I would assume that's been patched out in PHP itself now. Ref: https://www.joomla.org/announcements/release-news/5642-important-security-announcement-pre-release-347.html).

Honestly given how long ago that was I don't remember whether the clone operation was super relevant for that fix or not maybe @SniperSister can remember.

[SniperSister]

Honestly given how long ago that was I don't remember whether the clone operation was super relevant for that fix or not

It's not, so the patch is fine from a security perspective

[HLeithner]

Then I merge this, thanks all