search

staudenmeir / laravel-cte

Laravel queries with common table expressions
MIT License
514 stars 38 forks source link

Package forces Laravel 8 but seems to still be compatible with Laravel 7 #23

Closed swichers closed 2 years ago

swichers commented 2 years ago

The Laravel 8 commit 6da2778c78f884962c6de679ecbe98d6f3b1f49d bumped the version requirement, but there does not appear to be any code between the previous commit and now that actually needs laravel 8 to function. This change prevents this package from being added to Laravel 7.

A version requirement of ^7.0|^8.0 would allow installation on either.

I checked the changes via git diff 56794181c5e418efacb9f5fcd96a44aed6d9ab48...HEAD src

swichers commented 2 years ago

I can verify this still works on Laravel 7 by forking, updating the dependencies, and using it on a Laravel 7 project. The generated SQL is correct (assuming UNIONs are not used BUG: #24 ).

Opened a PR #25 if allowing installs on Laravel 7 is desired.

staudenmeir commented 2 years ago

The package has a separate version for every version of Laravel. You (should) get the matching version (in your case v1.3) with the installation command from the README: https://github.com/staudenmeir/laravel-cte#installation

swichers commented 2 years ago

Yeah, 1.3 is what installed, that's not the issue. The issue is the inability to bring in updates after that. As an example, 1.3 does not have materialized expression support.

The latest version seems to be perfectly compatible with Laravel 7, and I cannot find any functional or technical reason why Laravel 7 wouldn't work with these changes. A dependency of "illuminate/database": "^7.0|^8.0" will allow more people to leverage this package without resorting to workarounds until such point that something is utilized that is Laravel 8 only.

As an example, let's say you are able to find a solution for #24. That wouldn't matter for someone in my shoes, where I am working in a Laravel 7 project. I wouldn't get that fix with version 1.3, I would have to resort to a workaround (in my case, fork the project, make the dependency change, add the forked repo in place of yours) in order to bring that change in.

You're the maintainer so you can just say you don't want to, nothing I can do there, but again I want to stress there isn't a technical or compatibility reason for this restriction.

staudenmeir commented 2 years ago

I use this approach of separate versions for every version of Laravel for my packages because it makes my life much, much easier. Since new Laravel versions often/always change the requirements for PHP version and packages like PHPUnit, a new package version allows to me make clean cuts. Supporting multiple Laravel versions with different PHP and package requirements especially makes the automated testing much more complicated. I tried it in the past and it was a nightmare.

As an example, let's say you are able to find a solution for #24. That wouldn't matter for someone in my shoes, where I am working in a Laravel 7 project. I wouldn't get that fix with version 1.3, I would have to resort to a workaround (in my case, fork the project, make the dependency change, add the forked repo in place of yours) in order to bring that change in.

I always backport fixes and new features to older versions if someone asks for it. In cases like yours where I already know that the issue's author is using an older Laravel and package version, that branch also gets a release by default. For #24, I already prepared the 1.3 branch yesterday.

In short, the approach saves me valuable time while not having any disadvantages for the package users besides having to ask and wait a few days for a backport.