Skip to content

Conversation

@Flowdalic
Copy link
Contributor

spring-ws needs to be upgraded to use Smack 4.0 before spring-integration needs to be upgraded in #1137

@Flowdalic Flowdalic changed the title Update Smack to 4.0.0-rc2 Update Smack to 4.0.0 Jun 8, 2014
@artembilan
Copy link
Member

@Flowdalic , thank you for contribution. I'm commenting here, because don't want to disturb @poutsma from SI PR.

I see changes to Spring WS are less, then to SI and I don't think that it will hurt to include it in point release of WS (2.2.1). It allows for SI 4.1 to be released independently of plans for WS 2.3.
@poutsma , WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Can't SmackException and IOException be wrapped with XMPPException to avoid breaking changes for end-applications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it's possible but I don't think that this is a good idea because in Smack 4 XMPPExceptions are only for errors defined in a XMPP related protocol.

@Flowdalic
Copy link
Contributor Author

Please note that any version of Spring-ws that includes Smack 4 will break any version of spring integration that does not use Smack 4.

@artembilan
Copy link
Member

will break any version of spring integration that does not use Smack 4.

It doesn't matter, because Spring Integration WS doesn't depend of Smack. We actually have an issue with versions only, if user wants to use SI-ws and SI-xmpp in the same app.

@Flowdalic, forget that versions issue - it is already our 'headache'.

In this PR I just want to have breaking changes as less as possible to allow end-users just upgrade Spring-WS and Smack. Actually the same is up to SI.

That's why we still need an input from @poutsma as a WS project lead.

@Flowdalic
Copy link
Contributor Author

Ahh right, Smack is an optional dependency in spring-ws. So I guess I leave it to you to figure out the details. Feel free to ask if there is anything else. :)

@artembilan
Copy link
Member

This one should be closed in favor of #48 .
Thank you @Flowdalic for the contribution anyway!

@gregturn
Copy link
Contributor

I actually used this PR to write the other one. Salvaging the other was too difficult with all the master changes. Thanks!

@gregturn
Copy link
Contributor

I plan to denote the commit here when completed.

@gregturn
Copy link
Contributor

Resolved via 85c4832

@gregturn gregturn closed this Jan 27, 2016
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

Successfully merging this pull request may close these issues.

3 participants