-
Notifications
You must be signed in to change notification settings - Fork 4
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 compatibility #17
base: master
Are you sure you want to change the base?
Ss4 compatibility #17
Conversation
@patricknelson Think this could be merged? Then I have a PR for Silverstripe 5 support as well |
Oh yikes, I totally forgot about this. We sadly haven't even moved to SSv4 yet @wilr have you tested this? @PawelSuwinski can you correct the spelling namespace? Please use I can try to do more of an in-depth review later as well but it'll be a free time thing if I can find it, but I am willing to rubber stamp this after confirming that it's versioned out appropriately Edit: Correction, I see |
Done. Sorry for that 😳 , it was some bulk insert mispelling I guess. |
Sure, I get it! I know it's a bit annoying, but since it's important, I have to point out: It's @wilr would you mind giving this a gander really quick too? But yeah will definitely want to go through some in-depth code review on this when I have time to setup a new SS install (goal would be to tackle in the next few weeks but feel free to ping me again within the next month just in case). |
Done. |
Hi
SS4 compatibility upgrade.
I followed steps and considerations from docs on:
bypassed in most cases solving of any
@deprecated 5.0
andTODO
parts.What is working:
I think that most migration work is done, but because of unit tests low coverage (TODOs in the code) and I don't have yet any more complex migrations tasks (I didn't use it before) please consider it as a beta version for the next release that needs few further tests.
Fixes #15