Restore version 1 compatiblity

[ipundit]

The move to version 2 included adding new public functions to Model, as well as routing the implementation of find() and count() to the new Relation class. But in doing that, it broke version 1 compatibility and those tests have been removed from the test suite. Version 2 has feature parity with version 1, but users would have to rewrite their calls to find() to use where() and find() rather than Model::find() doing that for them.

PR #45 had backwards compatibility with version 1, demonstrating that with difficulty, it was possible to do, but PR #63 elected to go with code cleanliness and was merged instead. The maintainers have the right to do that, but users, including this one who wrote the original PR, also have the right to not upgrade to version 2. Our production code in the existing version 1 paradigm is working just fine. I suspect that many legacy users would make the same business decision as the changes required to move to version 2 are substantial, and database code is mission critical, and should not be touched unless it's necessary.

Given that business decision, it doesn't make sense for developers like me who rely on the version 1 API to contribute to this project, especially since PR #87 was closed unilaterally without warning and the code there won't be accepted. Again, that is the maintainer's right to do that, and any future contributors should be oriented to that policy in contributing.md and then decide whether it's worth contributing.

[shmax]

The thing you have to understand is that even if we rolled things back or tried to keep the old Model::find behavior, v2.x still wouldn't be backwards-compatible with v1.x. I've already mentioned the changes to the relationship API, but there's also the matter that the code has been refreshed to use PHP 8.1 language features, so if you have some code that depended on v1.x, it likely won't be as simple as bumping your active-record version, even if the old API was backwards-compatible.

I'm doing my best to understand your gripe, here, but I'm not sure I grasp the essence of what it is that's bothering you. If you feel that migrating to 2.x is going to be too much of a burden to you, then don't do it, right? Version 1.x will always be available, and I'm not really aware of any bugs.

So, why would one migrate to 2.x? For the new features. If you want to use the new features, then you should be prepared to migrate your code (which, in my opinion, should be pretty easy; all you really have to do is move any conditions currently being passed to find calls to a where clause--maybe we could even write a simple migration tool that does it for you).

It sounds like you want to consider the third case, which is where you want to use the new features, but not the new API. The problem there is that the new feature IS the new API. Correct me if I'm wrong, but there aren't really any new features that don't revolve around the new Relation object, are there?

I guess we could once again discuss the notion of supporting the passing of conditions to Model::where and Model::find, but again, why would one bother upgrading to 2.x only to pointedly dodge the new API and keep doing things the old way? Unless I'm missing something, there are really no new features or fixes here outside of the new Relation stuff, general cleanup, static checking, and the addition of PHP 8.1 language features, so if one isn't interested in the new API, there is no reason to upgrade.

As for #87 , I'm not sure what this has to do with the bc discussion, but your code was merged, only without all the stuff unrelated to the issue it was fixing. The other features are still welcome, and if you would like me to work my magic and extract them into a new branch for you, I'm only too happy to do so.

At any rate, I'm still open to whatever ideas you have about migration to 2.x. Even prepared to do a zoom call if you like.

[ipundit]

The most user-friendly way forward, and which happens to be widely practiced, is to break as little as possible when upgrading and emit deprecation notices, at least in the documentation, and if possible, suppressible ones in the code. Then in version 3, you can fully deprecate as you have done because you've given sufficient warning. It takes time for us users to upgrade to new libraries that change their API; the whole PHP 8 deprecation notices that originally drew me into this project is a model of this upgrade policy where release cycles and their potential breakages are communicate well, and are thus easier to manage and integrate.

If it's relatively painless for version 1 users to upgrade to version 2, then many of us will do so. That means keeping the old API and functionality of Model::find() and Model::count(). If you give us time to fix our code to be version 2 compliant without everything breaking as soon as we upgrade, then many of us will do and you'll get what you want by version 3. Managing, maintaining and transitioning away from legacy code is an unpleasant reality for vendors of widely used libraries (even if that library is purely internal to a company). But if a vendor shows willingness to break things on every version upgrade without suitable warning or an easy transition path, then it's time to stay on an old version of the library until the dependency changes forces us to upgrade, possibly to another vendor. This is not anything personal, but is a business decision for what is best for the company's who values stability in an API more than convenience for legacy code.

