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

SS4 Upgrade #137

Merged
merged 7 commits into from Feb 19, 2018

Conversation

3 participants
@sheadawson
Copy link
Contributor

sheadawson commented Feb 8, 2018

No description provided.

@sheadawson

This comment has been minimized.

Copy link
Contributor Author

sheadawson commented Feb 8, 2018

Some things included that were missing from #136

  • css and js moved to client/css, client/js
  • php files moved to src folder rather than code folder
  • php class namespaces include class type eg Model, Controller etc
  • update routes.yml to use namespaced classes
  • provide legacy.yml for classname_value_remapping of models on dev/build
  • lang files updated to namespaced classes
  • tests upgraded
  • Fixed the issues raised in discussion on #136
  • Updated calls to the Email api to suit changes

I haven't tested everything. This is as far as I've got and will get as I've run out of time. We are about to use this in production, all the functionality we are using is working well. I couldn't get the unit tests to work, it didn't seem to like the member-many-many-groups relationship defined in the yml fixture.

@nglasl

This comment has been minimized.

Copy link
Member

nglasl commented Feb 9, 2018

Thanks @sheadawson! I will be reviewing this early next week.

@nglasl

This comment has been minimized.

Copy link
Member

nglasl commented Feb 12, 2018

Throwing some findings here after playing around for a little.

  • some incorrect Email::class replacements
  • should aim to remove the SS4 progress
  • this should be a vendor module, and requirements updated to reflect this
  • password field has no default title
  • confirmation says you're logged in, but you're not actually logged in
  • viewing my profile shows multiple password fields (three including the confirmation)
  • $Member variables don't resolve, so the email says Dear $Member.Email
  • swapping to confirmation mode, i get a confirmation link that sends me to a page not found (seems like the wrong email has been sent)
  • the unit tests aren't working (as you've mentioned), which is the most important thing for an SS4 module from my upgrade experience (would be great if they covered a few more things too)
  • 2.0 branch to be created on merge

Going to leave this open for now, but I really do appreciate the work you've put forward here, so thanks heaps! Ideally we'll resolve some of these more important issues and get the tests running prior to tagging a stable release. When I have some more availability, I'll have a look at doing so. In the meantime, happy for anyone else to contribute here!

@JorisDebonnet

This comment has been minimized.

Copy link
Contributor

JorisDebonnet commented Feb 16, 2018

Awesome that this is in progress already! Maybe if it builds, it can already be merged into a new 2.0 branch of this plugin which can then get an alpha release, etc so that multiple people might more easily help bring it towards a stable release?

(edit - oh I just noticed now that there are already 2.0.x tags, so I guess some other version number then :D )

@JorisDebonnet

This comment has been minimized.

Copy link
Contributor

JorisDebonnet commented Feb 16, 2018

Upon closer examination, I would suggest:

  • Remove branch 'development'
  • Rename branch 1.1 to 2.0 (just for consistency), update that fact in README and release 2.0.3
  • Merge this PR into branch master, alias with 3.0 or 4.0 (doesn't hurt) in composer.json, maybe don't tag a release yet, but note 4.* compatibility in composer.json and README (so people can install this as dev-master or such)

What do you think? :)

@nglasl

This comment has been minimized.

Copy link
Member

nglasl commented Feb 19, 2018

Hi @JorisDebonnet, great idea, I'll merge it through so we can consider this being an alpha/beta release (updating the documentation to reflect this). The current version will become a new 2.0 branch, since this is already aliased as 3.0.

@nglasl nglasl merged commit b5acebe into symbiote:master Feb 19, 2018

@nglasl

This comment has been minimized.

Copy link
Member

nglasl commented Feb 19, 2018

Noting here that I've corrected the email replacements and updated the documentation to reflect that the current version is alpha (pointing to this PR for known issues).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.