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 compatibility #17

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

PawelSuwinski
Copy link

@PawelSuwinski PawelSuwinski commented Jun 6, 2022

Hi

SS4 compatibility upgrade.

I followed steps and considerations from docs on:

bypassed in most cases solving of any @deprecated 5.0 and TODO parts.

What is working:

  • units test - all green
  • some basic migrations task works on SS 4.9 as expected

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

@wilr
Copy link

wilr commented Sep 11, 2024

@patricknelson Think this could be merged? Then I have a PR for Silverstripe 5 support as well

@patricknelson
Copy link
Owner

patricknelson commented Sep 11, 2024

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 PatrickNelson instead 😄

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 (i.e. since it relies on some namespaces that aren't present in SS v3 but our deps rely on v3 in this repo, it'll need to point to v4+) and if @wilr is willing to give it a 👍.

Edit: Correction, I see composer.json appears to reference 4.9.0

@PawelSuwinski
Copy link
Author

@PawelSuwinski can you correct the spelling namespace? Please use PatrickNelson instead 😄

Done. Sorry for that 😳 , it was some bulk insert mispelling I guess.

@patricknelson
Copy link
Owner

Sure, I get it! I know it's a bit annoying, but since it's important, I have to point out: It's PatrickNelson, not PatricNelson. Please include the missing k there after the c. 😄

@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).

@PawelSuwinski
Copy link
Author

Done.

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

Successfully merging this pull request may close these issues.

SilverStripe v4 compatibility
3 participants