Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Upgrade Instructions for v1.1.12 do not include CActiveRecord::resetScope() change #1256

Closed
gtcode opened this Issue Aug 20, 2012 · 7 comments

Comments

4 participants

gtcode commented Aug 20, 2012

In v1.1.12, CActiveRecord::resetScope() added a default parameter. This was not documented in the framework's UPGRADE file, and breaks subclasses that override resetScope().

It might be a good idea to check v1.1.12 for any other changes to default parameters that were missed in UPGRADE.

@ghost ghost assigned samdark Sep 9, 2012

@samdark samdark closed this Sep 9, 2012

Contributor

rawtaz commented Oct 28, 2012

I think it is generally good practice not to break the API/BC in minor version changes. Such things should be done in 1.1 -> 1.2 instead.

This one was not a big deal for me personally, one just needs to update overridden method signatures, but I think bigger productions than my flexible projects might feel differently about it.

Contributor

creocoder commented Oct 29, 2012

I think it is generally good practice not to break the API/BC in minor version changes.

There is no API break for 100% apps and there is no BC break for 99%

and breaks subclasses that override resetScope()

Can't even imagine app which decide to override resetScope() and this is STILL good app design.

Contributor

rawtaz commented Oct 29, 2012

@creocoder Not quite. The API and its BC is broken, that's a fact. What you are highlighting is another discussion, namely the one about "how bad is it that the API broke?" and "how many apps/users are affected by it".

I'm not going to say it's horribly bad in practice, because I don't have any resetScope() overridden and I think you are right in that not many are affected by it. I just think it is fundamentally wrong to break the API in minor version changes and I think that's the general opinion, so I wanted to highlight this in case it wasn't clear to developers that it had happened here or it hadn't been considered thoroughly.

An API breaking like this signals that the API cannot be trusted (which, after all, as a big part of what an API is all about), and for a lot of projects it's not so much about whether they're affected by such things in practice, as it is about policy decisions. If a project shows that it isn't going to maintain a stable API throughout minor versions, that's something a lot of people would consider a warning sign, and they might even question the seriousness of the project.

Just saying it shouldn't be taken lightly, even if most of us feel it isn't a practical issue in our small or flexible projects.

Contributor

creocoder commented Oct 29, 2012

@rawtaz

I'm not going to say it's horribly bad in practice, because I don't have any resetScope() overridden

Then what are you worried about?

I just think it is fundamentally wrong to break the API in minor version changes and I think that's the general opinion

This is known.

and I think you are right in that not many are affected by it

Moreover, i think 0 apps affected.

There is no API break. There just method signature extending. This hope many many times in minor framework version. That the first time you have noticed it or what? The probability that the method is overriden is 0%.

Contributor

creocoder commented Oct 29, 2012

If a project shows that it isn't going to maintain a stable API throughout minor versions, that's something a lot of people would consider a warning sign, and they might even question the seriousness of the project.

Of you would have made a great manager or advertiser. So convincing! But not in the case of this small change.

Owner

samdark commented Oct 29, 2012

@rawtaz We're well aware of what API breakage means and not breaking it if there's no serious reason to do it. Anyway, thanks for reminder :)

Contributor

rawtaz commented Oct 29, 2012

@creocoder Reading your comments in this thread, it is clear that you are a truly amazing person. I can tell there is no use in further commenting on what you have said, so I shan't.

@samdark Thank you. I'm sure you know what it means, I was just surprised to see it happen. If noone but me cares, all is well. Peace!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment