feat: Add Firestore session handler. #2258

bshaffer · 2019-08-20T23:02:01Z

No description provided.

codecov · 2019-08-20T23:17:27Z

Codecov Report

Merging #2258 into master will decrease coverage by 0.07%.
The diff coverage is 97.52%.

@@             Coverage Diff              @@
##             master    #2258      +/-   ##
============================================
- Coverage      92.6%   92.53%   -0.08%     
- Complexity     4467     4501      +34     
============================================
  Files           307      312       +5     
  Lines         13323    13479     +156     
============================================
+ Hits          12338    12473     +135     
- Misses          985     1006      +21

Impacted Files	Coverage Δ	Complexity Δ
Firestore/src/Query.php	`99.2% <ø> (ø)`	`86 <0> (ø)`	⬇️
Firestore/src/FirestoreClient.php	`100% <100%> (ø)`	`21 <1> (+1)`	⬆️
Firestore/src/FirestoreSessionHandler.php	`97.36% <97.36%> (ø)`	`21 <21> (?)`
Core/src/Logger/AppEngineFlexHandler.php	`0% <0%> (-83.34%)`	`3% <0%> (ø)`
Core/src/Logger/AppEngineFlexFormatter.php	`0% <0%> (-77.78%)`	`2% <0%> (-1%)`
BigQuery/src/ExtractJobConfiguration.php	`100% <0%> (ø)`	`8% <0%> (+1%)`	⬆️
Spanner/src/Transaction.php	`100% <0%> (ø)`	`24% <0%> (ø)`	⬇️
BigQuery/src/LoadJobConfiguration.php	`100% <0%> (ø)`	`24% <0%> (+1%)`	⬆️
Core/src/Logger/FormatterTrait.php	`100% <0%> (ø)`	`2% <0%> (?)`
Core/src/Logger/AppEngineFlexHandlerV2.php	`100% <0%> (ø)`	`3% <0%> (?)`
... and 3 more

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 92a5dd0...da89a0a. Read the comment docs.

Firestore/src/FirestoreSessionHandler.php

Firestore/tests/Unit/FirestoreSessionHandlerTest.php

Co-Authored-By: John Pedrie <[email protected]>

googlebot · 2019-08-28T17:25:02Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-28T17:26:35Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

jdpedrie · 2019-08-28T17:27:46Z

@bshaffer I added some snippet and system tests to this based off the work I merged in #2266, and rebased against latest master. I have my work off on my fork as well, so if I have broken your intent in any way, feel free to force push again. ;)

googlebot · 2019-08-28T17:40:45Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

jdpedrie

looks good! stoked you solved the transaction problem. :)

Firestore/src/FirestoreSessionHandler.php

jdpedrie

surface lgtm. couple minor test suggestions.

Firestore/tests/System/FirestoreSessionHandlerTest.php

Firestore/tests/Unit/FirestoreSessionHandlerTest.php

bshaffer · 2019-09-24T15:45:21Z

When I used $this instead of $phpunit, the tests fail with method not found exceptions. Looks like $this is being assigned to the Connection class, so the phpunit methods are not available in that context.

jdpedrie · 2019-09-24T15:53:52Z

so the phpunit methods are not available in that context.

Apologies for steering you wrong!

dwsupplee

One small question, outside of that looks great :)

dwsupplee · 2019-09-24T17:02:34Z

Firestore/src/FirestoreSessionHandler.php

+        $this->sessionName = $sessionName;
+        $database = $this->databaseName($this->projectId, $this->database);
+
+        $beginTransaction = $this->connection->beginTransaction([


Should we wrap this in a try/catch block as well?

If we do this, open will return false instead of throwing an exception if it fails. This seems consistent and correct to me.

googlebot · 2019-09-24T19:21:30Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

bshaffer requested a review from jdpedrie as a code owner August 20, 2019 23:02

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 20, 2019

bshaffer mentioned this pull request Aug 20, 2019

Getting Started: Sessions GoogleCloudPlatform/getting-started-php#209

Merged

jdpedrie reviewed Aug 21, 2019

View reviewed changes

Firestore/src/FirestoreSessionHandler.php Outdated Show resolved Hide resolved

Firestore/src/FirestoreSessionHandler.php Outdated Show resolved Hide resolved

Firestore/src/FirestoreSessionHandler.php Outdated Show resolved Hide resolved

jdpedrie changed the title ~~Adds Firestore session handler~~ feat: Add Firestore session handler Aug 21, 2019

jdpedrie changed the title ~~feat: Add Firestore session handler~~ feat: Add Firestore session handler. Aug 21, 2019

jdpedrie previously requested changes Aug 23, 2019

View reviewed changes

Firestore/src/FirestoreSessionHandler.php Outdated Show resolved Hide resolved

jdpedrie reviewed Aug 23, 2019

View reviewed changes

Firestore/tests/Unit/FirestoreSessionHandlerTest.php Outdated Show resolved Hide resolved

bshaffer and others added 5 commits August 28, 2019 13:24

adds Firestore session handler

2efb494

Update Firestore/src/FirestoreSessionHandler.php

e690911

Co-Authored-By: John Pedrie <[email protected]>

wraps writes in transactions

2ad13e2

removes collection name validation, mocks runTransaction better

d5212d1

Add snippet and system tests

9166018

jdpedrie force-pushed the firestore-session-handler branch from 5bd5dba to 9166018 Compare August 28, 2019 17:24

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 28, 2019

remove extraneous doc lines

d4249e6

dwsupplee added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 28, 2019

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Aug 28, 2019

dwsupplee added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Aug 28, 2019

Skip tests if grpc missing

dc26d83

googlebot removed the cla: yes This human has signed the Contributor License Agreement. label Aug 28, 2019

bshaffer added 2 commits September 23, 2019 12:38

adds options comment, adds collectionNameTemplate option

9065fc2

removes redundant optional identifier

e730412

jdpedrie suggested changes Sep 23, 2019

View reviewed changes

addresses review comments

36de91a

jdpedrie suggested changes Sep 24, 2019

View reviewed changes

addresses more review comments

d9190ba

adds back variable to callables

b5cff2d

jdpedrie approved these changes Sep 24, 2019

View reviewed changes

dwsupplee reviewed Sep 24, 2019

View reviewed changes

adds try/catch around beginTransaction

da89a0a

dwsupplee approved these changes Sep 24, 2019

View reviewed changes

dwsupplee added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Sep 24, 2019

dwsupplee merged commit f5f006b into master Sep 24, 2019

bshaffer deleted the firestore-session-handler branch October 1, 2019 00:37

yoshi-automation mentioned this pull request Feb 7, 2020

feat!: rename formatting method and update listGroupStats signature #2677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Firestore session handler. #2258

feat: Add Firestore session handler. #2258

bshaffer commented Aug 20, 2019

codecov bot commented Aug 20, 2019 •

edited

Loading

googlebot commented Aug 28, 2019

googlebot commented Aug 28, 2019

jdpedrie commented Aug 28, 2019

googlebot commented Aug 28, 2019

jdpedrie left a comment

jdpedrie left a comment

bshaffer commented Sep 24, 2019

jdpedrie commented Sep 24, 2019

dwsupplee left a comment

dwsupplee Sep 24, 2019

bshaffer Sep 24, 2019

googlebot commented Sep 24, 2019

feat: Add Firestore session handler. #2258

feat: Add Firestore session handler. #2258

Conversation

bshaffer commented Aug 20, 2019

codecov bot commented Aug 20, 2019 • edited Loading

Codecov Report

googlebot commented Aug 28, 2019

googlebot commented Aug 28, 2019

jdpedrie commented Aug 28, 2019

googlebot commented Aug 28, 2019

jdpedrie left a comment

Choose a reason for hiding this comment

jdpedrie left a comment

Choose a reason for hiding this comment

bshaffer commented Sep 24, 2019

jdpedrie commented Sep 24, 2019

dwsupplee left a comment

Choose a reason for hiding this comment

dwsupplee Sep 24, 2019

Choose a reason for hiding this comment

bshaffer Sep 24, 2019

Choose a reason for hiding this comment

googlebot commented Sep 24, 2019

codecov bot commented Aug 20, 2019 •

edited

Loading