Skip to content

[4] Cast session_id to string before destroying it#33787

Closed
PhilETaylor wants to merge 1 commit intojoomla:4.0-devfrom
PhilETaylor:fixsessionidtostring
Closed

[4] Cast session_id to string before destroying it#33787
PhilETaylor wants to merge 1 commit intojoomla:4.0-devfrom
PhilETaylor:fixsessionidtostring

Conversation

@PhilETaylor
Copy link
Copy Markdown
Contributor

Pull Request for Issue #33740

Summary of Changes

Cast session_id to a string before we use it

Testing Instructions

#33740

Actual result BEFORE applying this Pull Request

Cant logout under some circumstances with pgsql

Expected result AFTER applying this Pull Request

Can logout with no errors under some circumstances with pgsql

Documentation Changes Required

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Copy Markdown
Member

@PhilETaylor Sure that is the solution? When I add a error_log((string) $sessionId); just before the changed line and have session handler set to filesystem so I can reproduce the problem, I get in the error log: Resource id #386.

With the code change I had proposed in the issue, I did get the correct session ID in the log.

@PhilETaylor

This comment was marked as abuse.

@richard67
Copy link
Copy Markdown
Member

Well it doesn't cause that type error. But does it really destroy the session?

@richard67
Copy link
Copy Markdown
Member

richard67 commented May 11, 2021

With my code, the guest session was removed after login, with the code proposed here it isn't.

Update: I mean the session records in database when collection session metadata is switched on and the session handler is file system.

@joomdonation
Copy link
Copy Markdown
Contributor

I think this PR does not work. See https://www.php.net/manual/en/language.types.string.php#language.types.string.casting

Resources are always converted to strings with the structure "Resource id #1", where 1 is the resource number assigned to the resource by PHP at runtime. While the exact structure of this string should not be relied on and is subject to change, it will always be unique for a given resource within the lifetime of a script being executed (ie a Web request or CLI process) and won't be reused

So from what I see, you don't see the PHP error because the code successfully convert the data to string, however, it return wrong Session ID as you can guess.

After some Google Search, I think we might have to accept the ugly code #33740 (comment) and move on.

@richard67
Copy link
Copy Markdown
Member

@PhilETaylor We cast stuff (e.g. objects) to strings here and there, but on a quick search yesterday I haven't found any place where we do that for resources. If you find such a place, please report it with a new issue because that would be a bug, as far as I understand PHP docs.

@richard67
Copy link
Copy Markdown
Member

Alternative PR see #33819 .

@rdeutz rdeutz changed the title [4][Release Blocker] Cast session_id to string before destorying it [4] Cast session_id to string before destorying it May 12, 2021
@brianteeman
Copy link
Copy Markdown
Contributor

Can you update the title please. We should use tags to indicate release blocker

@richard67 richard67 changed the title [4] Cast session_id to string before destorying it [4] Cast session_id to string before destroying it May 12, 2021
@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

@PhilETaylor

This comment was marked as abuse.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants