kodeine / laravel-meta

Fluent Meta Data for Eloquent Models, as if it is a property on your model
MIT License
400 stars 90 forks source link

[suggestion] make new version to only support recent laravel versions #94

Closed siamak2 closed 2 years ago

siamak2 commented 2 years ago

Hi @kodeine right now laravel-meta supports laravel 5.0 to 9.x but older versions are no longer supported by laravel. and kinda it's hard to add backward compatible features to laravel meta. one of these features is the new laravel Attribute accessor and mutator that only available on laravel 8.x and 9.x. my suggestion is that you create a new branch called 2.x or whatever you like and only support laravel 8.x, 9.x and of course the incoming versions. I'm willing to make those changes and submit PR. however I have these changes in mind to add in laravel-meta:

  1. remove legacy getter and change parts of code that depend on this https://github.com/kodeine/laravel-meta/blob/15961bf2553d00c9103dd76547cb0217863b3252/src/Kodeine/Metable/Metable.php#L379-L380
  2. make saveMeta mthod public.
  3. add events for metas like metaSaving, metaSaved, metaCreated and more.

what's your opinion about these changes?

siamak2 commented 2 years ago

I made some of the changes I mentioned in my forked repo. check it out: https://github.com/siamak2/laravel-meta/tree/2.x I didn't submit PR because:

  1. it still needs some changes and haven't added the events yet
  2. it's better if you create another branch and I submit PR to that branch
siamak2 commented 2 years ago

I was reading README and I noticed this: https://github.com/kodeine/laravel-meta/blob/15961bf2553d00c9103dd76547cb0217863b3252/README.md?plain=1#L239-L243

But second parameter of getMeta is called $raw and treated as boolean. I wonder which one I should correct. README or the method?

kodeine commented 2 years ago

@siamak2 in regards to getMeta() second param should be a default value like in readme. lets create a 2.0 branch will become master as well and we will move current master as 1.0. The suggestions you mentioned are good, please submit the PR for it on 2.0 branch.

Thank you for your time and efforts in doing this.

siamak2 commented 2 years ago

still one branch is available. maybe you forgot to push the new branch? I believe that you shouldn't rename master branch. according to readme many have added the master branch to their project: "kodeine/laravel-meta": "master"

and depend on that branch. since my PR is gonna have some backward incompatible changes, if any one runs composer update it will likely break their app. I will write upgrade guide in readme and also mention the backward incompatible changes.

in regards to getMeta() second param should be a default value like in readme.

OK I will change the code.

My changes also fixes some of the open issues. I'll mention them in my PR to be auto closed after merge

siamak2 commented 2 years ago

@kodeine I added everything exept for the events. I'll add events later. I added changelog file. I think it's better if we add changes we make to that file Now all I need is the new branch so I can submit PR to that branch

kodeine commented 2 years ago

@siamak2 first of all, thank you very much for your contribution, really appreciate it.

I have pushed the branch 2.0 so you can make changes there.

Sure, ill keep master as is.

siamak2 commented 2 years ago

@kodeine In order to my pull request auto close issues I mention, branch 2.0 need to be default. if it's not default then github won't auto close issues. You change default branch in repository setting -> Branches from left menu. I'll submit PR in a few minutes.

siamak2 commented 2 years ago

I submitted the PR https://github.com/kodeine/laravel-meta/pull/95

kodeine commented 2 years ago

@siamak2 thanks a lot!, i've merged it, made default branch as 2.0.