czim / laravel-repository

Repository pattern implementation for Laravel
MIT License
51 stars 19 forks source link

Bump to php 7.4 #22

Open Tjoosten opened 3 years ago

Tjoosten commented 3 years ago

Hey @czim

It's been a long time considering to write this issue or not. But i gave a question. Can we bump the minimum required versqion of PHP to 7.4. For newer releases. Because PHP 7.2 is EOL. And no longer to supported.

I know u want to keep it to 7.2 (referring to #17). But if we support 7/4 from now on. We can clean up several (useless docblocks like this one https://github.com/czim/laravel-repository/blob/master/src/ExtendedPostProcessingRepository.php#L357-L363). And started to make u of typehint and return types.

And strip docblocks this one out. Because they are not given any more information as a typehint imo.

Does it break with older Laravel versions?

Not really IMO because the older versions of the laravel framework. Can u older versions of the packages.

Keep in mind that this is an suggestion. Im glad to hear your opinions.

Kind regards, Tim Joosten

czim commented 3 years ago

Hey Tim!

  1. I'm not against this idea in general. I think it's a good idea to upgrade packages to make use of newer PHP features and bump minimum version -- especially when packages are bound to a framework like Laravel.

  2. But... I don't consider it worth it for this package. As far as I'm considered, this is end of life. I've posted about this before in a request to update the package to support a recent Laravel version, but the short version: It's an anti-pattern to implement repository classes with this abstract, totalitarian approach in a framework that works with active record; the code is pretty ugly and overly complicated; neither my colleagues nor myself have used this package in years.

So I think the best way to upgrade to newer, healthier ways to code is to drop this package entirely. Instead, work with active record directly, write custom smaller repository classes for special use cases, use query objects, etc. -- anything but this.

If you are dependent on this package and can't or don't want to migrate away from it, I strongly advise you to fork it and rewrite it as you prefer. I'd be perfectly on board with this and would only welcome it becoming the 'official' version!

Tjoosten commented 3 years ago

Hey Czim,

Thanks for sharing your view and opinion. Here is hoe see it:

I see a future in this package and i like the approach in a strange way. Maintaining this gives me an playground as an hobby.

Op wo 3 mrt. 2021 om 19:10 schreef Conrad Carpenter < notifications@github.com>

Hey Tim!

1.

I'm not against this idea in general. I think it's a good idea to upgrade packages to make use of newer PHP features and bump minimum version -- especially when packages are bound to a framework like Laravel. 2.

But... I don't consider it worth it for this package. As far as I'm considered, this is end of life. I've posted about this before in a request to update the package to support a recent Laravel version, but the short version: It's an anti-pattern to implement repository classes with this abstract, totalitarian approach in a framework that works with active record; the code is pretty ugly and overly complicated; neither my colleagues nor myself have used this package in years.

So I think the best way to upgrade to newer, healthier ways to code is to drop this package entirely. Instead, work with active record directly, write custom smaller repository classes for special use cases, use query objects, etc. -- anything but this.

If you are dependent on this package and can't or don't want to migrate away from it, I strongly advise you to fork it and rewrite it as you prefer. I'd be perfectly on board with this and would only welcome it becoming the 'official' version!

β€” You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/czim/laravel-repository/issues/22#issuecomment-789946199, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHLF2MUCXJMU2CGFFSO7KLTBZ3TDANCNFSM4YRQC3UA .

Tjoosten commented 3 years ago

So Forget the ask in my mailed reply. Is it Allright that i also fork/maintain your listify package?

Also i ask myself. Isn't it better that i keep sending PR's cuz i don't want Theo take all your hard work.

czim commented 3 years ago

You're always free to fork any of my packages -- that's the spirit of open source and I approve of it. No worries there.

Don't consider it 'taking', but building onto what was shared in the first place πŸ‘

Tjoosten commented 3 years ago

Ha Good to hear! For me it felt As offensive to β€˜take’ old package to maintain them. And make modifications and never PR them back to the originaliteit repo!

Op wo 3 mrt. 2021 om 19:46 schreef Conrad Carpenter < notifications@github.com>

You're always free to fork any of my packages -- that's the spirit of open source and I approve of it. No worries there.

Don't consider it 'taking', but building onto what was shared in the first place πŸ‘

β€” You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/czim/laravel-repository/issues/22#issuecomment-789969447, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHLF2L5UMP7NDXCKGIYB2DTBZ7WTANCNFSM4YRQC3UA .

czim commented 3 years ago

For actively maintained packages that is definitely rude, I guess. But not in this case!

Tjoosten commented 3 years ago

Alright. Thx for your time invested in this thread. I Will start tomorrow working on maintaining this one in a fork.