Skip to content

[4.0] sessionID & pgsql#33817

Closed
alikon wants to merge 5 commits intojoomla:4.0-devfrom
alikon:patch-81
Closed

[4.0] sessionID & pgsql#33817
alikon wants to merge 5 commits intojoomla:4.0-devfrom
alikon:patch-81

Conversation

@alikon
Copy link
Copy Markdown
Contributor

@alikon alikon commented May 12, 2021

Pull Request for Issue #33740 . alternative to #33787

Summary of Changes

Handle resources

Testing Instructions

see #33740
use postgresql
change session to file
login/logout

Actual result BEFORE applying this Pull Request

error

Expected result AFTER applying this Pull Request

logout successfully

@alikon alikon mentioned this pull request May 12, 2021
Closed
richard67
richard67 reviewed May 12, 2021
View reviewed changes
@alikon @richard67
Update plugins/user/joomla/joomla.php
b2849e9 
Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@richard67 richard67 mentioned this pull request May 12, 2021
Closed
@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

Alternative PR: #33819 .

@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

Added release blocker label as inherited from the issue.

@joomdonation
Copy link
Copy Markdown
Contributor

joomdonation commented May 12, 2021

@alikon Should we close this in favor of #33819 ?

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 12, 2021 

i would like to avoid to touch the session lib, as this issue is not related to the session, but feel free to close it

2 similar comments
@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 12, 2021 

i would like to avoid to touch the session lib, as this issue is not related to the session, but feel free to close it

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 12, 2021 

i would like to avoid to touch the session lib, as this issue is not related to the session, but feel free to close it

@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

@alikon If we want to do it your way with this PR, you have to do the same here, too:

joomla-cms/plugins/user/joomla/joomla.php

Line 134 in b2849e9

if (!$sessionManager->destroySessions($sessionIds))

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 12, 2021 via email

@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

@alikon Both are not nice and I understand what you mean. But as I said, please add the same change at the other place I've mentioned because we have the same issue there, reading the session id with loadColumn and so on.

@PhilETaylor

This comment was marked as abuse.

Sign in to view
@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

@alikon If you merge @PhilETaylor 's PR alikon#71 , I'll close my PR in favour of this one.

@joomdonation
Copy link
Copy Markdown
Contributor

joomdonation commented May 12, 2021

I still prefer doing it in a central place but guess we will have to respect API design.

@alikon
Merge pull request #71 from PhilETaylor/also
a6536fd 
add additional place and redocument
@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 12, 2021

i don't know after this commit f8445ef
what is the current situation

@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

I've closed my PR now in favour of this one.

@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

I have tested this item ✅ successfully on a6536fd

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33817.

@nibra nibra mentioned this pull request May 12, 2021
Merged
@richard67
Copy link
Copy Markdown
Member

richard67 commented May 12, 2021

@alikon Meanwhile I favour @nibra 's new PR #33822 . Please check if you come to the same conclusion.

@alikon
Copy link
Copy Markdown
Contributor Author

alikon commented May 12, 2021

closed in favour of #33822

@alikon alikon closed this May 12, 2021
@alikon alikon deleted the patch-81 branch May 12, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richard67 richard67 richard67 left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alikon @richard67 @joomdonation @PhilETaylor @joomla-cms-bot