Migration to Play 2.5 #298
Conversation
| } | ||
|
|
||
| protected Application getApplication() { | ||
| return application; | ||
| } | ||
|
|
||
| @Override | ||
| public void onStart() { | ||
| protected void onStart() { | ||
| if (PlayAuthenticate.hasUserService()) { | ||
| final String oldServiceClass = PlayAuthenticate.getUserService().getClass().getName(); | ||
|
|
There was a problem hiding this comment.
i think this should be renamed to AbstractUserService
There was a problem hiding this comment.
Roger. Makes sense.
|
other than the little things i pointed out, good work!! On 3/21/16, pdolega notifications@github.com wrote:
|
|
nice work @pdolega |
|
ping! @pdolega :) |
pdolega
force-pushed
the
play-2.5
branch
2 times, most recently
from
c941d87
to
e152960
Compare
| @@ -2,6 +2,8 @@ organization := "com.feth" | |||
|
|
|||
| name := "play-authenticate" | |||
|
|
|||
| javacOptions ++= Seq("-Werror") | |||
There was a problem hiding this comment.
added this so that any warning makes compilation fail
There was a problem hiding this comment.
There are still lots of deprecation though - but I'd rather take care of them after this PR is merged (it's already quite big, didn't want to make things even more complicated)
|
Status is:
|
| import com.feth.play.module.pa.user.AuthUser; | ||
| import com.feth.play.module.pa.user.AuthUserIdentity; | ||
| import com.google.inject.Inject; | ||
|
|
||
| public class TestUserServicePlugin extends UserServicePlugin { |
There was a problem hiding this comment.
lets make this class simply TestUserService
There was a problem hiding this comment.
Done - still working on testing though.
|
So status is - code compile, test app working, unit tests working. Gotta see not samples and make them work and then make code a little bit nicer (probably some namings could be improved). |
| @@ -576,7 +514,7 @@ public static Result handleAuthentication(final String provider, | |||
| } | |||
| } | |||
|
|
|||
| public static AuthProvider getProvider(final String providerKey) { | |||
| public AuthProvider getProvider(final String providerKey) { | |||
| return AuthProvider.Registry.get(providerKey); | |||
| } | |||
| } | |||
There was a problem hiding this comment.
Brilliant! on the conversion of this class to DI
|
LGTM. @joscha |
|
Guys, let me look first at samples (before merging) - they stopped working definitely. I should be able to do this in two days max. |
|
Update: one of the samples upgraded, readme files updated. Everything seems to work. Last thing that seems to be missing is play-authenticate-usage sample (which I am right now half-way through) |
|
that one will be the most challenging as there's a lot to migrate in it. good work |
|
I am on it - trying to squeeze it somehow between may other daily responsibilities. I am nearly there - needed to migrate Deadbolt and eBean too unfortunately (both were not working with 2.5) |
|
OK - so basically everything is now upgraded. play-authenticate-usage also seems to work. The only thing that is left is running unit tests in this particular sample project. I think it should work (classes are loaded correctly etc) - the problem I have is probably more correlated with selenium test. Here is the exception I am getting (below). I think it may be worth to look at this issue on separate PR as this one is already kinda huge. If so, let me know what comments you guys @oexza / @joscha may have so I could fix them. Also let me know if you want to have commits squashed. Exception I am getting from selenium: |
pdolega
changed the title
|
Awesome! there's no need for a new issue as the problem is a part of this PR, squashing the commits would be good since this PR solves ONE problem "migrating to play 2.5" but then i doubt if its neccessary i'd leave that to @joscha. what version of selenium is in the class path? the problem may be related to this issue SeleniumHQ/selenium#1431 upgrading to the latest selenium driver may solve it. |
|
Good Good Good work! LGTM @joscha On 4/2/16, pdolega notifications@github.com wrote:
|
|
Really good! I will create a new branch and a Snapshot Release later today. |
|
ping! @joscha 😄 i'm looking to submit a PR this week that will spring board from this pr |
|
|
|
@joscha I'll take a look. Tests were passing on this PR here: https://travis-ci.org/joscha/play-authenticate/builds/120244214. I'll take a look within 1 hour |
|
the tests were not run there, as they are only run when the secret vars are available. It's really strange, when I run them locally: they are not even found. |
|
You can run them with something like this: the error is actually that the config can not be found: so proper credentials are most likely not needed. |
|
I believe it has something to do with how the configuration was amended in 2.4.x (most likely changed in 2.5.x): https://github.com/joscha/play-authenticate/blob/master/samples/java/play-authenticate-usage/test/test/GoogleOAuth2Base.java#L15-L17 however as the tests won't run for me locally at the moment, I can't reproduce it. |
|
@pdolega i think part of the problem is the build.sbt files of both the core and play-authenticate-usage example should have |
|
@oexza I tried upgrading easymail to 0.8.0-SNAPSHOT, there was a compile
|
|
Sounds good, I think it would be great if we could keep backwards
|
|
@joscha Yeah that would be nice (especially as this is actually minor version number that changed). Unfortunately even play-framework itself doesn't follow semver - I think it's impossible to keep backward compatibility here as plugin class disappeared (and whole plugin mechanism). |
|
yeah it would be nice to, unfortunately we can't for the reason On 4/5/16, pdolega notifications@github.com wrote:
|
|
A thought - perhaps it may be worth to switch versioning scheme of play-authenticate and easymail to be aligned with play-framework versioning number. Would make compatibility more explicit... |
|
Yes, deadbolt did that as well - there is nothing that keeps us from it I
|
|
|
Upgrading project to Play 2.5 version.
Basically the the main theme is to switch to DI completely and get rid of
Plugin.Things to do: