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-16935 Fix inconsistent method name #7049

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Jul 24, 2023

@quaff
Copy link
Contributor Author

quaff commented Jul 25, 2023

@sebersole Please review.

@Sanne
Copy link
Member

Sanne commented Jul 26, 2023

That looks reasonable, thanks!
I'll let @sebersole decide about the final method name, but could you re-introduce the old method name as a deprecated delegate please? Otherwise I'm not sure which platforms we might break, and we wouldn't want to do that in a micro.

@quaff
Copy link
Contributor Author

quaff commented Jul 27, 2023

That looks reasonable, thanks! I'll let @sebersole decide about the final method name, but could you re-introduce the old method name as a deprecated delegate please? Otherwise I'm not sure which platforms we might break, and we wouldn't want to do that in a micro.

@Sanne Updated by introducing deprecated default method as your suggestion.

@quaff
Copy link
Contributor Author

quaff commented Aug 16, 2023

It's not just renaming but also logic error fix, please check it ASAP. @Sanne @sebersole

@Override
public boolean disallowExtensionsInCdi() {
return allowExtensionsInCdi;
}

@@ -243,5 +243,15 @@ default boolean isXmlMappingEnabled() {
/**
* Check to see if extensions can be hosted in CDI
*/
boolean disallowExtensionsInCdi();
boolean isAllowExtensionsInCdi();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an extremely awkward-sounding name.

How about something like isCdiIntegrationEnabled(). WDYT, @sebersole?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an extremely awkward-sounding name.

How about something like isCdiIntegrationEnabled(). WDYT, @sebersole?

Please keep in mind AvailableSettings.ALLOW_EXTENSIONS_IN_CDI is public, consistency or compatibility will be broken if renaming is made again.

String ALLOW_EXTENSIONS_IN_CDI = "hibernate.cdi.extensions";

@quaff
Copy link
Contributor Author

quaff commented Sep 5, 2023

What's the decision?

@sebersole
Copy link
Member

Sorry, missed the notifications from this. 👍 Jan is going to rebase this and apply

@jrenaat
Copy link
Contributor

jrenaat commented Oct 11, 2023

Sorry, missed the notifications from this. 👍 Jan is going to rebase this and apply

Done.

@beikov beikov merged commit f309a88 into hibernate:main Oct 11, 2023
21 of 23 checks passed
@quaff
Copy link
Contributor Author

quaff commented Oct 16, 2023

It should be backported to 6.2.x and 6.3.x since it's bugfix.

@mgvinuesa
Copy link

Hello, I think this change it is related with this bug

https://hibernate.atlassian.net/jira/software/c/projects/HHH/issues/HHH-16881?filter=allissues

I don't know if you have time to check it, but currently it is not possible to use RevisionListener with Spring Boot 3.1.X

Thanks in advance

@quaff
Copy link
Contributor Author

quaff commented Oct 17, 2023

Hello, I think this change it is related with this bug

https://hibernate.atlassian.net/jira/software/c/projects/HHH/issues/HHH-16881?filter=allissues

I don't know if you have time to check it, but currently it is not possible to use RevisionListener with Spring Boot 3.1.X

Thanks in advance

I missed checking Helper.allowExtensionsInCdi().

@mgvinuesa
Copy link

I created a PR yesterday

#7446

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