Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates to FirestoreSessionHandler #2360

Merged
merged 6 commits into from
Oct 2, 2019

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Oct 1, 2019

There are still a few issues:

  • session_gc returns false, even though the records get deleted as expected. I do not know why.
  • The FirestoreTestCase function to delete $deletionQueue does not get registered. This is because the call do do this, which is in bootstrap.php, does not exist in system-bootstrap.php.
  • Even when the deletion function is registered, the collections registered to be deleted in these tests are not deleted. I suspect it has something to do with them running in separate processes but I can't figure it out.

@bshaffer bshaffer requested a review from jdpedrie as a code owner October 1, 2019 00:41
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 1, 2019
@jdpedrie jdpedrie added the api: firestore Issues related to the Firestore API. label Oct 1, 2019
Copy link
Contributor

@jdpedrie jdpedrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FirestoreTestCase function to delete $deletionQueue does not get registered. This is because the call do do this, which is in bootstrap.php, does not exist in system-bootstrap.php.

This is probably why so much junk piles up in the project! I'll look into that.

Firestore/src/FirestoreSessionHandler.php Outdated Show resolved Hide resolved
@bshaffer
Copy link
Contributor Author

bshaffer commented Oct 1, 2019

Remaining outstanding issues:

  • session_gc mysteriously returns false
  • $localDeletionQueue is not working

@codecov
Copy link

codecov bot commented Oct 2, 2019

Codecov Report

Merging #2360 into master will increase coverage by 0.01%.
The diff coverage is 95.74%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2360      +/-   ##
============================================
+ Coverage     92.53%   92.55%   +0.01%     
- Complexity     4501     4503       +2     
============================================
  Files           312      312              
  Lines         13479    13497      +18     
============================================
+ Hits          12473    12492      +19     
+ Misses         1006     1005       -1
Impacted Files Coverage Δ Complexity Δ
Firestore/src/FirestoreClient.php 100% <ø> (ø) 21 <0> (ø) ⬇️
Firestore/src/FirestoreSessionHandler.php 97.56% <95.74%> (+0.19%) 22 <3> (+1) ⬆️
PubSub/src/Subscription.php 100% <0%> (ø) 40% <0%> (ø) ⬇️
Spanner/src/Connection/Grpc.php 94.35% <0%> (+0.11%) 95% <0%> (+1%) ⬆️
Spanner/src/Session/CacheSessionPool.php 97.84% <0%> (+0.35%) 88% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfc7b37...c9758ef. Read the comment docs.

@dwsupplee dwsupplee changed the title calls open after running any Firestore close operation Updates to FirestoreSessionHandler Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants