Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Removed bug producing insert from JSessionStorageDatabase::write() #454

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@ghost

ghost commented Oct 21, 2011

If update results in an error[such as a non existant row] then write will already return false. If however it succeeds but is identical to the current data, affectedRows will be 0, which then invokes insert while will ALWAYS fail because it is a duplicate row.

An insert is already performed when the session is created in JSession, there is no need for this one and it can only result in an error.

@LouisLandry LouisLandry commented on the diff Oct 22, 2011

libraries/joomla/session/storage/database.php
@@ -117,13 +117,16 @@ class JSessionStorageDatabase extends JSessionStorage
}
else
{
- // If the session does not exist, we need to insert the session.
- $db->setQuery(
- 'INSERT INTO ' . $db->quoteName('#__session') .
- ' (' . $db->quoteName('session_id') . ', ' . $db->quoteName('data') . ', ' . $db->quoteName('time') . ')' .
- ' VALUES (' . $db->quote($id) . ', ' . $db->quote($data) . ', ' . (int) time() . ')'
- );
- return (boolean) $db->query();
+ // update did not error, so it succeeded but was identical
+ if (JDEBUG) {
+ jimport('joomla.error.log');
@LouisLandry

LouisLandry Oct 22, 2011

Contributor

This is deprecated usage of JLog. You'll want to have a look at JPATH_PLATFORM/joomla/log/log.php. The syntax is something like:

<?php
JLog::add(sprintf('Session ID %s update is identical to current record', $id));
Member

elkuku commented Nov 2, 2011

Checkstyle error details:
libraries/joomla/session/storage/database.php:120, 121
Tabs must be used to indent lines; spaces are not allowed

libraries/joomla/session/storage/database.php:121
Expected "if (...)\n"; found "if (...) "

libraries/joomla/session/storage/database.php:122 - 129
Tabs must be used to indent lines; spaces are not allowed

Build triggered by changes to the base.

Test log missing. Tests failed to execute.
Checkstyle analysis reported 233 warnings and 4165 errors.

Build triggered by changes to the base.

Test log missing. Tests failed to execute.
Checkstyle analysis reported 235 warnings and 11 errors.

Contributor

eddieajau commented Dec 11, 2011

Any followup on this issue?

Contributor

chdemko commented Feb 22, 2012

@garyamort The pull request cannot be merged

Contributor

AmyStephen commented May 8, 2012

@garyamort - It would be good to get this fix in-it continues to be reported as an error, especially on high-volume sites.

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=28510

http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=28249

@ghost

ghost commented May 15, 2012

hmm, thought this was fixed in another pull request... Elin reminded me it's been sitting here. Ok, I';ll have to pull the latest code and redo the update, this time without using depreciated JLog.

@ghost

ghost commented May 15, 2012

I re-arranged my repos into orgs for my own clarity and this branch was lost. I've recreated the fix and submitted it as pull request 1209.
#1209

@ghost ghost closed this May 15, 2012

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment