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

HHH-12295 Restore compatibility with Java 6 on branch 5.1 #2151

Closed
wants to merge 1 commit into from

Conversation

Sanne
Copy link
Member

@Sanne Sanne commented Feb 14, 2018

https://hibernate.atlassian.net/browse/HHH-12295

N.B. this PR is for branch 5.1 only.

@Sanne
Copy link
Member Author

Sanne commented Feb 14, 2018

@vladmihalcea , could you have a look please? I removed the Pattern.UNICODE_CHARACTER_CLASS flag in your regex but I'm not sure I understand the implications.

Pattern.UNICODE_CHARACTER_CLASS is not available in Java 6, while ORM 5.1 should still be compatible with Java 6, so this flag has to go. Maybe we need a different replacement?

@vladmihalcea
Copy link
Contributor

@Sanne I remember that I added the UNICODE_CHARACTER_CLASS after someone complained that his Java Constants written with Chinese Glyphs were not caught by that Regex.

If all tests pass just fine ReflectHelperTest#test_getConstantValue_constant_digits passes just fine, then no worries.

@Sanne
Copy link
Member Author

Sanne commented Feb 14, 2018

thanks Vlad. I'll merge this then.

@Sanne Sanne closed this Feb 14, 2018
@Sanne Sanne reopened this Feb 14, 2018
@Sanne
Copy link
Member Author

Sanne commented Feb 14, 2018

ah no.. forgot this is 5.1.

I'll let Gail merge it so she can check it all.

@Sanne
Copy link
Member Author

Sanne commented Apr 26, 2018

@gbadner ? I thought this was urgent, and should be trivial to check. Just looking for your ACK.

@gbadner
Copy link
Contributor

gbadner commented Apr 30, 2018

@Sanne , ReflectHelperTest#test_getConstantValue_constant_digits is failing.

@vladmihalcea , any suggestions?

@rstancel
Copy link

@gbadner the issue here is that the pattern is requiring class with 2 uppercase letters and the test class contains only one. So the solution can be to update test data. Something like that: rstancel@f0806b6

@gbadner
Copy link
Contributor

gbadner commented Aug 26, 2018

@rstancel, IIUC, you've just change the test code. The bug remains.

@rstancel
Copy link

rstancel commented Aug 28, 2018

@gbadner I don't understand. Sanne has fixed the issue itself but his fix was failing one test. So I took his fix and added also a fix for this test. Is there something else which should be fixed?

@gbadner
Copy link
Contributor

gbadner commented Nov 28, 2018

@rstancel, the problem is that the proposed fix to ReflectHelper will introduce a regression into 5.1 branch. Fixing the test to pass does not fix the regression.

It turns out that upgrading dom4j to 2.2.1 (HHH-12964) requires JDK8 in 5.1.17 anyhow, so it will not be possible to restore compatibility with JDK6 in 5.1.

@gbadner gbadner closed this Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants