-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
[JENKINS-64390] make FieldUtils behave as it used to #5105
Conversation
assertThrows(Exception.class, () -> FieldUtils.setProtectedFieldValue("bogus", sut, "whatever")); | ||
} | ||
|
||
class InnerClassWithPublicFinalField { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this behaved differently with the initial acegi-rewrite if it was public
Co-authored-by: Jesse Glick <jglick@cloudbees.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I rather prefer the new behavior, but regression is a regression
@@ -40,8 +42,14 @@ public static Object getProtectedFieldValue(String protectedField, Object object | |||
|
|||
public static void setProtectedFieldValue(String protectedField, Object object, Object newValue) { | |||
try { | |||
org.apache.commons.lang.reflect.FieldUtils.writeField(object, protectedField, newValue, true); | |||
} catch (IllegalAccessException x) { | |||
// acgegi would silently fail to write to final fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// acgegi would silently fail to write to final fields | |
// acegi would silently fail to write to final fields |
Well the whole class is deprecated, so the sole criterion is compatibility. |
acegi FieldUtils used to silently fail when you tried to set a field that was both
public
andfinal
Whilst it seems silly to do that (the code was obviously a no-op) I observed a breakage in a plugin (test code) that was doing exactly that. seems like there is a possibility that some production code may exist that also was doing this that would now be broken, so make the code also silently fail for that case.
See JENKINS-64390.
Proposed changelog entries
FieldUtils
now silently fails to setpublic final
fields again.Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeDesired reviewers
@jglick
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).