pressflow uses what is known as lazy session loading for anonymous users. basicly sessions don't exist before information is writen to them. as such. session_encode and session_destroy won't work as there is no session unless data was writen to them. causing these two errors in the log
session_encode() [<a href='function.session-encode'>function.session-encode</a>]: Cannot encode non-existent session. in /home/aberdeen/public_html/sites/all/modules/services/services.module on line 837.
and
session_destroy() [<a href='function.session-destroy'>function.session-destroy</a>]: Trying to destroy uninitialized session in /home/aberdeen/public_html/sites/all/modules/services/services/user_service/user_service.inc on line 124.
I know that pressflow possibly is not high on the list of systems to write for as it is a drupal derivative, and not core. I personally find these error messages annoying [they don't break anything tho]

CommentFileSizeAuthor
#1 services_pressflow.patch1.13 KBkatbailey

Comments

katbailey’s picture

Status: Active » Needs review
StatusFileSize
new1.13 KB

Seeing as the only problem here seems to be the littering of the logs with the above error messages (as I can attest to with my own environment), it should suffice to wrap the call to session_encode() in an if statement that just checks that $_SESSION is not empty... patch attached.

marcus_w’s picture

Status: Needs review » Reviewed & tested by the community

I confirm this path works correctly, it has to be implemented at user_service.inc on line 124 too though. The user.logout function gave me a session_destroy() error.

voxpelli’s picture

Status: Reviewed & tested by the community » Needs work

Wouldn't you need to do something about when the backup us used as well?

whytewolf’s picture

@voxpelli not really, the backup is only used to restore the session, and user, the calls used to do so are benign when used with null data. see services.module lines 870-894

function services_session_unload($backup) {
  global $user;

  // No point in reverting if it's the same user's data
  if ($user->sid == $backup->sid) {
    return;
  }

  // Some client/servers, like XMLRPC, do not handle cookies, so imitate it to make sess_read() function try to look for user,
  // instead of just loading anonymous user :).
  if (!isset($_COOKIE[session_name()])) $_COOKIE[session_name()] = $backup->sessid;

  // Save current session data
  sess_write($user->sid, session_encode());

  // Empty current session data
  $_SESSION = array();

  // Revert to previous user and session data
  $user = $backup;
  session_id($backup->sessid);
  session_decode($user->session);

  session_save_session(TRUE);
}
voxpelli’s picture

But it's called whenever a session couldn't be successfully loaded - right? And that is equally as possible with Pressflow as without - isn't it? And in Pressflow it then tries to restore a session that doesn't exist - seems like that needs to be fixes as well?

Try a call with an invalid session id - that should trigger a backup to be restored.

whytewolf’s picture

there is no argument that the backup is triggered, but there is no need to change the way the backup is restored, the change made in the patch doesn't completely remove the backup it just removes the session from the backup when there isn't a session to backup. when the backup is restored it will set the session to the same state it was when the backup was created. which is non-existent.
which is planned behavior,

session_decode does not throw an error when null is passed to it.

claudiu.cristea’s picture

Priority: Minor » Normal
Status: Needs work » Reviewed & tested by the community

The patch from #1 fixes the issue on Pressflow.

kylebrowning’s picture

Assigned: Unassigned » gdd

Im not really against this at all and willing to commit it, heyrocker, thoughts?

marcingy’s picture

Version: 6.x-2.2 » 6.x-2.x-dev
Category: support » feature

Reclassifying I'm happy for this to go in but I'm stil having issues with git, so Kyle if tou want to commit it I'm more than happy.

marcingy’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

2.x is no longer supported