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

Bump to php 7.4 #22

Open
Tjoosten opened this issue Mar 3, 2021 · 7 comments
Open

Bump to php 7.4 #22

Tjoosten opened this issue Mar 3, 2021 · 7 comments

Comments

@Tjoosten
Copy link
Contributor

Tjoosten commented Mar 3, 2021

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
Copy link
Owner

czim commented Mar 3, 2021

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
Copy link
Contributor Author

Tjoosten commented Mar 3, 2021 via email

@Tjoosten
Copy link
Contributor Author

Tjoosten commented Mar 3, 2021

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
Copy link
Owner

czim commented Mar 3, 2021

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
Copy link
Contributor Author

Tjoosten commented Mar 3, 2021 via email

@czim
Copy link
Owner

czim commented Mar 3, 2021

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

@Tjoosten
Copy link
Contributor Author

Tjoosten commented Mar 3, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants