timgws / QueryBuilderParser

A simple to use query builder for the jQuery QueryBuilder plugin for use with Laravel.
MIT License
159 stars 65 forks source link

make compatible with future Laravel updates #42

Closed crowdco-developer closed 4 years ago

crowdco-developer commented 5 years ago

I have tried making changes to the compatible versions so a PR is not needed for every version.

lionslair commented 5 years ago

Not sure what the conflict is about

timgws commented 5 years ago

@lionslair this might sound crazy, but I would prefer to have to make a new PR for each new version.

When making a pull request, Travis CI will automatically run the test suite to ensure that QueryBuilderParser actually still continues to work.

I use this package in a number of Laravel projects, that are usually bumped to a recent version of Laravel a couple of weeks after a new version has been released, so it usually does not take too long to get updated.

Also, if this package does block someone from upgrading Laravel, it is 100% possible for them to open a pull request.

This package is used to filter data in Laravel with (jQuery) QueryBuilder. The last thing I want is incorrect queries that are built by QueryBuilderParser. Failing early ("QueryBuilderParser does not support this version of Laravel") is preferred to successfully installing, but generating incorrect (Laravel/Illuminate) QueryBuilder results.

I would much prefer Composer to not allow installing this package on new un-vetted versions of Laravel. The alternative (which I am open to) would be to change the master branch to not point to a version of Laravel, have a new development branch, which would be the primary branch used by tags/releases.

Then, with composer, you can request to install the dev-master version.

https://github.com/timgws/QueryBuilderParser/blob/b0b5624ca53dcdc20d3f595437f63396cac8f218/tests/QueryBuilderParserTest.php#L59-L67

I'm happy to have an issue opened to however change my opinion.

lionslair commented 5 years ago

At the moment the only thing stopping it even being tried is the version number. We have the same discussion on pwnd validator https://github.com/valorin/pwned-validator/pull/8

Maybe dev-master is the best solution. Only other argument is packages from spatie are also using ^6.0 https://github.com/spatie/laravel-medialibrary/blob/master/composer.json

timgws commented 5 years ago

At the moment the only thing stopping it even being tried is the version number. We have the same discussion on pwnd validator

The supported version lists 6.1.

https://github.com/timgws/QueryBuilderParser/blob/b0b5624ca53dcdc20d3f595437f63396cac8f218/composer.json#L23-L25

Shkeats commented 4 years ago

@timgws Is it not the case that since version 6, any 6.1, 6.2, 6.3 release of illuminate/database will be a non breaking one? https://laravel.com/docs/6.x/releases#versioning-scheme

kaytotes commented 4 years ago

Yeah now Laravel has gone to semantic versioning it's safe to bump for 6.x.0 updates.

david-fanin commented 4 years ago

Hello, I cannot install your package with Laravel 6.3.0 Can you update the composer.json file? Thanks!

timgws commented 4 years ago

As soon as the tests pass, the version will be bumped.