If I were to start a new project today, I would use the latest version of a library. But for all legacy code that I maintain, I'm stuck on old versions of a library because it's cheaper to stick with what's working than upgrading. Make the cost of upgrading small, or preferably zero, then I'll upgrade. But if not, which is fully in your purview, then I'll stick with version 1, and I'm sure a large percentage of your user base would make the same decision. Is code cleanliness worth the loss of so many users, who also happen to be potential contributors?

For #87, if you want the free contribution for those features, please feel free to modify and then merge them into master, but the PR is closed so I won't be pursing the matter further. They come with new test cases that pass regression, and conform to Rail's API except for the weird where.not() behavior as discussed in #83.

[shmax]

The most user-friendly way forward, and which happens to be widely practiced, is to break as little as possible when upgrading and emit deprecation notices, at least in the documentation, and if possible, suppressible ones in the code. Then in version 3, you can fully deprecate as you have done because you've given sufficient warning. It takes time for us users to upgrade to new libraries that change their API; the whole PHP 8 deprecation notices that originally drew me into this project is a model of this upgrade policy where release cycles and their potential breakages are communicate well, and are thus easier to manage and integrate.

Yes, that's a thing, and would make sense if we were going to stay in the 1.x major version. But we are fated to do a major release bump and there's just no getting around it (even if we tweaked find and count the way you likely have in mind). If we wanted to do a backwards-compatible release, here's what needs to happen:

1. we modify Model::find and Model::count. This is the easy part.
2. we roll back the changes to the relationship stuff. To be clear, I'm not talking about Relation, here, but rather the has_many, belongs_to, etc stuff.
3. we roll back all the static analysis stuff (that I spent weeks on). The only way to make modern PHPStan happy is to annotate all the class members and return types, and this functionality was not available in PHP versions prior to PHP 7.4, so all of that needs to come out.

I suppose we could argue about 2, but if we can agree that 3 is not going to happen, then we are locked into doing a major release bump, and major release bumps are where bc-breaking changes are permitted.

And just to state it once more, other than the Relation stuff, there is no new functionality in the upcoming 2.x release, so this isn't a case where one would bump to the next major version to make use of some other new feature, and also possibly opt-in to the Relation stuff. If you aren't interested in the new API, then there is positively, absolutely no reason to move to 2.x.

If you're still not convinced and are hell-bent on restoring condition support for find and count out of sheer cussedness, then by all means open a PR, but do everything in your power to make it clear that the feature is terminally-deprecated, and you will be responsible for making any necessary modifications to the PHPStan extensions.

[shmax]

To put it another way, even if we had never introduced Relation, the code would still not be backwards-compatible.

[ipundit]

For your three points:

1. Modifying Model::find() and Model::count() to be backwards compatible shouldn't be hugely difficult, but it would make upgrading to version 2 much easier because these are the core features of ActiveRecord. The $model->mixedCaseNameSupport was a great enough convenience upgrade that that I submitted code from my personal fork to this project and that got merged. It would be a 1.x or higher compatible upgrade.
2. Can explain more about what breaking changes there are to has_many and belongs_to stuff? As a maintainer of legacy code, I have no problem with version 2 adding features, but do have problems that my existing code breaks.
3. https://packagist.org/packages/php-patterns/activerecord says we're on version v1.1.11, and it requires readme.md says it requires PHP 8.1 or later. (Is that true actually? Or does it work on PHP 7.4 too? The changes I personally made to resolve the 8.2 deprecation notices were backwards compatible with PHP 7.4) Version 2 also requires PHP 8.1 or later, so I don't think that's an issue. Thus, I think we can keep static checking. For people who want to stay on PHP 7.x, then they're stuck with ancient versions of libraries, including ActiveRecord.

[shmax]

The $model->mixedCaseNameSupport was a great enough convenience upgrade that that I submitted code from my personal fork to this project and that got merged. It would be a 1.x or higher compatible upgrade.

You're the very first person to complain about this in 13 years, but if you feel it is a high-value fix, then you are free to start a 1.x branch, and backport your fix (it won't be easy).

Can explain more about what breaking changes there are to has_many and belongs_to stuff? As a maintainer of legacy code, I have no problem with version 2 adding features, but do have problems that my existing code breaks.

Yep. Before, you would set up has_many like this:

class Person extends Model 
{
  static $has_many = [
    'courses',
    "through" => "school",
    "primary_key" => "course_id"
   "
  ]
}

Now, we employ a key=>config structure, like this:

class Person extends Model 
{
  static $has_many = [
    'courses' => [
      "through" => "school",
      "primary_key" => "course_id"
   ]
  ];
}

