Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential backward-compatibility break in 3 #4255

Closed
kinglozzer opened this issue Jun 5, 2015 · 5 comments
Closed

Potential backward-compatibility break in 3 #4255

kinglozzer opened this issue Jun 5, 2015 · 5 comments
Milestone

Comments

@kinglozzer
Copy link
Member

#2955 was merged before we started using semantic versioning, and before we created the 3 branch from master. It makes the default error handler fatal and also adds E_RECOVERABLE_ERROR as a warning (to fix #2928) - in 3.1.x those errors are silently ignored. This just came up in sheadawson/silverstripe-zenvalidator#34, so it probably would come up for users of that module when they upgrade to 3.2 (if it’s not fixed, of course).

@silverstripe/core-team Should we revert this change entirely from the 3 branch? E_RECOVERABLE_ERROR should really be fatal too (the pull request set it as a warning to reduce upgrade pains, with the intention of making it fatal for 4.0). So, I’m proposing:

  • Revert this change for 3
  • In master, move E_RECOVERABLE_ERROR to the fatal handlers block
@micmania1
Copy link
Contributor

Well...

https://github.com/silverstripe/silverstripe-framework/blob/3.1/core/manifest/ConfigStaticManifest.php#L81

I didn't think 3.2 was 100% semver compliant but if we're going that way then statics should be allowed to be public again, as they are in 3.1?

Opens can of worms and leaves

@tractorcow
Copy link
Contributor

@micmania1 in your case, it could be solved by moving the deprecation notice up to 4.0.

I'm not sure if it would fix more problems than it would cause to now re-remove the E_RECOVERABLE_ERROR. The fact that it was once a silently ignored error is more due to the fact that this (core error) wasn't even handled in 3.1. Adding this in seems to be more the correction of an oversight than a new feature.

@chillu
Copy link
Member

chillu commented Jun 8, 2015

Agree with Damian, but we should make sure its well documented in the upgrading guide for 3.2 (since we missed the boat on that for whatever 3.1 release the commit went into)

@tractorcow
Copy link
Contributor

I've added a changelog note for this issue at #4287

Can you please review and confirm this is sufficient for us to release 3.2?

@kinglozzer
Copy link
Member Author

Closed in 5a0f3c0, thanks all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants