feat: initial commit to move laravel-cache to momentohq #1

poppoerika · 2022-11-02T04:21:53Z

Moved all the files from my personal repo.

Relevant ticket: https://github.com/momentohq/dev-eco-issue-tracker/issues/126

poppoerika · 2022-11-02T04:24:18Z

@cprice404 @pgautier404
I don't have admin rights to I'm not sure how other repos are set up in terms of rules.
If you can add these on the settings, that would be appreciated. Or, if you can let me know, I can do that as well 🙇‍♀️

pgautier404 · 2022-11-03T18:46:37Z

README.md

+{
+  "require": {
+    "momentohq/laravel-momento-cache": "0.0.1"
+  }
+}


Suggested change

{

"require": {

"momentohq/laravel-momento-cache": "0.0.1"

}

}

{

"repositories": [

{

"type": "vcs",

"url": "https://github.com/momentohq/laravel-cache"

}

],

"require": {

"momentohq/laravel-cache": "0.0.1"

}

}

composer.json

….lock

pgautier404 · 2022-11-03T21:32:23Z

README.md

+  "repositories": [
+    {
+      "type": "vcs",
+      "url": "https://github.com/momentohq/client-sdk-php"


The repository here needs to match the require below it:

Suggested change

"url": "https://github.com/momentohq/client-sdk-php"

"url": "https://github.com/momentohq/laravel-cache"

cprice404

looking really good! sorry it took me so long to review.

cprice404 · 2022-11-04T14:26:24Z

README.md

+<head>
+  <meta name="Momento Laravel cache driver" content="Taggable Momento serverless cache driver for Laravel">
+</head>
+<img src="https://docs.momentohq.com/img/logo.svg" alt="logo" width="400"/>


we should be able to use the README generator for this. As long as you set the "type" to "other" then it won't require all of the sections that an SDK README requires, but you will get all of the header and badge stuff w/o having to manage it by hand.

cprice404 · 2022-11-04T14:28:38Z

README.md

+        ],
+		// ...
+],
+```


I think we need to add a section about the support for tags here. Specifically, if a tag may contain more than ~4MB worth of keys, tagging may not work properly. If the user needs support for a larger tag set, reach out to us via support@momentohq.com or our discord and we can do some work to make it more flexible.

cprice404 · 2022-11-04T14:30:02Z

src/Cache/MomentoStore.php

+    public function flush()
+    {
+        throw new UnknownError("flush operations is currently not supported.");
+    }


We probably need to record this list of unsupported operations in the README as well.

cprice404 · 2022-11-04T14:31:11Z

src/Cache/MomentoStore.php

+
+    public function increment($key, $value = 1)
+    {
+        return false;


Why is this one returning false instead of erroring like the others?

Also, I wonder if we should go ahead and do a client-side implementation for putMany, increment, and decrement for now. We might want to have a zoom to discuss this.

It looks like increment is used when RouteServiceProvider is booting up in the weather app. So an error is thrown, I was not able to start the app.

okay then in that case let's do a client side implementation of this, where you read the key, if it doesn't exist set it to 0, then increment it, then save it.

cprice404 · 2022-11-04T14:31:22Z

src/Cache/MomentoStore.php

+
+    public function forget($key)
+    {
+        throw new UnknownError("forget operations is currently not supported.");


I believe that we can implement this one today?

cprice404 · 2022-11-04T14:32:28Z

src/Cache/MomentoServiceProvider.php

+
+use Illuminate\Support\ServiceProvider;
+
+class MomentoServiceProvider extends ServiceProvider


can you please create a ticket on the dev eco board to remind us to try to PR this directly into laravel once we have added the official support for increment/decrement/putMany in the future?

cprice404 · 2022-11-04T14:32:52Z

src/Cache/MomentoTaggedCache.php

+    {
+        $tags = $this->tags->getNames();
+        $cacheName = $this->store->getCacheName();
+        $value = "erika";


Yup. I noticed that too. I will update it 🙇‍♀️

cprice404 · 2022-11-04T14:33:19Z

src/Cache/MomentoTaggedCache.php

+        }
+        return $value;
+    }
+}


is there any way to add a few simple integration tests to this repo?

I wanted to add it in a follow-up PR if it's okay.

cprice404 · 2022-11-04T14:39:21Z

@cprice404 @pgautier404 I don't have admin rights to I'm not sure how other repos are set up in terms of rules. If you can add these on the settings, that would be appreciated. Or, if you can let me know, I can do that as well 🙇‍♀️

I added the branch protection rules for main. Thanks for the heads up. If we see anything else missing I can update further.

cprice404

i left a few nits for readme/github actions but if you prefer to merge and then do those in a follow-up that's okay. Also just verifying that you are planning to add some tests in a follow-up?

cprice404 · 2022-11-07T16:32:06Z

.github/workflows/on-pull-request.yaml

+          project_status: official
+          project_stability: alpha
+          project_type: other
+          sdk_language: PHP


nit: I don't think this field gets used for non-SDK artifacts. Might be more clear to remove it.

cprice404 · 2022-11-07T16:33:24Z

.github/workflows/on-push-to-release.yaml

+    steps:
+      - uses: actions/checkout@v3
+
+      - name: Verify README generation


I don't think we want to do readme generation on the release step, I think you can remove this.

This step is for verifying a generated README in the push to main, so we want to keep this to make sure the generated README is valid and follows the required rules? (Our PHP SDK has this step for the on-push-to-release workflow as well)

cprice404 · 2022-11-07T16:34:24Z

README.template.md

+  "repositories": [
+    {
+      "type": "vcs",
+      "url": "https://github.com/momentohq/laravel-cache"


will these be the correct directions for pulling from our release branch?

My understanding is that we specify which version/branch to pull in require property like the following:

"require": { "momentohq/client-sdk-php": "0.2.0" },

And 0.2.0 is created by pushing to our release branch, so I believe this is pulling from our release branch.
@pgautier404 Please correct me if I'm mistaken.

@poppoerika I think the initial tag for laravel-cache is going to be 0.1.0 instead of 0.0.1.

cprice404 · 2022-11-07T16:35:17Z

README.template.md

+
+Check out full working code in [the example app](https://github.com/momentohq/laravel-example)!
+
+### Error Handling


You can remove these sections. They are not required for non-sdk projects.

cprice404 · 2022-11-07T16:36:39Z

README.template.md

+
+### Unsupported Cache Operations
+
+The following cache operations are not supported today:


can you add a bit more verbiage here and say something like: "if you need these operations, please reach out to us and we can prioritize the work to complete them. you can file a github issue, e-mail us at support@momentohq.com, or join our discord (link to discord)

also, can you file a github issue for us to come back and implement these? You can add the "PHP ADS 1.0" milestone to it.

Roger to both.

cprice404 · 2022-11-07T16:40:03Z

src/MomentoTaggedCache.php

+            $keys = $this->store->setFetch($cacheName, $tag);
+            foreach ($keys as $k) {
+                if ($k == $key) {
+                    $value = $this->store->get($key);


this looks to me like it would just overwrite the $value every time? And so we would end up just returning the last key that matched? but maybe there is some PHP nuance here that I am missing.

Maybe I'm misunderstanding how tagged cache get should work here...
When Cache::tags(['weather', 'seattle'])->get($cityWeatherIdInfo); is called, only the value for the key tagged with both weather and seattle needs to be returned?
In other word, if there is a key only tagged with weather, we should return null as its value?
Or as long as one of the tags are tagged to a key, we can return the value for that key?

But even with my current understanding, I need to update this for loop so that null will be assigned when $k != $key.

oh wow, yeah I think your understanding was closer than mine:

Cache tags allow you to tag related items in the cache and then flush all cached values that have been assigned a given tag. You may access a tagged cache by passing in an ordered array of tag names. Items stored via tags may not be accessed without also providing the tags that were used to store the value. For example, let's access a tagged cache and put a value into the cache:

So it should indeed only be returning one item, you are correct. But in that case you can break out of the loop at the point where you find a match.

We might need to think a bit more about the implementation for this. It will be easier to do when we have a way to write tests. So I think I will just give a thumbs up on this for now and we may need to re-work it once you are able to write tests.

poppoerika · 2022-11-07T17:29:02Z

@cprice404
I addressed all the comments above, and would like to resolve before merging this.
Yes, I'm going to add tests in a follow-up PR. This is the ticket to track: #2

pgautier404 · 2022-11-07T17:41:00Z

src/MomentoServiceProvider.php

+        app('cache')->extend('momento', function ($app, $config) {
+            $store = new MomentoStore($config['cache_name'], $config['default_ttl']);
+            return app('cache')->repository($store);


Something about this doesn't look right to me (and my IDE). I don't think app is a function in scope here, and I'm afraid this code may not be getting called during the bootstrap process. I can't get the end-to-end Laravel example app to install for me, so I can't really troubleshoot much deeper, but please double check that this function is working correctly.

cprice404

i'm good with this going in and we can address outstanding items in follow-up, assuming @pgautier404 is also good with it

feat: initial commit to move laravel-cache to momentohq

23fe4e7

poppoerika requested review from cprice404 and pgautier404 November 2, 2022 04:22

poppoerika mentioned this pull request Nov 2, 2022

feat: move files from personal repo to momentohq momentohq/laravel-example#1

Merged

poppoerika added 2 commits November 2, 2022 15:50

chore: remove authors

103442e

fix: update package name to match repo name

22ad3a4

pgautier404 reviewed Nov 3, 2022

View reviewed changes

composer.json Show resolved Hide resolved

poppoerika added 2 commits November 3, 2022 12:08

fix: add repositories for client-sdk-php; update README; add composer…

8db8115

….lock

chore: update README to include repository

667f6cc

pgautier404 reviewed Nov 3, 2022

View reviewed changes

fix: update README repository

baeeb09

poppoerika requested a review from pgautier404 November 3, 2022 23:00

cprice404 reviewed Nov 4, 2022

View reviewed changes

poppoerika added 3 commits November 4, 2022 09:54

chore: add github workflow

ea5dbbb

chore: address Chris feedback

1d73415

chore: remove README

b3bc4f3

poppoerika requested a review from cprice404 November 4, 2022 17:23

poppoerika added 5 commits November 4, 2022 12:38

chore: implement increment

d52e6e5

fix: pass cache name to get operation

39020a0

chore: add package dicovery

9e8e8d9

chore: restructure laravel cache

900e40c

fix: update autoload

5ab2b1c

cprice404 reviewed Nov 7, 2022

View reviewed changes

poppoerika added 3 commits November 7, 2022 09:08

chore: remove sdk type from GitHub workflows

081aabc

chore: remove Error Handling section from README

5ee556b

chore: add more details to non-supported operations

2d2680f

chore: update release version in README template

6577d7c

pgautier404 reviewed Nov 7, 2022

View reviewed changes

cprice404 previously approved these changes Nov 7, 2022

View reviewed changes

chore: add lock file

796813f

poppoerika dismissed cprice404’s stale review via 796813f November 7, 2022 17:57

pgautier404 approved these changes Nov 7, 2022

View reviewed changes

poppoerika merged commit 8aeea2c into main Nov 7, 2022

poppoerika deleted the feat/add-files-for-cache branch November 7, 2022 22:35

	"url": "https://github.com/momentohq/client-sdk-php"
	"url": "https://github.com/momentohq/laravel-cache"


		use Illuminate\Support\ServiceProvider;

		class MomentoServiceProvider extends ServiceProvider


		Check out full working code in [the example app](https://github.com/momentohq/laravel-example)!

		### Error Handling


		### Unsupported Cache Operations

		The following cache operations are not supported today:

feat: initial commit to move laravel-cache to momentohq #1

feat: initial commit to move laravel-cache to momentohq #1

Conversation

poppoerika commented Nov 2, 2022

poppoerika commented Nov 2, 2022

Choose a reason for hiding this comment

pgautier404 Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

cprice404 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cprice404 commented Nov 4, 2022

cprice404 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poppoerika commented Nov 7, 2022

Choose a reason for hiding this comment

cprice404 left a comment

Choose a reason for hiding this comment

pgautier404 Nov 3, 2022 •

edited

Loading