The latter can be statically-checked (array<string, HasManyOptions>). For the former, the best we can do is array<mixed>.

(Is that true actually? Or does it work on PHP 7.4 too?

I've done a few little fixes for 8.0 deprecation notices. So, the 1.1 code is compatible with both 8.0 and 5.4. The new code on master, however, is not compatible with anything prior to 8.1 because:

• union types

and not compatible to anything prior to 7.4 because:

• coalesce operator
• typehinted members

[ipundit]

For the the three points, your answers are encouraging.

1. Model::find(), count(), all(), first(), last() should be fairly easy to make backwards compatible as it's just converting the conditions into where() clauses, calling the existing version 2 implementation, and restoring the old test suite for version 1 compatibility. The implementation can be version 2 under the hood. In fact, why deprecate support for the old Model behavior at all? It's compliant to Ruby on Rails version 3 or 2, while the new Relation stuff is Ruby on Rails 4 or higher.
2. Converting from one structure to another is not difficult to do. Emitting a deprecation notice would be appropriate, but a hard "everything dies when I upgrade" will cause many to not upgrade.
3. So readme.md for version 1.1.11 should say PHP compatibility between 5.4 and 8.0, and version 2 is 8.1 or higher. However, I am on 1.1.11 now in my production code and it's working great for PHP 8.2. I believe you've promoted the deprecation fixes to be 8.2 compatible, so shouldn't 1.1.11 say PHP >= 5.4 rather than the PHP >= 8.1 that it says now?

So if it's not a lot of work to restore backwards compatibility, but it will encourage and ease a lot of your existing user base to upgrade, then why not do it? You have decide whether having a useful library for many customers vs code cleanliness is a higher priority. Most employers, of which most of us are accountable to, would understandably prioritize what customers want over what a developer believes is code cleanliness.

[shmax]

So if it's not a lot of work to restore backwards compatibility

Yes, it sure is, if you mean "backwards compatible" in the semver sense. Supporting the passing of conditions to find and count will not make 2.x "backwards compatible" with 1.x. Because:

1. the code has been bumped to PHP 8.1 language features. It absolutely will not run on any box that doesn't have PHP 8.1 installed. You have to let that sink in for us to continue discussing this rationally. edit: if it sounds extreme, bear in mind that 8.1 is now nearly 2 years old.
2. I've changed the syntactical behavior of has_many|belongs_to, etc. If you want to work out a way to make that "backwards compatible" it would completely undermine all the static checker work I've done around those features. Yes, we could determine the shape of the properties at runtime and branch accordingly, but the static checker wouldn't be able to sort it out, which means the only way to resolve it would be to roll back the changes.

Let me put it this way: for me, all the static checking upgrades and language refresh stuff is the main selling point of 2.x, and was always going to be a breaking change. When you became interested in the Relation stuff I was happy to run with it because we were already doing a release with breaking changes, anyway, so in for a penny, in for a pound.

Granted, there are going to be a class of people (like myself) that are currently using a modern version of PHP with the 1.1 version of Active Record. And yes, if those people upgrade to 2.x, the first thing they're going to notice is that everything breaks. That's still going to be the case whether they have the robust versions of find and count, or not, because of the has_many|belongs_to changes. So, anyone that wants to upgrade to 2.x is going to have to take a look at the release notes, which will mention the steps needed to migrate. And since they have to do a few migration steps either way, in my opinion they may as well take the extra 10 minutes or whatever it takes to switch over to where clauses (we could potentially develop a tool that does it automatically).

I would love it if all that were enough to convince you, but if you still want to forge ahead with the changes to find and count I don't really mind, but please get "backwards compatible` out of your head and out of our discussions, as it's just not a thing.

All that said, I do have a few comments on possible changes to find and count:

1. Let's not formalize the changes in the static checker extensions. That means that anybody that upgrades to 2.x and is modern enough to be using PHPStan will run their own checker, see all the errors light up, and will be in a position to run through and fix everything.
2. You can mention 'conditions" in the documentation, but be sure to annotate them with giant @deprecated tags.
3. I'm not sure if you want to bring back the find_all_by_ branch of static methods, but if you do, again, let's leave it out of the static checker.

[shmax]

only way to resolve it would be to roll back the changes

Well, strictly speaking, I guess we could also just shunt any complaints from PHPStan over into the baseline errors file.

[shmax]

Closing due to inactivity, but feel free to reopen if you want to keep the discussion going.