Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Misc. fixes in database session storage#13075

New issue

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

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
mbabker wants to merge1 commit intojoomla:stagingfrommbabker:database-session-store-fixes
Closed

Misc. fixes in database session storage#13075

mbabker wants to merge1 commit intojoomla:stagingfrommbabker:database-session-store-fixes

Conversation

@mbabker
Copy link
Contributor

@mbabkermbabker commentedDec 3, 2016•
edited by joomla-cms-bot
Loading

Summary of Changes

This is only the changes for the database session storage from#10905

  • Right now,JSessionStorageDatabase::write() depends on the application having inserted a record into the#__session database table to work correctly, the method is changed to query for the presence of the record first and to be able to insert it if not already present
  • Garbage collection in PHP's session module happens when the session is started. Instead of running this cleanup operation at the beginning of the request cycle, we'll catch that garbage collection has been requested and run this when the session is closed at the end of the request cycle. This defers the cleanup operation to a point after the user should have gotten the HTML response instead of this cleanup being a blocking operation (at least for the default database handler).
  • Try to explicitly initialize the database connection when the session is opened instead of waiting for the connection to be lazy started; the only practical benefit here is if there is a database connection issue the error is raised sooner
  • A session handler'sread() method should always return a string, so if an exception is caught inJSessionStorageDatabase::read() we'll return an empty string instead of a boolean false now

Testing Instructions

Sessions using the database storage should continue to work correctly

Documentation Changes Required

N/A

@dgrammatiko
Copy link
Contributor

@mbabker is there any noticeable improvement in the response times?

@mbabker
Copy link
ContributorAuthor

TBH, probably not. The one thing that might make a difference is moving garbage collection to a point after the HTML response should have been sent, but really its only a single DELETE query we're talking about here.

Symfony does this in part (seesymfony/symfony#10908) to allow their handler to use locking mechanisms and transactions. So it honestly doesn't hurt us much, but at the same time it's probably not extremely beneficial either.

@andrepereiradasilva
Copy link
Contributor

with this patch applied when using shared sessions i cannot logout

@andrepereiradasilva
Copy link
Contributor

@mbabker any idea why i could'nt log out in shared sessions afte this patch? (add to manually delete the cookie to logout)

@mbabker
Copy link
ContributorAuthor

Not a clue. Nothing in the database session handler is client aware, including shared sessions.

@mbabker
Copy link
ContributorAuthor

Found it. The issue is unrelated to this patch.

@mbabker
Copy link
ContributorAuthor

See#13273

The branch athttps://github.com/mbabker/joomla-cms/tree/logout-database-sessions combines these two PRs for convenience.

@andrepereiradasilva
Copy link
Contributor

I have tested this item ✅ successfully on939278c

Tested both#13273 and#13075 applied in the patchtester.
Tested with shared and non shared session both login and logout from admin and site client
all worked fine. so works as described

This comment was created with theJ!Tracker Application atissues.joomla.org/tracker/joomla-cms/13075.

@csthomas
Copy link
Contributor

I have tested this item ✅ successfully on939278c

The same as above.
Tested both#13273 and#13075 applied in the patchtester.
Tested with shared and non shared session both login and logout from admin and site client
all worked fine.

This comment was created with theJ!Tracker Application atissues.joomla.org/tracker/joomla-cms/13075.

@zero-24zero-24 added this to theJoomla 3.7.0 milestoneDec 20, 2016
@zero-24zero-24 added the RTCThis Pull Request is Ready To Commit labelDec 20, 2016
@csthomas
Copy link
Contributor

csthomas commentedDec 21, 2016•
edited
Loading

One warning.
When I have applied this PR then on backend "Logged-in Users" module does not work.
Session on database, not shared and shared session.

Can anybody confirm?

@zero-24
Copy link
Contributor

When I have applied this PR then on backend "Logged-in Users" module does not work.
Session on database, not shared and shared session.

Can you double check that you have the very last chnages from staging?

@csthomas
Copy link
Contributor

I have checked again and the same result.
Applied by patch tester and by git as:
curl https://patch-diff.githubusercontent.com/raw/joomla/joomla-cms/pull/13075.diff | git apply

After I applied then Logged Users module is empty after relogin as admin.
When I do checkout and relogin then module show me again.

joomla_3.7-dev

@csthomas
Copy link
Contributor

With this PR in table#__sessions username and guest column is always empty.

@rdeutz
Copy link
Contributor

I have tested this item 🔴 unsuccessfully on939278c

Same result as@csthomas after logout and login the "LOGGED-IN USERS" List is empty

This comment was created with theJ!Tracker Application atissues.joomla.org/tracker/joomla-cms/13075.

@rdeutzrdeutz removed the RTCThis Pull Request is Ready To Commit labelJan 4, 2017
@mbabker
Copy link
ContributorAuthor

I can't say I care enough to dig into why this issue is occurring because zero changes are made anywhere which can cause it. So, sorry for wasting time.

@rdeutz
Copy link
Contributor

This is the reason why we having tests, to find the issues that can't occurring because nothing is changed

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Joomla 3.7.0

Development

Successfully merging this pull request may close these issues.

7 participants

@mbabker@dgrammatiko@andrepereiradasilva@csthomas@zero-24@rdeutz@joomla-cms-bot
[8]ページ先頭
©2009-2025 Movatter.jp