Laravel-Backpack / PermissionManager

Admin interface for managing users, roles, permissions, using Backpack CRUD
http://backpackforlaravel.com
Other
516 stars 166 forks source link

[untested] restored Backpack `^v4.1` compatibility #290

Closed olipayne closed 2 years ago

olipayne commented 2 years ago

WHY

6.0.12 introduced a breaking change where it dropped support for Backpack v4

HOW

How did you achieve that, in technical terms?

Since backpack_pro() only exists in v5+, this adds a additional clause to include the previously available functions.

Is it a breaking change or non-breaking change?

Non-breaking

How can we test the before & after?

If it builds with v4, that'll do it.

welcome[bot] commented 2 years ago

BOOM! Your first PR with us, thank you so much! Someone will take a look at it shortly.

Please keep in mind that:

Thank you!

-- Justin Case The Backpack Robot

pxpm commented 2 years ago

Hey again @olipayne 👍

I think it would be best for us if we revert the commits for v5 and re-tag the v5 branch with 7.x. So it will be 4.1.x uses up to 6.x and v5 the new one.

What do you think @tabacitu ? Can we do it or you foresee any problems ?

olipayne commented 2 years ago

Absolutely agree, either major version bump or make the change non-breaking. Now you suggest it, major bump makes a lot more sense for simplicity. Thanks for the review! Feel free to resolve this ticket when you don't need it anymore.

tabacitu commented 2 years ago

Hey guys,

Thanks for the PR @olipayne . I'm not sure we should go back to support both v4 and v5 for this package...

Dropping support for Backpack v4 was intentional:

GRANTED. This dropping-support-in-a-patch-release puts us in a corner if we ever need to add or fix something for v4 users. But since v4.1 only supports Laravel up to v8, and Laravel 9 LTS just launched... I think fewer and fewer people will use L8...

You guys think we should support v4 too? Why?

pxpm commented 2 years ago

I don't think we should support v4 going foward. We have a lot of work to do in v5, I am pretty sure we are not coming back to 4.1 unless for security reasons. Like you said, I just think a good separation will make our life easier in the future in case we ever got to that corner. Maybe not worth the time doing something about this rigth now and just hope for the best? 🙏

olipayne commented 2 years ago

I was much more opinionated about this issue when I was still running Backpack v4, however now I'm really not too bothered. If we want to follow proper Semver guidelines then this breaking change warrants a major release (also to reduce headaches when it comes to backporting security fixes). However that risk is indeed quite low, and the guideline would then just be "upgrade to v5".

I'll close this PR, but I appreciate the discussion!